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

Include Pagination for KeyValue #1904

Merged
merged 8 commits into from
May 20, 2024

Conversation

nathannaveen
Copy link
Collaborator

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 nathannaveen force-pushed the nathan/keyvalue-pagination4 branch from 6795bbd to cb890d4 Compare May 16, 2024 14:36
@nathannaveen nathannaveen force-pushed the nathan/keyvalue-pagination4 branch from cb890d4 to 648544a Compare May 16, 2024 14:37
@nathannaveen nathannaveen requested a review from pxp928 May 16, 2024 14:39
@nathannaveen nathannaveen force-pushed the nathan/keyvalue-pagination4 branch from 648544a to 28b3dc4 Compare May 16, 2024 21:13
@nathannaveen nathannaveen requested a review from pxp928 May 16, 2024 21:15
Signed-off-by: nathannaveen <[email protected]>
@nathannaveen nathannaveen force-pushed the nathan/keyvalue-pagination4 branch from 28b3dc4 to 4390123 Compare May 17, 2024 15:14
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.

Signed-off-by: nathannaveen <[email protected]>
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
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
Collaborator Author

Thank you @mihaimaruseac!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-review Needs writer LGTM size/XXL
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants