Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Search REST via Purl #2089

Closed

Conversation

nathannaveen
Copy link
Collaborator

@nathannaveen nathannaveen commented Aug 26, 2024

Description of the PR

PR Checklist

  • All commits have a Developer Certificate of Origin (DCO) -- they are generated using -s flag to git commit.
  • All new changes are covered by tests
  • If GraphQL schema is changed, make generate has been run
  • If GraphQL schema is changed, GraphQL client updates/additions have been made
  • If OpenAPI spec is changed, make generate has been run
  • If ent schema is changed, make generate has been run
  • If collectsub protobuf has been changed, make proto has been run
  • All CI checks are passing (tests and formatting)
  • All dependent PRs have already been merged

@nathannaveen nathannaveen changed the title Nathan/search rest via purl Search REST via Purl Aug 26, 2024
@funnelfiasco
Copy link
Contributor

while the queries like vulns or dependencies are being passed in as parameters (i.e. http://localhost:8081/v1/package/pkg%3Agolang%2Ftest-namespace-1%2Ftest-name-1?vulns=true)

I think it would be better from a usability point of view (assuming the initial way people interact with this will be manually creating the search with curl on in a browser) to have something like query=vulns instead of vulns=true, unless we expect to support querying multiple aspects in a single call (thus vulns=true&deps=true), but even then I think something like query=vulns,deps would be better. Is this something you considered and if not, does it make the implementation or maintainability more difficult?

@nathannaveen
Copy link
Collaborator Author

@funnelfiasco thank you for your input! I think this is a great idea! I never thought about passing all of the flags as a single query parameter. So, now our endpoint will look similar to: http://localhost:8081/v1/package/pkg%3Agolang%2Ftest-namespace-1%2Ftest-name-1?query=vulns,dependencies

@nathannaveen nathannaveen force-pushed the nathan/searchRestViaPurl branch 8 times, most recently from 7900dbc to fa57306 Compare September 3, 2024 20:16
@pxp928 pxp928 requested review from mdeicas and lumjjb September 4, 2024 18:41
pkg/guacrest/openapi.yaml Outdated Show resolved Hide resolved
pkg/guacrest/openapi.yaml Outdated Show resolved Hide resolved
pkg/guacrest/openapi.yaml Outdated Show resolved Hide resolved
pkg/guacrest/openapi.yaml Outdated Show resolved Hide resolved
pkg/guacrest/openapi.yaml Outdated Show resolved Hide resolved
pkg/guacrest/openapi.yaml Outdated Show resolved Hide resolved
pkg/guacrest/server/server.go Outdated Show resolved Hide resolved
pkg/guacrest/helpers/getPackageInfo.go Outdated Show resolved Hide resolved
pkg/guacrest/helpers/getPackageInfo.go Outdated Show resolved Hide resolved
@nathannaveen nathannaveen force-pushed the nathan/searchRestViaPurl branch 6 times, most recently from 8ed557a to c6f4e28 Compare October 8, 2024 17:57
@nathannaveen nathannaveen force-pushed the nathan/searchRestViaPurl branch from c6f4e28 to c7426c0 Compare October 8, 2024 18:22
@nathannaveen nathannaveen requested a review from mdeicas October 8, 2024 18:22
@pxp928 pxp928 requested a review from mlieberman85 October 10, 2024 16:41
pkg/guacrest/openapi.yaml Outdated Show resolved Hide resolved
pkg/guacrest/openapi.yaml Outdated Show resolved Hide resolved
pkg/guacrest/openapi.yaml Outdated Show resolved Hide resolved
pkg/guacrest/openapi.yaml Outdated Show resolved Hide resolved
pkg/guacrest/openapi.yaml Show resolved Hide resolved
pkg/guacrest/server/server.go Outdated Show resolved Hide resolved
pkg/guacrest/server/server.go Outdated Show resolved Hide resolved
pkg/guacrest/openapi.yaml Show resolved Hide resolved
pkg/guacrest/openapi.yaml Outdated Show resolved Hide resolved
pkg/guacrest/helpers/getPackageInfo.go Outdated Show resolved Hide resolved
@nathannaveen nathannaveen force-pushed the nathan/searchRestViaPurl branch from 98f3c71 to fd59789 Compare October 22, 2024 20:03
@nathannaveen nathannaveen force-pushed the nathan/searchRestViaPurl branch from fd59789 to 7c1f177 Compare October 23, 2024 18:15
@nathannaveen nathannaveen requested a review from mdeicas October 23, 2024 19:16
Copy link
Contributor

@lumjjb lumjjb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you do a favor to make this easier to review to split this PR into a couple smaller ones:

  1. changes to internal/testing
  2. adding the REST API in openapi.yaml and codegen
  3. Adding the implementation of the rest api function

That will be much appreciated!

nathannaveen added a commit to nathannaveen/guac that referenced this pull request Oct 25, 2024
* First Part of PR guacsec#2089

Signed-off-by: nathannaveen <[email protected]>
nathannaveen added a commit to nathannaveen/guac that referenced this pull request Oct 25, 2024
* Second Part of guacsec#2089
* All the OpenAPI Spec changes have been included
* The `/query/dependencies` is no longer going to be used, instead, the `/v0/package/{purl}/dependencies` and `/v0/artifact/{digest}/dependencies` will be replacing it.
* The code in retrieveDependencies.go has been updated to work for the new endpoints.

Signed-off-by: nathannaveen <[email protected]>
This was referenced Oct 25, 2024
@nathannaveen
Copy link
Collaborator Author

Can you do a favor to make this easier to review to split this PR into a couple smaller ones:

  1. changes to internal/testing
  2. adding the REST API in openapi.yaml and codegen
  3. Adding the implementation of the rest api function

That will be much appreciated!

@lumjjb I have created a couple small PRs to replace this one #2216 and #2217. There will be a subsequent PR after these two are merged.

nathannaveen added a commit to nathannaveen/guac that referenced this pull request Oct 25, 2024
* Second Part of guacsec#2089
* All the OpenAPI Spec changes have been included
* The `/query/dependencies` is no longer going to be used, instead, the `/v0/package/{purl}/dependencies` and `/v0/artifact/{digest}/dependencies` will be replacing it.
* The code in retrieveDependencies.go has been updated to work for the new endpoints.

Signed-off-by: nathannaveen <[email protected]>
nathannaveen added a commit to nathannaveen/guac that referenced this pull request Nov 7, 2024
* First Part of PR guacsec#2089

Signed-off-by: nathannaveen <[email protected]>
kodiakhq bot pushed a commit that referenced this pull request Nov 9, 2024
* Updated GraphQL Testing

* First Part of PR #2089

Signed-off-by: nathannaveen <[email protected]>

* Updated based on code review

Signed-off-by: nathannaveen <[email protected]>

---------

Signed-off-by: nathannaveen <[email protected]>
nathannaveen added a commit to nathannaveen/guac that referenced this pull request Dec 4, 2024
* Second Part of guacsec#2089
* All the OpenAPI Spec changes have been included
* The `/query/dependencies` is no longer going to be used, instead, the `/v0/package/{purl}/dependencies` and `/v0/artifact/{digest}/dependencies` will be replacing it.
* The code in retrieveDependencies.go has been updated to work for the new endpoints.

Signed-off-by: nathannaveen <[email protected]>
nathannaveen added a commit to nathannaveen/guac that referenced this pull request Feb 7, 2025
* Second Part of guacsec#2089
* All the OpenAPI Spec changes have been included
* The `/query/dependencies` is no longer going to be used, instead, the `/v0/package/{purl}/dependencies` and `/v0/artifact/{digest}/dependencies` will be replacing it.
* The code in retrieveDependencies.go has been updated to work for the new endpoints.

Signed-off-by: nathannaveen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants