Skip to content

feat: add pagination support #278

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

Merged
merged 2 commits into from
Jan 11, 2021
Merged

feat: add pagination support #278

merged 2 commits into from
Jan 11, 2021

Conversation

JGAntunes
Copy link
Contributor

Based on our docs and a couple of expriments I did (and on bitballoon's codebase) we support pagination for all of our list endpoints but it's missing from our open-api spec.

This PR updates our open-api spec and respective golang client. It also addresses #277 and serves as a base to address netlify/js-client#39, netlify/js-client#44 and netlify/js-client#167.

Let me know what you think 👍

@JGAntunes JGAntunes requested review from vbrown608 and ehmicky January 7, 2021 17:57
@JGAntunes JGAntunes requested review from a team as code owners January 7, 2021 17:57
@netlify
Copy link

netlify bot commented Jan 7, 2021

✔️ Deploy preview for open-api ready!

🔨 Explore the source changes: 9f301e0

🔍 Inspect the deploy logs: https://app.netlify.com/sites/open-api/deploys/5ff843c8a684830007e9091e

😎 Browse the preview: https://deploy-preview-278--open-api.netlify.app

@JGAntunes JGAntunes added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Jan 7, 2021
Copy link
Contributor

@ehmicky ehmicky left a comment

Choose a reason for hiding this comment

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

Awesome! 🎉 🎉 🎉

This looks good to me, but I don't have a good understanding of the API pagination, unlike @vbrown608.

rstavchansky
rstavchansky previously approved these changes Jan 7, 2021
Copy link

@rstavchansky rstavchansky left a comment

Choose a reason for hiding this comment

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

Output looks good for docs! 🥇

@vbrown608
Copy link
Contributor

vbrown608 commented Jan 7, 2021

Only those controllers that handle pagination_options and set pagination_headers support pagination. listAccountTypesForUser (AccountsController#types) does not, for example.

We're in the process of migrating our API docs to be built from inline annotations in the API codebase, so it might be better to add pagination to swagger as needed here, or just to endpoints where it seems most valuable to users of the SDK.

@vbrown608
Copy link
Contributor

I suggest we start with following actions, which all support pagination, and are likely to include 100+ records for some users.
listSites
listSiteSubmissions
listSiteDeploys
listSiteBuilds
listFormSubmissions
listAccountAuditEvents

Thanks for working on this!

@JGAntunes
Copy link
Contributor Author

JGAntunes commented Jan 8, 2021

Thanks for the review 👍 and thanks for checking this @vbrown608! I'll add this to those specific endpoints in that case. However, should we keep listSitesForAccount also? It's an endpoint that supports pagination and seems to be likely to have 100+ records.

@JGAntunes
Copy link
Contributor Author

JGAntunes commented Jan 8, 2021

Meanwhile I've updated the spec but kept listSitesForAccount, let me know if it makes sense to remove that also. 👍

Copy link
Contributor

@vbrown608 vbrown608 left a comment

Choose a reason for hiding this comment

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

Nice catch! Yeah, let's definitely keep that one too. This looks good to me.

@JGAntunes JGAntunes merged commit 706edd6 into master Jan 11, 2021
@JGAntunes JGAntunes deleted the feat/add-pagination branch January 11, 2021 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants