diff --git a/pkg/cmd/secret/list/list.go b/pkg/cmd/secret/list/list.go index 57ca0e37d..0d2e6f3f1 100644 --- a/pkg/cmd/secret/list/list.go +++ b/pkg/cmd/secret/list/list.go @@ -3,6 +3,7 @@ package list import ( "fmt" "net/http" + "slices" "strings" "time" @@ -39,6 +40,8 @@ var secretFields = []string{ "numSelectedRepos", } +const fieldNumSelectedRepos = "numSelectedRepos" + func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Command { opts := &ListOptions{ IO: f.IOStreams, @@ -114,9 +117,21 @@ func listRun(opts *ListOptions) error { return fmt.Errorf("%s secrets are not supported for %s", secretEntity, secretApp) } - var secrets []Secret + // Since populating the `NumSelectedRepos` field costs further API requests + // (one per secret), it's important to avoid extra calls when the output will + // not present the field's value. So, we should only populate this field in + // these cases: + // 1. The command is run in the TTY mode without the `--json ` option. + // 2. The command is run with `--json ` option, and `numSelectedRepos` + // is among the selected fields. In this case, TTY mode is irrelevant. showSelectedRepoInfo := opts.IO.IsStdoutTTY() + if opts.Exporter != nil { + // Note that if there's an exporter set, then we don't mind the TTY mode + // because we just have to populate the requested fields. + showSelectedRepoInfo = slices.Contains(opts.Exporter.Fields(), fieldNumSelectedRepos) + } + var secrets []Secret switch secretEntity { case shared.Repository: secrets, err = getRepoSecrets(client, baseRepo, secretApp) diff --git a/pkg/cmd/secret/list/list_test.go b/pkg/cmd/secret/list/list_test.go index 5b1c161c2..f52e153f0 100644 --- a/pkg/cmd/secret/list/list_test.go +++ b/pkg/cmd/secret/list/list_test.go @@ -104,6 +104,7 @@ func Test_listRun(t *testing.T) { tests := []struct { name string tty bool + json bool opts *ListOptions wantOut []string }{ @@ -153,6 +154,30 @@ func Test_listRun(t *testing.T) { "SECRET_THREE\t1975-11-30T00:00:00Z\tSELECTED", }, }, + { + name: "org tty, json", + tty: true, + json: true, + opts: &ListOptions{ + OrgName: "UmbrellaCorporation", + }, + wantOut: []string{ + // Note the `"numSelectedRepos":2` pair in the last entry. + `[{"name":"SECRET_ONE","numSelectedRepos":0,"selectedReposURL":"","updatedAt":"1988-10-11T00:00:00Z","visibility":"all"},{"name":"SECRET_TWO","numSelectedRepos":0,"selectedReposURL":"","updatedAt":"2020-12-04T00:00:00Z","visibility":"private"},{"name":"SECRET_THREE","numSelectedRepos":2,"selectedReposURL":"https://api.github.com/orgs/UmbrellaCorporation/actions/secrets/SECRET_THREE/repositories","updatedAt":"1975-11-30T00:00:00Z","visibility":"selected"}]`, + }, + }, + { + name: "org not tty, json", + tty: false, + json: true, + opts: &ListOptions{ + OrgName: "UmbrellaCorporation", + }, + wantOut: []string{ + // Note the `"numSelectedRepos":2` pair in the last entry. + `[{"name":"SECRET_ONE","numSelectedRepos":0,"selectedReposURL":"","updatedAt":"1988-10-11T00:00:00Z","visibility":"all"},{"name":"SECRET_TWO","numSelectedRepos":0,"selectedReposURL":"","updatedAt":"2020-12-04T00:00:00Z","visibility":"private"},{"name":"SECRET_THREE","numSelectedRepos":2,"selectedReposURL":"https://api.github.com/orgs/UmbrellaCorporation/actions/secrets/SECRET_THREE/repositories","updatedAt":"1975-11-30T00:00:00Z","visibility":"selected"}]`, + }, + }, { name: "env tty", tty: true, @@ -203,6 +228,30 @@ func Test_listRun(t *testing.T) { "SECRET_THREE\t1975-11-30T00:00:00Z\tSELECTED", }, }, + { + name: "user tty, json", + tty: true, + json: true, + opts: &ListOptions{ + UserSecrets: true, + }, + wantOut: []string{ + // Note that `numSelectedRepos` fields are not set to default (zero). + `[{"name":"SECRET_ONE","numSelectedRepos":1,"selectedReposURL":"https://api.github.com/user/codespaces/secrets/SECRET_ONE/repositories","updatedAt":"1988-10-11T00:00:00Z","visibility":"selected"},{"name":"SECRET_TWO","numSelectedRepos":2,"selectedReposURL":"https://api.github.com/user/codespaces/secrets/SECRET_TWO/repositories","updatedAt":"2020-12-04T00:00:00Z","visibility":"selected"},{"name":"SECRET_THREE","numSelectedRepos":3,"selectedReposURL":"https://api.github.com/user/codespaces/secrets/SECRET_THREE/repositories","updatedAt":"1975-11-30T00:00:00Z","visibility":"selected"}]`, + }, + }, + { + name: "user not tty, json", + tty: false, + json: true, + opts: &ListOptions{ + UserSecrets: true, + }, + wantOut: []string{ + // Note that `numSelectedRepos` fields are not set to default (zero). + `[{"name":"SECRET_ONE","numSelectedRepos":1,"selectedReposURL":"https://api.github.com/user/codespaces/secrets/SECRET_ONE/repositories","updatedAt":"1988-10-11T00:00:00Z","visibility":"selected"},{"name":"SECRET_TWO","numSelectedRepos":2,"selectedReposURL":"https://api.github.com/user/codespaces/secrets/SECRET_TWO/repositories","updatedAt":"2020-12-04T00:00:00Z","visibility":"selected"},{"name":"SECRET_THREE","numSelectedRepos":3,"selectedReposURL":"https://api.github.com/user/codespaces/secrets/SECRET_THREE/repositories","updatedAt":"1975-11-30T00:00:00Z","visibility":"selected"}]`, + }, + }, { name: "Dependabot repo tty", tty: true, @@ -310,13 +359,11 @@ func Test_listRun(t *testing.T) { }, } - if tt.tty { - reg.Register( - httpmock.REST("GET", fmt.Sprintf("orgs/%s/actions/secrets/SECRET_THREE/repositories", tt.opts.OrgName)), - httpmock.JSONResponse(struct { - TotalCount int `json:"total_count"` - }{2})) - } + reg.Register( + httpmock.REST("GET", fmt.Sprintf("orgs/%s/actions/secrets/SECRET_THREE/repositories", tt.opts.OrgName)), + httpmock.JSONResponse(struct { + TotalCount int `json:"total_count"` + }{2})) } if tt.opts.UserSecrets { @@ -342,17 +389,15 @@ func Test_listRun(t *testing.T) { } path = "user/codespaces/secrets" - if tt.tty { - for i, secret := range payload.Secrets { - hostLen := len("https://api.github.com/") - path := secret.SelectedReposURL[hostLen:len(secret.SelectedReposURL)] - repositoryCount := i + 1 - reg.Register( - httpmock.REST("GET", path), - httpmock.JSONResponse(struct { - TotalCount int `json:"total_count"` - }{repositoryCount})) - } + for i, secret := range payload.Secrets { + hostLen := len("https://api.github.com/") + path := secret.SelectedReposURL[hostLen:len(secret.SelectedReposURL)] + repositoryCount := i + 1 + reg.Register( + httpmock.REST("GET", path), + httpmock.JSONResponse(struct { + TotalCount int `json:"total_count"` + }{repositoryCount})) } } @@ -381,6 +426,12 @@ func Test_listRun(t *testing.T) { return t } + if tt.json { + exporter := cmdutil.NewJSONExporter() + exporter.SetFields(secretFields) + tt.opts.Exporter = exporter + } + err := listRun(tt.opts) assert.NoError(t, err) @@ -390,6 +441,199 @@ func Test_listRun(t *testing.T) { } } +// Test_listRun_populatesNumSelectedReposIfRequired asserts that NumSelectedRepos +// field is populated **only** when it's going to be presented in the output. Since +// populating this field costs further API requests (one per secret), it's important +// to avoid extra calls when the output will not present the field's value. Note +// that NumSelectedRepos is only meant for user or organization secrets. +// +// We should only populate the NumSelectedRepos field in these cases: +// 1. The command is run in the TTY mode without the `--json ` option. +// 2. The command is run with `--json ` option, and `numSelectedRepos` +// is among the selected fields. In this case, TTY mode is irrelevant. +func Test_listRun_populatesNumSelectedReposIfRequired(t *testing.T) { + type secretKind string + const secretKindUser secretKind = "user" + const secretKindOrg secretKind = "org" + + tests := []struct { + name string + kind secretKind + tty bool + jsonFields []string + wantPopulated bool + }{ + { + name: "org tty", + kind: secretKindOrg, + tty: true, + wantPopulated: true, + }, + { + name: "org tty, json with numSelectedRepos", + kind: secretKindOrg, + tty: true, + jsonFields: []string{"numSelectedRepos"}, + wantPopulated: true, + }, + { + name: "org tty, json without numSelectedRepos", + kind: secretKindOrg, + tty: true, + jsonFields: []string{"name"}, + wantPopulated: false, + }, + { + name: "org not tty", + kind: secretKindOrg, + tty: false, + wantPopulated: false, + }, + { + name: "org not tty, json with numSelectedRepos", + kind: secretKindOrg, + tty: false, + jsonFields: []string{"numSelectedRepos"}, + wantPopulated: true, + }, + { + name: "org not tty, json without numSelectedRepos", + kind: secretKindOrg, + tty: false, + jsonFields: []string{"name"}, + wantPopulated: false, + }, + { + name: "user tty", + kind: secretKindUser, + tty: true, + wantPopulated: true, + }, + { + name: "user tty, json with numSelectedRepos", + kind: secretKindUser, + tty: true, + jsonFields: []string{"numSelectedRepos"}, + wantPopulated: true, + }, + { + name: "user tty, json without numSelectedRepos", + kind: secretKindUser, + tty: true, + jsonFields: []string{"name"}, + wantPopulated: false, + }, + { + name: "user not tty", + kind: secretKindUser, + tty: false, + wantPopulated: false, + }, + { + name: "user not tty, json with numSelectedRepos", + kind: secretKindUser, + tty: false, + jsonFields: []string{"numSelectedRepos"}, + wantPopulated: true, + }, + { + name: "user not tty, json without numSelectedRepos", + kind: secretKindUser, + tty: false, + jsonFields: []string{"name"}, + wantPopulated: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + reg.Verify(t) + + t0, _ := time.Parse("2006-01-02", "1988-10-11") + opts := &ListOptions{} + + if tt.kind == secretKindOrg { + opts.OrgName = "umbrellaOrganization" + reg.Register( + httpmock.REST("GET", "orgs/umbrellaOrganization/actions/secrets"), + httpmock.JSONResponse(struct{ Secrets []Secret }{ + []Secret{ + { + Name: "SECRET", + UpdatedAt: t0, + Visibility: shared.Selected, + SelectedReposURL: "https://api.github.com/orgs/umbrellaOrganization/actions/secrets/SECRET/repositories", + }, + }, + })) + reg.Register( + httpmock.REST("GET", "orgs/umbrellaOrganization/actions/secrets/SECRET/repositories"), + httpmock.JSONResponse(struct { + TotalCount int `json:"total_count"` + }{999})) + } + + if tt.kind == secretKindUser { + opts.UserSecrets = true + reg.Register( + httpmock.REST("GET", "user/codespaces/secrets"), + httpmock.JSONResponse(struct{ Secrets []Secret }{ + []Secret{ + { + Name: "SECRET", + UpdatedAt: t0, + Visibility: shared.Selected, + SelectedReposURL: "https://api.github.com/user/codespaces/secrets/SECRET/repositories", + }, + }, + })) + reg.Register( + httpmock.REST("GET", "user/codespaces/secrets/SECRET/repositories"), + httpmock.JSONResponse(struct { + TotalCount int `json:"total_count"` + }{999})) + } + + if tt.jsonFields != nil { + exporter := cmdutil.NewJSONExporter() + exporter.SetFields(tt.jsonFields) + opts.Exporter = exporter + } + + ios, _, _, _ := iostreams.Test() + ios.SetStdoutTTY(tt.tty) + opts.IO = ios + + opts.BaseRepo = func() (ghrepo.Interface, error) { + return ghrepo.FromFullName("owner/repo") + } + opts.HttpClient = func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + } + opts.Config = func() (config.Config, error) { + return config.NewBlankConfig(), nil + } + opts.Now = func() time.Time { + t, _ := time.Parse(time.RFC822, "4 Apr 24 00:00 UTC") + return t + } + + err := listRun(opts) + assert.NoError(t, err) + + if tt.wantPopulated { + // There should be 2 requests; one to get the secrets list and + // another to populate the numSelectedRepos field. + assert.Len(t, reg.Requests, 2) + } else { + // Only one requests to get the secrets list. + assert.Len(t, reg.Requests, 1) + } + }) + } +} + func Test_getSecrets_pagination(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t)