From d4e33858efe4b8b96dafc13fa2e806ec857886c0 Mon Sep 17 00:00:00 2001 From: William Martin Date: Tue, 25 Jun 2024 17:02:00 +0200 Subject: [PATCH] Fetch variable selected repo relationship when required --- pkg/cmd/variable/list/list.go | 17 ++++- pkg/cmd/variable/list/list_test.go | 117 +++++++++++++++++++++++++++++ 2 files changed, 133 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/variable/list/list.go b/pkg/cmd/variable/list/list.go index 2d26a03b6..764c0af4d 100644 --- a/pkg/cmd/variable/list/list.go +++ b/pkg/cmd/variable/list/list.go @@ -3,6 +3,7 @@ package list import ( "fmt" "net/http" + "slices" "strings" "time" @@ -30,6 +31,8 @@ type ListOptions struct { EnvName string } +const fieldNumSelectedRepos = "numSelectedRepos" + func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Command { opts := &ListOptions{ IO: f.IOStreams, @@ -94,9 +97,21 @@ func listRun(opts *ListOptions) error { return err } - var variables []shared.Variable + // 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 variables []shared.Variable switch variableEntity { case shared.Repository: variables, err = getRepoVariables(client, baseRepo) diff --git a/pkg/cmd/variable/list/list_test.go b/pkg/cmd/variable/list/list_test.go index ed4d59c59..4933d3957 100644 --- a/pkg/cmd/variable/list/list_test.go +++ b/pkg/cmd/variable/list/list_test.go @@ -304,6 +304,123 @@ 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 variable), it's important +// to avoid extra calls when the output will not present the field's value. Note +// that NumSelectedRepos is only meant for organization variables. +// +// 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) { + tests := []struct { + name string + tty bool + jsonFields []string + wantPopulated bool + }{ + { + name: "org tty", + tty: true, + wantPopulated: true, + }, + { + name: "org tty, json with numSelectedRepos", + tty: true, + jsonFields: []string{"numSelectedRepos"}, + wantPopulated: true, + }, + { + name: "org tty, json without numSelectedRepos", + tty: true, + jsonFields: []string{"name"}, + wantPopulated: false, + }, + { + name: "org not tty", + tty: false, + wantPopulated: false, + }, + { + name: "org not tty, json with numSelectedRepos", + tty: false, + jsonFields: []string{"numSelectedRepos"}, + wantPopulated: true, + }, + { + name: "org not tty, json without numSelectedRepos", + 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) + + reg.Register( + httpmock.REST("GET", "orgs/umbrellaOrganization/actions/variables"), + httpmock.JSONResponse(struct{ Variables []shared.Variable }{ + []shared.Variable{ + { + Name: "VARIABLE", + Visibility: shared.Selected, + SelectedReposURL: "https://api.github.com/orgs/umbrellaOrganization/actions/variables/VARIABLE/repositories", + }, + }, + })) + reg.Register( + httpmock.REST("GET", "orgs/umbrellaOrganization/actions/variables/VARIABLE/repositories"), + httpmock.JSONResponse(struct { + TotalCount int `json:"total_count"` + }{999})) + + opts := &ListOptions{ + OrgName: "umbrellaOrganization", + } + + 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() (gh.Config, error) { + return config.NewBlankConfig(), nil + } + opts.Now = func() time.Time { + t, _ := time.Parse(time.RFC822, "4 Apr 24 00:00 UTC") + return t + } + + require.NoError(t, listRun(opts)) + + if tt.wantPopulated { + // There should be 2 requests; one to get the variables list and + // another to populate the numSelectedRepos field. + assert.Len(t, reg.Requests, 2) + } else { + // Only one requests to get the variables list. + assert.Len(t, reg.Requests, 1) + } + }) + } +} + func Test_getVariables_pagination(t *testing.T) { reg := &httpmock.Registry{} defer reg.Verify(t)