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.

Sign up for GitHub

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

Jump to bottom

Get all pages #171

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Get all pages #171

wants to merge 3 commits into from

Conversation

TomOnTime
Copy link

Fixes #170

This is a work in progress.

This codes enables ls repos and ls pkgs to get all pages.

I wanted to add a flag --all-pages but I couldn't figure out how. I made a couple attempts to add the appropriate click statements but I couldn't get the value to propagate to where it was needed. I could use some help!

In the meanwhile I added a (brutal but temporarily) way to activate this code. The (undocumented) --page -1 enables this feature.

$ python3 -m cloudsmith_cli ls pkgs --page-size 3 --page -1  stackoverflow/raw
Getting list of packages ... OK
...
...
[REDACTED]                  | 2023.1.17          | Completed | [REDACTED]/raw/[REDACTED]               

Results: 62 packages visible

Could you help me add a proper flag? Ideally this should be the default, with the --page x enabling the old behavior for backwards compatibility.

Tom Limoncelli and others added 2 commits September 20, 2024 09:05
@nickxn
Copy link

nickxn commented Sep 20, 2024

Hi Tom! Good call out - let us take a look

@tlimoncelli
Copy link

After thinking about it a bit, let me suggest this instead:

The default should be to get all pages. I can't imagine a situation where someone would intentionally want partial information. In that case, my suggestion is to simply have ls pkgs and ls repos ignore the --page and --page-size flags and always return the complete information.

However a case can be made for backwards compatibility. There may be scripts that issue --page 1, --page 2, etc. and would break. On one hand, we can simply let those scripts break as the authors would probably be very happy to not have to assemble the individual pages any more. On the other hand, you could offer backwards compatibility if the --page flag is used. That is, "all pages" would be the default but any use of the --page flag would trigger backwards compatibility.

Neither would require any new flags.

I'd be glad to implement the first suggestion. I'd need assistance implementing the second.

@BartoszBlizniak
Copy link
Collaborator

BartoszBlizniak commented Sep 25, 2024 •
edited
Loading

Hey!

@TomOnTime - Thank you for the PR!
@tlimoncelli - Thank you for feedback!

Given that we don't want to impact customers who rely on the current logic, I would ideally like to avoid changing the default behaviours.

My suggestion is to have a new flag introduced called --show-all which will allow the users to show all content when they wish. Changing the default behaviour to show all content might have a significant performance impact due to the amount of time it takes to display that data which will be especially true for huge repositories.

Example for all packages:

> python3 -m cloudsmith_cli ls packages bart-demo-org/dockerland --show-all
Retrieving all packages for repository bart-demo-org/dockerland. This may take a while...

....

Results: 90 of 90 packages visible (all pages)

Example for all repos:

> python3 -m cloudsmith_cli ls repos bart-demo-org --show-all
Retrieving all repositories for bart-demo-org. This may take a while...

...
Results: 80 of 80 repositories visible (all pages)

@tlimoncelli
Copy link

Hi @BartoszBlizniak

Sounds good.

I made a few attempts at adding a flag with no success. Can you assist? I tried adding the flag to common_cli_list_options in cloudsmith_cli/cli/decorators.py but I can't see how to get the value to propagate to cloudsmith_cli/cli/commands/list_.py

@BartoszBlizniak
Copy link
Collaborator

Hi @BartoszBlizniak

Sounds good.

I made a few attempts at adding a flag with no success. Can you assist? I tried adding the flag to common_cli_list_options in cloudsmith_cli/cli/decorators.py but I can't see how to get the value to propagate to cloudsmith_cli/cli/commands/list_.py

No problem, I will unfortunately have to create a branch myself due to limitations for tests running from Fork. Once I have everything ready I will reference and credit the contribution!

@BartoszBlizniak BartoszBlizniak mentioned this pull request Sep 25, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add flag to get all pages
4 participants
@TomOnTime @nickxn @tlimoncelli @BartoszBlizniak