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

Fix Fog::Compute::Google::Servers#all paging #608

Merged
merged 1 commit into from
Oct 29, 2023

Conversation

agrare
Copy link
Contributor

@agrare agrare commented Sep 27, 2023

If there are more instances than will fit in a page a next_page_token is returned. This has to be passed in to subsequent calls in order to retrieve the full list of servers.

Note: I think this method could use some refactoring but I wanted to model this after the way Snapshots#all was fixed for consistency and change as little as possible for a targeted bug fix.

Fixes #607

If there are more instances than will fit in a page a next_page_token is
returned.  This has to be passed in to subsequent calls in order to
retrieve the full list of servers.
@agrare agrare force-pushed the fix_compute_servers_all_paging branch from 247b03f to bd4fa07 Compare September 27, 2023 17:19
@agrare
Copy link
Contributor Author

agrare commented Sep 27, 2023

I'm curious on the maintainers' points of view on this but options to this method include :max_results and conceptually it isn't clear if this is intended to mean "never return more than this number of results" or specifically meant to be passed directly to the list_servers and list_aggregated_servers methods.

These options were added here 9891535#diff-bb76744c2fc47cf4c93fd72cfe6563ceb8d1421a342dd36a583fac4e49178142R7-R8 in what looks like simply adding explicit kwargs for each of the options that list_servers and list_aggregated_servers take.

I could see a user wanting to limit both the total results and the size of each page so maybe an additional option is needed?

@agrare
Copy link
Contributor Author

agrare commented Sep 27, 2023

cc @Temikus

Copy link
Member

@Temikus Temikus left a comment

Choose a reason for hiding this comment

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

LGTM

Regarding semantics - that particular commit was during a massive quick-and-dirty rewrite when google-cloud lib broke compatibility, so I'm in favor of a refactor to whatever is more user-friendly.

@Temikus Temikus merged commit a78bcf8 into fog:master Oct 29, 2023
@agrare agrare deleted the fix_compute_servers_all_paging branch October 30, 2023 15:02
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.

Unable to list more than 500 instances with Fog::Compute::Google::Servers#all
2 participants