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.

Sign up for GitHub

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

Jump to bottom

Include Pagination for KeyValue #1904

Merged
merged 8 commits into from May 20, 2024
Merged

Include Pagination for KeyValue #1904

merged 8 commits into from May 20, 2024

Conversation

nathannaveen
Copy link
Contributor

Description of the PR

Included pagination for queries in KeyValue, and fixed the tests.

Part of issue #1525

Same as #1901

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 OpenAPI spec 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
Include pagination for some more commands
cfca9d5 
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
@nathannaveen
Included pagination for certifyScorecard
b612ced 
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
@nathannaveen
Included Pagination for certifyBad
82a48ef 
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
@nathannaveen
Included pagination for more commands
e1abf5a 
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
@nathannaveen
Added pagination for more queries
94e1980 
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
@nathannaveen
Fixed tests
00954b4 
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
@nathannaveen nathannaveen requested a review from pxp928 May 16, 2024 14:39
pxp928
pxp928 reviewed May 16, 2024
View reviewed changes
pkg/assembler/backends/keyvalue/certifyBad.go Outdated Show resolved Hide resolved
pkg/assembler/backends/keyvalue/certifyGood.go Outdated Show resolved Hide resolved
pkg/assembler/backends/keyvalue/pkg.go Outdated Show resolved Hide resolved
pkg/assembler/backends/keyvalue/src.go Outdated Show resolved Hide resolved
pkg/assembler/backends/keyvalue/vulnerability.go Outdated Show resolved Hide resolved
@nathannaveen nathannaveen requested a review from pxp928 May 16, 2024 21:15
@nathannaveen
Updated based on code review
4390123 
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
pxp928
pxp928 reviewed May 20, 2024
View reviewed changes
Copy link
Collaborator

@pxp928 pxp928 left a comment

Choose a reason for hiding this comment

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

Missing next page check when foundOne is set to true for all the nodes.

pkg/assembler/backends/keyvalue/certifyBad.go Outdated Show resolved Hide resolved
pkg/assembler/backends/keyvalue/certifyGood.go Outdated Show resolved Hide resolved
@nathannaveen
Updated based on code review
8695bdc 
Signed-off-by: nathannaveen <42319948+nathannaveen@users.noreply.github.com>
pxp928
pxp928 approved these changes May 20, 2024
View reviewed changes
Copy link
Collaborator

@pxp928 pxp928 left a comment

Choose a reason for hiding this comment

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

Awesome work!

@pxp928 pxp928 requested review from jeffmendoza and lumjjb May 20, 2024 16:39
@pxp928 pxp928 added the needs-review Needs writer LGTM label May 20, 2024
mihaimaruseac
mihaimaruseac reviewed May 20, 2024
View reviewed changes
Copy link
Collaborator

@mihaimaruseac mihaimaruseac left a comment

Choose a reason for hiding this comment

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

This LGTMs. Adding as a comment instead of approval in case we want other reviewers to also look at it. If not, I can approve.

@pxp928
Copy link
Collaborator

pxp928 commented May 20, 2024

This LGTMs. Adding as a comment instead of approval in case we want other reviewers to also look at it. If not, I can approve.

You are good to approve @mihaimaruseac, I just added the others for whoever could review/approve first.

@kodiakhq kodiakhq bot merged commit 0970c35 into guacsec:main May 20, 2024
8 checks passed
@mihaimaruseac
Copy link
Collaborator

Thank you @pxp928 for the thorough review and @nathannaveen for a significant amount of work for this PR

@nathannaveen
Copy link
Contributor Author

Thank you @mihaimaruseac!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mihaimaruseac mihaimaruseac mihaimaruseac approved these changes

@pxp928 pxp928 pxp928 approved these changes

@jeffmendoza jeffmendoza Awaiting requested review from jeffmendoza

@lumjjb lumjjb Awaiting requested review from lumjjb

Assignees
No one assigned
Labels
needs-review Needs writer LGTM size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nathannaveen @pxp928 @mihaimaruseac