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
Contributor

@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
Contributor 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
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
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
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
Contributor 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]>
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