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

feat: cursor pagination of get_all_users in /admin/users route #1424

Conversation

ajanitshimanga
Copy link
Contributor

Please describe the purpose of this pull request.
Is it to add a new feature? Is it to fix a bug?
New feature: cursor based pagination of get_all_users in /admin/users route

How to test
How can we test your PR during review? What commands should we run? What outcomes should we expect?

Have you tested this PR?
Have you tested the latest commit on the PR? If so please provide outputs from your tests.
In progress.

Related issues or PRs
Please link any related GitHub issues or PRs.

Relevant issue: #1156

Is your PR over 500 lines of code?
If so, please break up your PR into multiple smaller PRs so that we can review them quickly, or provide justification for its length.

Not more than 500 lines.

Additional context
Add any other context or screenshots about the PR here.

@ajanitshimanga ajanitshimanga changed the title feature: cursor pagination of get_all_users in /admin/users route feat: cursor pagination of get_all_users in /admin/users route May 28, 2024
@ajanitshimanga ajanitshimanga marked this pull request as draft May 28, 2024 00:39
@sarahwooders
Copy link
Collaborator

@ajanitshimanga is this ready for review? Just wanted to check since its marked as a draft.

@sarahwooders
Copy link
Collaborator

Could you please rebase on top of main? We added some fixes to the tests.

@ajanitshimanga
Copy link
Contributor Author

ajanitshimanga commented Jun 5, 2024

I wanted to add tests for this but was running into some issues getting the full test suite passing locally on a clean top of main. Should new tests be added here or would the existing tests cover the cases here? Do you want me to still rebase and put this up for review? @sarahwooders

@ajanitshimanga ajanitshimanga marked this pull request as ready for review June 5, 2024 20:59
@ajanitshimanga ajanitshimanga marked this pull request as draft June 5, 2024 21:02
@sarahwooders
Copy link
Collaborator

If you could add a test in test_client.py (our main set of integration tests) that would be great! For our workflow, don't worry about the docker integration and OpenAI tests since they rely on an OpenAI key, but the pytest tests should pass.

@ajanitshimanga
Copy link
Contributor Author

ajanitshimanga commented Jun 7, 2024

Sounds good. I added a pagination test to test_admin_client.py. Opened a new PR since I think I messed up the commit history on this branch. cherry-picked the feature commit with a clean branch off main (found here: #1441)

Currently running into some local testing issues when running the Postgres's server via the docker bash file. Not certain how to proceed, guidance would be appreciated as this is my only blocker. I can provide some errors logs upon request. Followed contribution setup steps, memgpt.io docs, and any setup test errors that pop up when running pytest.

This is the new PR link: #1441

And I will subsequently close this once acknowledged.

@cpacker
Copy link
Collaborator

cpacker commented Jun 8, 2024

Closing in place of #1441

@cpacker cpacker closed this Jun 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants