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

Sort envs returned by REST API by current build's scheduled_on time #881

Merged

Conversation

peytondmurray
Copy link
Contributor

Fixes #859.

Description

This PR sorts environments returned when a GET is sent to /environment/ by the time the environments' current builds were submitted. In systems where users are creating environments while environments are being queried, this ensures that all results are returned; other sorting methods (e.g. by name) can yield incomplete results.

Pull request checklist

  • Did you test this change locally?
  • Did you update the documentation (if required)?
  • Did you add/update relevant tests for this change (if required)?

Copy link

netlify bot commented Sep 17, 2024

Deploy Preview for conda-store canceled.

Name Link
🔨 Latest commit 129793d
🔍 Latest deploy log https://app.netlify.com/sites/conda-store/deploys/679178c25c5b51000860ef46

@peytondmurray peytondmurray force-pushed the 859-sorted-pagination branch 2 times, most recently from 7a9b540 to 0d57b2b Compare September 26, 2024 21:51
@peytondmurray peytondmurray force-pushed the 859-sorted-pagination branch from 871e0e3 to 0442238 Compare October 3, 2024 21:10
@peytondmurray peytondmurray changed the title [WIP] Sort envs returned by REST API by current build's scheduled_on time Sort envs returned by REST API by current build's scheduled_on time Oct 3, 2024
@peytondmurray peytondmurray marked this pull request as ready for review October 17, 2024 03:11
@peytondmurray peytondmurray requested a review from soapy1 October 17, 2024 03:11
@peytondmurray peytondmurray marked this pull request as draft October 17, 2024 18:36
@peytondmurray
Copy link
Contributor Author

After a discussion, it seems like no matter how we solve this there's going to be a breaking change to the API. Cursor-based pagination will allow for any sorting order, so I'll look into implementing that, probably through fastapi-pagination.

@peytondmurray
Copy link
Contributor Author

peytondmurray commented Oct 18, 2024

Adding pagination via fastapi-pagination seems simple on the surface, but they don't publish an API reference in the docs, so without reading the source for the project I'm not sure how it affects what query parameters are accepted by our API. So in the process of trying to implement this, I started working on adding hot reloading, but due to the fact that we're using traitlets to load server configuration (and instantiate the FastAPI app) and that uvicorn has specific requirements about how the FastAPI app is instantiated when hot reloading, it's trickier to get this working than I thought. See #901 for details.

@peytondmurray peytondmurray force-pushed the 859-sorted-pagination branch 2 times, most recently from 9e55051 to 847eed4 Compare November 1, 2024 21:28
@peytondmurray
Copy link
Contributor Author

Hot reloading's done, but the fastapi_pagination docs aren't enough to get this working easily, and in the time it would take to actually read through the source for the project I think we can just implement a simple cursor-based paginator instead :/

@peytondmurray peytondmurray force-pushed the 859-sorted-pagination branch 2 times, most recently from 7b5a0bc to dd5a139 Compare November 6, 2024 21:59
@peytondmurray peytondmurray force-pushed the 859-sorted-pagination branch 4 times, most recently from b673196 to 54f7907 Compare January 3, 2025 23:53
@peytondmurray peytondmurray force-pushed the 859-sorted-pagination branch 2 times, most recently from 664d122 to 63db9d0 Compare January 16, 2025 21:56
@peytondmurray
Copy link
Contributor Author

Not sure if this is actually ready to be merged, as it is not currently compatible with the frontend, and a discussion about REST API versioning needs to be had before this approach can be merged. But otherwise, tests are passing and the implementation is I think mostly complete :) @soapy1

Copy link
Contributor

@soapy1 soapy1 left a comment

Choose a reason for hiding this comment

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

Looking good! Thank you for adding such beautiful formatted and thorough docs to a lot of these functions 🤩

Just a few things to sort out.

I took notes on what I tested manually in this gist https://gist.github.com/soapy1/beae935f78725cfd1dd0897c1ce016d4

@soapy1
Copy link
Contributor

soapy1 commented Jan 17, 2025

One more question (related to how we want to approach versioning for this change): how do we want to approach pagination for the rest of the api? Do we want all the other endpoints to also adopt this type of pagination?

@peytondmurray
Copy link
Contributor Author

I vote yes - as far as I can tell the advantages of limit/offset are that it's easy to implement, but the cost of it is exactly what is described in #859. Cursor based pagination addresses that.

@peytondmurray
Copy link
Contributor Author

The cursor-based pagination is now on /api/v2/environment/, and /api/v1/environment/ has been restored as well. Ready for a second look!

Copy link
Contributor

@soapy1 soapy1 left a comment

Choose a reason for hiding this comment

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

Looks great! Just 2 small changes suggested

Copy link
Contributor

@soapy1 soapy1 left a comment

Choose a reason for hiding this comment

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

Looks great 📃

@peytondmurray peytondmurray marked this pull request as ready for review January 23, 2025 20:11
@peytondmurray peytondmurray merged commit 5ce425d into conda-incubator:main Jan 23, 2025
30 checks passed
@peytondmurray peytondmurray deleted the 859-sorted-pagination branch January 23, 2025 20:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: api 🌐 area: user experience 👩🏻‍💻 Items impacting the end-user experience type: bug 🐛 Something isn't working
Projects
Status: Done 💪🏾
Development

Successfully merging this pull request may close these issues.

[ENH] - Ensure completeness when fetching all pages using REST API
3 participants