Support standard output format flags for secret and deploy-key list #8081

rajhawaldar · 2023-09-25T16:07:55Z

Signed-off-by: rajhawaldar <rajhawaldar.in@gmail.com>

andyfeller

@rajhawaldar : thank you for continuing to contribute to GitHub CLI! 🙇

How would you implement tests similar to export_pr_test.go for these changes?

rajhawaldar · 2023-09-27T16:50:38Z

@rajhawaldar : thank you for continuing to contribute to GitHub CLI! 🙇

How would you implement tests similar to export_pr_test.go for these changes?

Hello there, @andyfeller. Before writing the code for this issue, I explored the --json flag in the 'gh pr list' and 'gh workflow list' commands. I believe the AddJSONFlag capability has its own test cases, and so these commands do not have test cases written for it. Am I missing something here?

andyfeller · 2023-09-28T16:06:19Z

@rajhawaldar : thank you for continuing to contribute to GitHub CLI! 🙇
How would you implement tests similar to export_pr_test.go for these changes?

Hello there, @andyfeller. Before writing the code for this issue, I explored the --json flag in the 'gh pr list' and 'gh workflow list' commands. I believe the AddJSONFlag capability has its own test cases, and so these commands do not have test cases written for it. Am I missing something here?

Good question, @rajhawaldar. 🤔

You are correct: pkg/cmdutil/json_flags.go utilities have a generic suite of tests in pkg/cmdutil/json_flags_test.go to ensure they work a particular way in general. That said, it doesn't ensure how these features are instrumented work as expected.

Beyond these tests, several other commands have their own tests that ensure exported data works as expected:

So I ask myself the question when should a specific command feature write feature-specific tests that ensure the data is exported as expected? 🤔

andyfeller · 2023-09-28T18:37:33Z

pkg/cmd/secret/list/list.go

+var secretFields = []string{
+	"selected_repos_url",
+	"name",
+	"visibility",
+	"updated_at",
+	"num_selected_repos",
+}
+


The fields available differs on the type of secret:

organization

repository

repository environment

$ gh api /repos/random-org/services/actions/secrets { "total_count": 2, "secrets": [ { "name": "APP_ID", "created_at": "2022-05-02T23:26:42Z", "updated_at": "2022-05-02T23:36:52Z" }, { "name": "APP_PEM", "created_at": "2022-05-02T23:27:06Z", "updated_at": "2022-05-02T23:27:06Z" } ] } $ gh api /repos/random-org/services/environments/production/secrets { "total_count": 1, "secrets": [ { "name": "ORG_ADMIN_TOKEN", "created_at": "2022-01-26T04:20:30Z", "updated_at": "2022-01-26T04:20:30Z" } ] } $ gh api /orgs/random-org/actions/secrets { "total_count": 1, "secrets": [ { "name": "EXTERNAL_REPOSITORY_TOKEN", "created_at": "2021-12-03T20:18:46Z", "updated_at": "2021-12-03T22:57:21Z", "visibility": "selected", "selected_repositories_url": "https://api.github.com/orgs/random-org/actions/secrets/EXTERNAL_REPOSITORY_TOKEN/repositories" } ] }

Wanting to review the underlying logic that this handles non-existent fields gracefully

I had a same query at the time of updating the code. I explored getOrgSecrets, getUserSecrets, getEnvSecrets, getRepoSecrets functions to find the answer. These functions call getSecrets which populates the result in below object for all types of secrets,

type Secret struct { Name string UpdatedAt time.Time `json:"updated_at"` Visibility shared.Visibility SelectedReposURL string `json:"selected_repositories_url"` NumSelectedRepos int }

It keeps the value blank for the particular field if it does not exist for the specific secret.

@samcoe @williammartin : I worry having these additional empty fields on the different types of secrets will cause confusion. I generally lean on the side of only returning actually relevant data that matches the GitHub API endpoints.

Here's what this looks like from my local testing of these changes:

$ ./main secret list --json 'name,numSelectedRepos,selectedReposURL,updatedAt,visibility' --jq '.' --repo some-org/some-repo [ { "name": "APP_ID", "num_selected_repos": 0, "selected_repositories_url": "", "updated_at": "2022-05-02T23:36:52Z", "visibility": "" }, { "name": "APP_PEM", "num_selected_repos": 0, "selected_repositories_url": "", "updated_at": "2022-05-02T23:27:06Z", "visibility": "" } ] $ ./main secret list --json 'name,numSelectedRepos,selectedReposURL,updatedAt,visibility' --jq '.' --repo some-org/some-repo --env some-org [ { "name": "ORG_ADMIN_TOKEN", "num_selected_repos": 0, "selected_repositories_url": "", "updated_at": "2022-01-26T04:20:30Z", "visibility": "" } ] $ ./main secret list --json 'name,numSelectedRepos,selectedReposURL,updatedAt,visibility' --jq '.' --org some-org [ { "name": "EXTERNAL_REPOSITORY_TOKEN", "num_selected_repos": 0, "selected_repositories_url": "https://api.github.com/orgs/some-org/actions/secrets/EXTERNAL_REPOSITORY_TOKEN/repositories", "updated_at": "2021-12-03T22:57:21Z", "visibility": "selected" } ]

@andyfeller Agreed. Since it is only possible to list a single type of secret at a time we should remove the fields that do not apply to that type of secret. My only question is should we validate the input of the --json flag against the type of secret or just silently return only valid fields in the output?

I would normally say we have 3 different sets of fields for the 3 different types of secrets, which would ensure only valid fields show up and --jq would behave the right way. However, I think this situation is more complicated as we would need to process the flag values to determine which set of fields to use before invoking cmdutil.AddJSONFlags(cmd, &opts.Exporter, secretFields).

Rather than providing a static list of fields to cmdutil.AddJSONFlags(), the only idea is providing a function that will run at the time and return the set of fields. In this case, we could evaluate the flag values parsed before any pre/post run hooks.

That is a good point about how complicated the implementation would be. I think that for now let's just silently ignore fields that do not apply to the specified type of secret. That will get this PR into a shape that I think we can ship and we can iterate on the validation later if we find the time/reason to do so.

That's fair. For now, the feature will work as-is with irrelevant fields for different kinds of secrets that won't break anything. It will give us time to see how it plays out and can do the heavy lifting when called out.

I agree with everything written above. FWIW if I had nothing better to do I would probably break that Secret struct apart. I wish Go had proper sum type support but alas. Make illegal types unrepresentable and all that.

pkg/cmd/repo/deploy-key/list/list.go

Signed-off-by: rajhawaldar <rajhawaldar.in@gmail.com>

andyfeller

@rajhawaldar : thank you again for the contribution! ❤️

As discussed in various conversation threads, I think there is follow up work but don't want to delay this further.

Support standard output format flags for secret and deploy-key list

965eabd

Signed-off-by: rajhawaldar <rajhawaldar.in@gmail.com>

rajhawaldar requested a review from a team as a code owner September 25, 2023 16:07

rajhawaldar requested review from andyfeller and removed request for a team September 25, 2023 16:07

cliAutomation added the external pull request originating outside of the CLI core team label Sep 25, 2023

andyfeller requested changes Sep 27, 2023

View reviewed changes

andyfeller reviewed Sep 28, 2023

View reviewed changes

samcoe reviewed Sep 29, 2023

View reviewed changes

pkg/cmd/repo/deploy-key/list/list.go Outdated Show resolved Hide resolved

Use camelCase for fields

79296c0

Signed-off-by: rajhawaldar <rajhawaldar.in@gmail.com>

rajhawaldar requested a review from samcoe September 29, 2023 07:42

Merge branch 'trunk' into SupportStandardOutputFormatFlags

976b84a

andyfeller approved these changes Oct 2, 2023

View reviewed changes

andyfeller merged commit bd072c5 into cli:trunk Oct 2, 2023

andyfeller mentioned this pull request Jan 5, 2024

Add option --json for gh variable list #8516

Merged

williammartin mentioned this pull request Jun 24, 2024

Add --json output option to gh secret list #9231

Closed

Support standard output format flags for secret and deploy-key list #8081

Support standard output format flags for secret and deploy-key list #8081

Uh oh!

Conversation

rajhawaldar commented Sep 25, 2023

Uh oh!

andyfeller left a comment

Choose a reason for hiding this comment

Uh oh!

rajhawaldar commented Sep 27, 2023

Uh oh!

andyfeller commented Sep 28, 2023

Uh oh!

andyfeller Sep 28, 2023

Choose a reason for hiding this comment

Uh oh!

rajhawaldar Sep 29, 2023

Choose a reason for hiding this comment

Uh oh!

andyfeller Sep 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samcoe Oct 2, 2023

Choose a reason for hiding this comment

Uh oh!

andyfeller Oct 2, 2023

Choose a reason for hiding this comment

Uh oh!

samcoe Oct 2, 2023

Choose a reason for hiding this comment

Uh oh!

andyfeller Oct 2, 2023

Choose a reason for hiding this comment

Uh oh!

williammartin Oct 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andyfeller left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

andyfeller Sep 29, 2023 •

edited

Loading