Skip to content

List all secrets instead of being subject to the default limit of 30 results#3679

Merged
mislav merged 2 commits intocli:trunkfrom
g14a:feature/list-secrets-limit
May 31, 2021
Merged

List all secrets instead of being subject to the default limit of 30 results#3679
mislav merged 2 commits intocli:trunkfrom
g14a:feature/list-secrets-limit

Conversation

@g14a
Copy link
Contributor

@g14a g14a commented May 20, 2021

This commit adds an option to set a limit to the listing of secrets via the --limit | -l flag.

Screenshot for reference.

image

Fixes #3670

@billygriffin
Copy link
Contributor

Just wanted to make sure you saw feedback from @mislav in the issue, so cross-posting it here:

Agreed: that we should not cap the secrets listed to 30 (the default API page limit)
I doubt, however, that we need the --limit option. Unlike the list of issues in a repository or the list or repositories under an organization, secrets is not a set of data that tends to grow indefinitely.

How about always listing all the secrets? I.e. fetching per_page=100 by default and paginating until all results are fetched. I'd be fine with that approach, and I don't think that even the largest repositories/organizations would ever have too many pages so that they would want a limit option.

@vilmibm
Copy link
Contributor

vilmibm commented May 21, 2021

ah thanks @billygriffin , I missed that. I also agree that --limit is superfluous here.

@g14a let's go that route -- you can drop the --limit flag and just use pagination to fetch all of the secrets each time.

@g14a
Copy link
Contributor Author

g14a commented May 22, 2021

@g14a let's go that route -- you can drop the --limit flag and just use pagination to fetch all of the secrets each time.

On it.

@g14a g14a force-pushed the feature/list-secrets-limit branch from d256f48 to 9e12c29 Compare May 22, 2021 08:28
@g14a
Copy link
Contributor Author

g14a commented May 22, 2021

@vilmibm done. Should another test be written for infinite pagination though? I thought that wasn't needed but I'd like to hear what you have to say about that. Hope I didn't make things worse.

@g14a
Copy link
Contributor Author

g14a commented May 30, 2021

I don't mean to hurry you but umm..any thoughts? @vilmibm

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Thanks @g14a! I've pushed additional code to change the pagination algorithm to respect the Link header and to add a test.

@mislav mislav changed the title Add limit option to list secrets List all secrets instead of being subject to the default limit of 30 results May 31, 2021
@mislav mislav enabled auto-merge May 31, 2021 18:41
@mislav mislav force-pushed the feature/list-secrets-limit branch from cf01cf5 to 260f720 Compare May 31, 2021 18:41
@mislav mislav merged commit 157bab1 into cli:trunk May 31, 2021
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.

Add -L, --limit to gh secret list

4 participants