From 41a457136e3134e5b5e6c17cba2704309ac609bb Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Wed, 15 Mar 2023 12:03:56 +1100 Subject: [PATCH] Tech debt cleanup for variable and secret commands (#7151) --- pkg/cmd/secret/delete/delete_test.go | 3 - pkg/cmd/secret/list/list.go | 135 +++++++------------- pkg/cmd/secret/list/list_test.go | 165 +++++++++++++------------ pkg/cmd/variable/delete/delete.go | 1 - pkg/cmd/variable/delete/delete_test.go | 98 +++------------ pkg/cmd/variable/list/list.go | 142 ++++++--------------- pkg/cmd/variable/list/list_test.go | 136 ++++++++++---------- pkg/cmd/variable/set/set.go | 4 +- pkg/cmd/variable/variable.go | 7 +- 9 files changed, 259 insertions(+), 432 deletions(-) diff --git a/pkg/cmd/secret/delete/delete_test.go b/pkg/cmd/secret/delete/delete_test.go index 3e4108703..8474e90c0 100644 --- a/pkg/cmd/secret/delete/delete_test.go +++ b/pkg/cmd/secret/delete/delete_test.go @@ -116,7 +116,6 @@ func TestNewCmdDelete(t *testing.T) { assert.Equal(t, tt.wants.EnvName, gotOpts.EnvName) }) } - } func Test_removeRun_repo(t *testing.T) { @@ -264,10 +263,8 @@ func Test_removeRun_org(t *testing.T) { assert.NoError(t, err) reg.Verify(t) - }) } - } func Test_removeRun_user(t *testing.T) { diff --git a/pkg/cmd/secret/list/list.go b/pkg/cmd/secret/list/list.go index faee8ae3e..965466b67 100644 --- a/pkg/cmd/secret/list/list.go +++ b/pkg/cmd/secret/list/list.go @@ -1,22 +1,19 @@ package list import ( - "encoding/json" "fmt" "net/http" - "regexp" "strings" "time" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/config" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/tableprinter" "github.com/cli/cli/v2/pkg/cmd/secret/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/iostreams" - "github.com/cli/cli/v2/utils" "github.com/spf13/cobra" ) @@ -25,6 +22,7 @@ type ListOptions struct { IO *iostreams.IOStreams Config func() (config.Config, error) BaseRepo func() (ghrepo.Interface, error) + Now func() time.Time OrgName string EnvName string @@ -37,6 +35,7 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman IO: f.IOStreams, Config: f.Config, HttpClient: f.HttpClient, + Now: time.Now, } cmd := &cobra.Command{ @@ -106,7 +105,7 @@ func listRun(opts *ListOptions) error { return fmt.Errorf("%s secrets are not supported for %s", secretEntity, secretApp) } - var secrets []*Secret + var secrets []Secret showSelectedRepoInfo := opts.IO.IsStdoutTTY() switch secretEntity { @@ -146,26 +145,26 @@ func listRun(opts *ListOptions) error { fmt.Fprintf(opts.IO.ErrOut, "failed to start pager: %v\n", err) } - //nolint:staticcheck // SA1019: utils.NewTablePrinter is deprecated: use internal/tableprinter - tp := utils.NewTablePrinter(opts.IO) + table := tableprinter.New(opts.IO) + if secretEntity == shared.Organization || secretEntity == shared.User { + table.HeaderRow("Name", "Updated", "Visibility") + } else { + table.HeaderRow("Name", "Updated") + } for _, secret := range secrets { - tp.AddField(secret.Name, nil, nil) - updatedAt := secret.UpdatedAt.Format("2006-01-02") - if opts.IO.IsStdoutTTY() { - updatedAt = fmt.Sprintf("Updated %s", updatedAt) - } - tp.AddField(updatedAt, nil, nil) + table.AddField(secret.Name) + table.AddTimeField(opts.Now(), secret.UpdatedAt, nil) if secret.Visibility != "" { if showSelectedRepoInfo { - tp.AddField(fmtVisibility(*secret), nil, nil) + table.AddField(fmtVisibility(secret)) } else { - tp.AddField(strings.ToUpper(string(secret.Visibility)), nil, nil) + table.AddField(strings.ToUpper(string(secret.Visibility))) } } - tp.EndRow() + table.EndRow() } - err = tp.Render() + err = table.Render() if err != nil { return err } @@ -197,14 +196,14 @@ func fmtVisibility(s Secret) string { return "" } -func getOrgSecrets(client httpClient, host, orgName string, showSelectedRepoInfo bool, app shared.App) ([]*Secret, error) { +func getOrgSecrets(client *http.Client, host, orgName string, showSelectedRepoInfo bool, app shared.App) ([]Secret, error) { secrets, err := getSecrets(client, host, fmt.Sprintf("orgs/%s/%s/secrets", orgName, app)) if err != nil { return nil, err } if showSelectedRepoInfo { - err = getSelectedRepositoryInformation(client, secrets) + err = populateSelectedRepositoryInformation(client, host, secrets) if err != nil { return nil, err } @@ -212,14 +211,14 @@ func getOrgSecrets(client httpClient, host, orgName string, showSelectedRepoInfo return secrets, nil } -func getUserSecrets(client httpClient, host string, showSelectedRepoInfo bool) ([]*Secret, error) { +func getUserSecrets(client *http.Client, host string, showSelectedRepoInfo bool) ([]Secret, error) { secrets, err := getSecrets(client, host, "user/codespaces/secrets") if err != nil { return nil, err } if showSelectedRepoInfo { - err = getSelectedRepositoryInformation(client, secrets) + err = populateSelectedRepositoryInformation(client, host, secrets) if err != nil { return nil, err } @@ -228,96 +227,46 @@ func getUserSecrets(client httpClient, host string, showSelectedRepoInfo bool) ( return secrets, nil } -func getEnvSecrets(client httpClient, repo ghrepo.Interface, envName string) ([]*Secret, error) { +func getEnvSecrets(client *http.Client, repo ghrepo.Interface, envName string) ([]Secret, error) { path := fmt.Sprintf("repos/%s/environments/%s/secrets", ghrepo.FullName(repo), envName) return getSecrets(client, repo.RepoHost(), path) } -func getRepoSecrets(client httpClient, repo ghrepo.Interface, app shared.App) ([]*Secret, error) { - return getSecrets(client, repo.RepoHost(), fmt.Sprintf("repos/%s/%s/secrets", - ghrepo.FullName(repo), app)) +func getRepoSecrets(client *http.Client, repo ghrepo.Interface, app shared.App) ([]Secret, error) { + return getSecrets(client, repo.RepoHost(), fmt.Sprintf("repos/%s/%s/secrets", ghrepo.FullName(repo), app)) } -type secretsPayload struct { - Secrets []*Secret -} - -type httpClient interface { - Do(*http.Request) (*http.Response, error) -} - -func getSecrets(client httpClient, host, path string) ([]*Secret, error) { - var results []*Secret - url := fmt.Sprintf("%s%s?per_page=100", ghinstance.RESTPrefix(host), path) - - for { - var payload secretsPayload - nextURL, err := apiGet(client, url, &payload) +func getSecrets(client *http.Client, host, path string) ([]Secret, error) { + var results []Secret + apiClient := api.NewClientFromHTTP(client) + path = fmt.Sprintf("%s?per_page=100", path) + for path != "" { + response := struct { + Secrets []Secret + }{} + var err error + path, err = apiClient.RESTWithNext(host, "GET", path, nil, &response) if err != nil { return nil, err } - results = append(results, payload.Secrets...) - - if nextURL == "" { - break - } - url = nextURL + results = append(results, response.Secrets...) } - return results, nil } -func apiGet(client httpClient, url string, data interface{}) (string, error) { - req, err := http.NewRequest("GET", url, nil) - if err != nil { - return "", err - } - req.Header.Set("Content-Type", "application/json; charset=utf-8") - - resp, err := client.Do(req) - if err != nil { - return "", err - } - defer resp.Body.Close() - - if resp.StatusCode > 299 { - return "", api.HandleHTTPError(resp) - } - - dec := json.NewDecoder(resp.Body) - if err := dec.Decode(data); err != nil { - return "", err - } - - return findNextPage(resp.Header.Get("Link")), nil -} - -var linkRE = regexp.MustCompile(`<([^>]+)>;\s*rel="([^"]+)"`) - -func findNextPage(link string) string { - for _, m := range linkRE.FindAllStringSubmatch(link, -1) { - if len(m) > 2 && m[2] == "next" { - return m[1] - } - } - return "" -} - -func getSelectedRepositoryInformation(client httpClient, secrets []*Secret) error { - type responseData struct { - TotalCount int `json:"total_count"` - } - - for _, secret := range secrets { +func populateSelectedRepositoryInformation(client *http.Client, host string, secrets []Secret) error { + apiClient := api.NewClientFromHTTP(client) + for i, secret := range secrets { if secret.SelectedReposURL == "" { continue } - var result responseData - if _, err := apiGet(client, secret.SelectedReposURL, &result); err != nil { + response := struct { + TotalCount int `json:"total_count"` + }{} + if err := apiClient.REST(host, "GET", secret.SelectedReposURL, nil, &response); err != nil { return fmt.Errorf("failed determining selected repositories for %s: %w", secret.Name, err) } - secret.NumSelectedRepos = result.TotalCount + secrets[i].NumSelectedRepos = response.TotalCount } - return nil } diff --git a/pkg/cmd/secret/list/list_test.go b/pkg/cmd/secret/list/list_test.go index 46f3c97d6..75aa321ee 100644 --- a/pkg/cmd/secret/list/list_test.go +++ b/pkg/cmd/secret/list/list_test.go @@ -3,8 +3,8 @@ package list import ( "bytes" "fmt" - "io" "net/http" + "net/url" "strings" "testing" "time" @@ -15,7 +15,6 @@ import ( "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" - "github.com/cli/cli/v2/test" "github.com/google/shlex" "github.com/stretchr/testify/assert" ) @@ -112,9 +111,10 @@ func Test_listRun(t *testing.T) { tty: true, opts: &ListOptions{}, wantOut: []string{ - "SECRET_ONE.*Updated 1988-10-11", - "SECRET_TWO.*Updated 2020-12-04", - "SECRET_THREE.*Updated 1975-11-30", + "NAME UPDATED", + "SECRET_ONE about 34 years ago", + "SECRET_TWO about 2 years ago", + "SECRET_THREE about 47 years ago", }, }, { @@ -122,9 +122,9 @@ func Test_listRun(t *testing.T) { tty: false, opts: &ListOptions{}, wantOut: []string{ - "SECRET_ONE\t1988-10-11", - "SECRET_TWO\t2020-12-04", - "SECRET_THREE\t1975-11-30", + "SECRET_ONE\t1988-10-11T00:00:00Z", + "SECRET_TWO\t2020-12-04T00:00:00Z", + "SECRET_THREE\t1975-11-30T00:00:00Z", }, }, { @@ -134,9 +134,10 @@ func Test_listRun(t *testing.T) { OrgName: "UmbrellaCorporation", }, wantOut: []string{ - "SECRET_ONE.*Updated 1988-10-11.*Visible to all repositories", - "SECRET_TWO.*Updated 2020-12-04.*Visible to private repositories", - "SECRET_THREE.*Updated 1975-11-30.*Visible to 2 selected repositories", + "NAME UPDATED VISIBILITY", + "SECRET_ONE about 34 years ago Visible to all repositories", + "SECRET_TWO about 2 years ago Visible to private repositories", + "SECRET_THREE about 47 years ago Visible to 2 selected repositories", }, }, { @@ -146,9 +147,9 @@ func Test_listRun(t *testing.T) { OrgName: "UmbrellaCorporation", }, wantOut: []string{ - "SECRET_ONE\t1988-10-11\tALL", - "SECRET_TWO\t2020-12-04\tPRIVATE", - "SECRET_THREE\t1975-11-30\tSELECTED", + "SECRET_ONE\t1988-10-11T00:00:00Z\tALL", + "SECRET_TWO\t2020-12-04T00:00:00Z\tPRIVATE", + "SECRET_THREE\t1975-11-30T00:00:00Z\tSELECTED", }, }, { @@ -158,9 +159,10 @@ func Test_listRun(t *testing.T) { EnvName: "Development", }, wantOut: []string{ - "SECRET_ONE.*Updated 1988-10-11", - "SECRET_TWO.*Updated 2020-12-04", - "SECRET_THREE.*Updated 1975-11-30", + "NAME UPDATED", + "SECRET_ONE about 34 years ago", + "SECRET_TWO about 2 years ago", + "SECRET_THREE about 47 years ago", }, }, { @@ -170,9 +172,9 @@ func Test_listRun(t *testing.T) { EnvName: "Development", }, wantOut: []string{ - "SECRET_ONE\t1988-10-11", - "SECRET_TWO\t2020-12-04", - "SECRET_THREE\t1975-11-30", + "SECRET_ONE\t1988-10-11T00:00:00Z", + "SECRET_TWO\t2020-12-04T00:00:00Z", + "SECRET_THREE\t1975-11-30T00:00:00Z", }, }, { @@ -182,9 +184,10 @@ func Test_listRun(t *testing.T) { UserSecrets: true, }, wantOut: []string{ - "SECRET_ONE.*Updated 1988-10-11.*Visible to 1 selected repository", - "SECRET_TWO.*Updated 2020-12-04.*Visible to 2 selected repositories", - "SECRET_THREE.*Updated 1975-11-30.*Visible to 3 selected repositories", + "NAME UPDATED VISIBILITY", + "SECRET_ONE about 34 years ago Visible to 1 selected repository", + "SECRET_TWO about 2 years ago Visible to 2 selected repositories", + "SECRET_THREE about 47 years ago Visible to 3 selected repositories", }, }, { @@ -194,9 +197,9 @@ func Test_listRun(t *testing.T) { UserSecrets: true, }, wantOut: []string{ - "SECRET_ONE\t1988-10-11\t", - "SECRET_TWO\t2020-12-04\t", - "SECRET_THREE\t1975-11-30\t", + "SECRET_ONE\t1988-10-11T00:00:00Z\tSELECTED", + "SECRET_TWO\t2020-12-04T00:00:00Z\tSELECTED", + "SECRET_THREE\t1975-11-30T00:00:00Z\tSELECTED", }, }, { @@ -206,9 +209,10 @@ func Test_listRun(t *testing.T) { Application: "Dependabot", }, wantOut: []string{ - "SECRET_ONE.*Updated 1988-10-11", - "SECRET_TWO.*Updated 2020-12-04", - "SECRET_THREE.*Updated 1975-11-30", + "NAME UPDATED", + "SECRET_ONE about 34 years ago", + "SECRET_TWO about 2 years ago", + "SECRET_THREE about 47 years ago", }, }, { @@ -218,9 +222,9 @@ func Test_listRun(t *testing.T) { Application: "Dependabot", }, wantOut: []string{ - "SECRET_ONE\t1988-10-11", - "SECRET_TWO\t2020-12-04", - "SECRET_THREE\t1975-11-30", + "SECRET_ONE\t1988-10-11T00:00:00Z", + "SECRET_TWO\t2020-12-04T00:00:00Z", + "SECRET_THREE\t1975-11-30T00:00:00Z", }, }, { @@ -231,9 +235,10 @@ func Test_listRun(t *testing.T) { OrgName: "UmbrellaCorporation", }, wantOut: []string{ - "SECRET_ONE.*Updated 1988-10-11.*Visible to all repositories", - "SECRET_TWO.*Updated 2020-12-04.*Visible to private repositories", - "SECRET_THREE.*Updated 1975-11-30.*Visible to 2 selected repositories", + "NAME UPDATED VISIBILITY", + "SECRET_ONE about 34 years ago Visible to all repositories", + "SECRET_TWO about 2 years ago Visible to private repositories", + "SECRET_THREE about 47 years ago Visible to 2 selected repositories", }, }, { @@ -244,9 +249,9 @@ func Test_listRun(t *testing.T) { OrgName: "UmbrellaCorporation", }, wantOut: []string{ - "SECRET_ONE\t1988-10-11\tALL", - "SECRET_TWO\t2020-12-04\tPRIVATE", - "SECRET_THREE\t1975-11-30\tSELECTED", + "SECRET_ONE\t1988-10-11T00:00:00Z\tALL", + "SECRET_TWO\t2020-12-04T00:00:00Z\tPRIVATE", + "SECRET_THREE\t1975-11-30T00:00:00Z\tSELECTED", }, }, } @@ -254,32 +259,38 @@ func Test_listRun(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { reg := &httpmock.Registry{} + reg.Verify(t) path := "repos/owner/repo/actions/secrets" if tt.opts.EnvName != "" { path = fmt.Sprintf("repos/owner/repo/environments/%s/secrets", tt.opts.EnvName) + } else if tt.opts.OrgName != "" { + path = fmt.Sprintf("orgs/%s/actions/secrets", tt.opts.OrgName) } t0, _ := time.Parse("2006-01-02", "1988-10-11") t1, _ := time.Parse("2006-01-02", "2020-12-04") t2, _ := time.Parse("2006-01-02", "1975-11-30") - payload := secretsPayload{} - payload.Secrets = []*Secret{ - { - Name: "SECRET_ONE", - UpdatedAt: t0, - }, - { - Name: "SECRET_TWO", - UpdatedAt: t1, - }, - { - Name: "SECRET_THREE", - UpdatedAt: t2, + payload := struct { + Secrets []Secret + }{ + Secrets: []Secret{ + { + Name: "SECRET_ONE", + UpdatedAt: t0, + }, + { + Name: "SECRET_TWO", + UpdatedAt: t1, + }, + { + Name: "SECRET_THREE", + UpdatedAt: t2, + }, }, } if tt.opts.OrgName != "" { - payload.Secrets = []*Secret{ + payload.Secrets = []Secret{ { Name: "SECRET_ONE", UpdatedAt: t0, @@ -297,7 +308,6 @@ func Test_listRun(t *testing.T) { SelectedReposURL: fmt.Sprintf("https://api.github.com/orgs/%s/actions/secrets/SECRET_THREE/repositories", tt.opts.OrgName), }, } - path = fmt.Sprintf("orgs/%s/actions/secrets", tt.opts.OrgName) if tt.tty { reg.Register( @@ -309,7 +319,7 @@ func Test_listRun(t *testing.T) { } if tt.opts.UserSecrets { - payload.Secrets = []*Secret{ + payload.Secrets = []Secret{ { Name: "SECRET_ONE", UpdatedAt: t0, @@ -365,43 +375,36 @@ func Test_listRun(t *testing.T) { tt.opts.Config = func() (config.Config, error) { return config.NewBlankConfig(), nil } + tt.opts.Now = func() time.Time { + t, _ := time.Parse(time.RFC822, "15 Mar 23 00:00 UTC") + return t + } err := listRun(tt.opts) assert.NoError(t, err) - reg.Verify(t) - - //nolint:staticcheck // prefer exact matchers over ExpectLines - test.ExpectLines(t, stdout.String(), tt.wantOut...) + expected := fmt.Sprintf("%s\n", strings.Join(tt.wantOut, "\n")) + assert.Equal(t, expected, stdout.String()) }) } } func Test_getSecrets_pagination(t *testing.T) { - var requests []*http.Request - var client testClient = func(req *http.Request) (*http.Response, error) { - header := make(map[string][]string) - if len(requests) == 0 { - header["Link"] = []string{`; rel="previous", ; rel="next"`} - } - requests = append(requests, req) - return &http.Response{ - Request: req, - Body: io.NopCloser(strings.NewReader(`{"secrets":[{},{}]}`)), - Header: header, - }, nil - } - + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.QueryMatcher("GET", "path/to", url.Values{"per_page": []string{"100"}}), + httpmock.WithHeader( + httpmock.StringResponse(`{"secrets":[{},{}]}`), + "Link", + `; rel="previous", ; rel="next"`), + ) + reg.Register( + httpmock.REST("GET", "page/2"), + httpmock.StringResponse(`{"secrets":[{},{}]}`), + ) + client := &http.Client{Transport: reg} secrets, err := getSecrets(client, "github.com", "path/to") assert.NoError(t, err) - assert.Equal(t, 2, len(requests)) assert.Equal(t, 4, len(secrets)) - assert.Equal(t, "https://api.github.com/path/to?per_page=100", requests[0].URL.String()) - assert.Equal(t, "http://example.com/page/2", requests[1].URL.String()) -} - -type testClient func(*http.Request) (*http.Response, error) - -func (c testClient) Do(req *http.Request) (*http.Response, error) { - return c(req) } diff --git a/pkg/cmd/variable/delete/delete.go b/pkg/cmd/variable/delete/delete.go index 743036bf3..5096c365e 100644 --- a/pkg/cmd/variable/delete/delete.go +++ b/pkg/cmd/variable/delete/delete.go @@ -23,7 +23,6 @@ type DeleteOptions struct { VariableName string OrgName string EnvName string - Application string } func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Command { diff --git a/pkg/cmd/variable/delete/delete_test.go b/pkg/cmd/variable/delete/delete_test.go index 578ddae38..c860a234e 100644 --- a/pkg/cmd/variable/delete/delete_test.go +++ b/pkg/cmd/variable/delete/delete_test.go @@ -84,120 +84,62 @@ func TestNewCmdDelete(t *testing.T) { } } -func Test_removeRun_repo(t *testing.T) { +func TestRemoveRun(t *testing.T) { tests := []struct { name string opts *DeleteOptions wantPath string }{ { - name: "Actions", + name: "repo", opts: &DeleteOptions{ - Application: "actions", VariableName: "cool_variable", }, wantPath: "repos/owner/repo/actions/variables/cool_variable", }, - } - - for _, tt := range tests { - reg := &httpmock.Registry{} - - reg.Register( - httpmock.REST("DELETE", tt.wantPath), - httpmock.StatusStringResponse(204, "No Content")) - - ios, _, _, _ := iostreams.Test() - - tt.opts.IO = ios - tt.opts.HttpClient = func() (*http.Client, error) { - return &http.Client{Transport: reg}, nil - } - tt.opts.Config = func() (config.Config, error) { - return config.NewBlankConfig(), nil - } - tt.opts.BaseRepo = func() (ghrepo.Interface, error) { - return ghrepo.FromFullName("owner/repo") - } - - err := removeRun(tt.opts) - assert.NoError(t, err) - - reg.Verify(t) - } -} - -func Test_removeRun_env(t *testing.T) { - reg := &httpmock.Registry{} - - reg.Register( - httpmock.REST("DELETE", "repositories/owner/repo/environments/development/variables/cool_variable"), - httpmock.StatusStringResponse(204, "No Content")) - - ios, _, _, _ := iostreams.Test() - - opts := &DeleteOptions{ - IO: ios, - HttpClient: func() (*http.Client, error) { - return &http.Client{Transport: reg}, nil + { + name: "env", + opts: &DeleteOptions{ + VariableName: "cool_variable", + EnvName: "development", + }, + wantPath: "repositories/owner/repo/environments/development/variables/cool_variable", }, - Config: func() (config.Config, error) { - return config.NewBlankConfig(), nil - }, - BaseRepo: func() (ghrepo.Interface, error) { - return ghrepo.FromFullName("owner/repo") - }, - VariableName: "cool_variable", - EnvName: "development", - } - - err := removeRun(opts) - assert.NoError(t, err) - - reg.Verify(t) -} - -func Test_removeRun_org(t *testing.T) { - tests := []struct { - name string - opts *DeleteOptions - wantPath string - }{ { name: "org", opts: &DeleteOptions{ - OrgName: "UmbrellaCorporation", + VariableName: "cool_variable", + OrgName: "UmbrellaCorporation", }, - wantPath: "orgs/UmbrellaCorporation/actions/variables/tVirus", + wantPath: "orgs/UmbrellaCorporation/actions/variables/cool_variable", }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { reg := &httpmock.Registry{} + defer reg.Verify(t) - reg.Register( - httpmock.REST("DELETE", tt.wantPath), + reg.Register(httpmock.REST("DELETE", tt.wantPath), httpmock.StatusStringResponse(204, "No Content")) ios, _, _, _ := iostreams.Test() + tt.opts.IO = ios + + tt.opts.HttpClient = func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + } tt.opts.Config = func() (config.Config, error) { return config.NewBlankConfig(), nil } + tt.opts.BaseRepo = func() (ghrepo.Interface, error) { return ghrepo.FromFullName("owner/repo") } - tt.opts.HttpClient = func() (*http.Client, error) { - return &http.Client{Transport: reg}, nil - } - tt.opts.IO = ios - tt.opts.VariableName = "tVirus" err := removeRun(tt.opts) assert.NoError(t, err) - - reg.Verify(t) }) } } diff --git a/pkg/cmd/variable/list/list.go b/pkg/cmd/variable/list/list.go index c816de42a..8754f8475 100644 --- a/pkg/cmd/variable/list/list.go +++ b/pkg/cmd/variable/list/list.go @@ -1,17 +1,14 @@ package list import ( - "encoding/json" "fmt" "net/http" - "regexp" "strings" "time" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" "github.com/cli/cli/v2/internal/config" - "github.com/cli/cli/v2/internal/ghinstance" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/tableprinter" "github.com/cli/cli/v2/pkg/cmd/variable/shared" @@ -25,10 +22,10 @@ type ListOptions struct { IO *iostreams.IOStreams Config func() (config.Config, error) BaseRepo func() (ghrepo.Interface, error) + Now func() time.Time - OrgName string - EnvName string - Application string + OrgName string + EnvName string } func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Command { @@ -36,6 +33,7 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman IO: f.IOStreams, Config: f.Config, HttpClient: f.HttpClient, + Now: time.Now, } cmd := &cobra.Command{ @@ -93,7 +91,7 @@ func listRun(opts *ListOptions) error { return err } - var variables []*Variable + var variables []Variable showSelectedRepoInfo := opts.IO.IsStdoutTTY() switch variableEntity { @@ -104,14 +102,11 @@ func listRun(opts *ListOptions) error { case shared.Organization: var cfg config.Config var host string - cfg, err = opts.Config() if err != nil { return err } - host, _ = cfg.Authentication().DefaultHost() - variables, err = getOrgVariables(client, host, orgName, showSelectedRepoInfo) } @@ -130,24 +125,20 @@ func listRun(opts *ListOptions) error { } table := tableprinter.New(opts.IO) - if opts.OrgName != "" { - table.HeaderRow("NAME", "VALUE", "UPDATED AT", "VISIBILITY") + if variableEntity == shared.Organization { + table.HeaderRow("Name", "Value", "Updated", "Visibility") } else { - table.HeaderRow("NAME", "VALUE", "UPDATED AT") + table.HeaderRow("Name", "Value", "Updated") } for _, variable := range variables { table.AddField(variable.Name) table.AddField(variable.Value) - updatedAt := variable.UpdatedAt.Format("2006-01-02") - if opts.IO.IsStdoutTTY() { - updatedAt = fmt.Sprintf("Updated %s", updatedAt) - } - table.AddField(updatedAt) + table.AddTimeField(opts.Now(), variable.UpdatedAt, nil) if variable.Visibility != "" { if showSelectedRepoInfo { - table.AddField(fmtVisibility(*variable)) + table.AddField(fmtVisibility(variable)) } else { - table.AddField(strings.ToUpper(string(variable.Visibility)), nil, nil) + table.AddField(strings.ToUpper(string(variable.Visibility))) } } table.EndRow() @@ -186,14 +177,22 @@ func fmtVisibility(s Variable) string { return "" } -func getOrgVariables(client httpClient, host, orgName string, showSelectedRepoInfo bool) ([]*Variable, error) { +func getRepoVariables(client *http.Client, repo ghrepo.Interface) ([]Variable, error) { + return getVariables(client, repo.RepoHost(), fmt.Sprintf("repos/%s/actions/variables", ghrepo.FullName(repo))) +} + +func getEnvVariables(client *http.Client, repo ghrepo.Interface, envName string) ([]Variable, error) { + path := fmt.Sprintf("repositories/%s/environments/%s/variables", ghrepo.FullName(repo), envName) + return getVariables(client, repo.RepoHost(), path) +} + +func getOrgVariables(client *http.Client, host, orgName string, showSelectedRepoInfo bool) ([]Variable, error) { variables, err := getVariables(client, host, fmt.Sprintf("orgs/%s/actions/variables", orgName)) if err != nil { return nil, err } - if showSelectedRepoInfo { - err = getSelectedRepositoryInformation(client, variables) + err = populateSelectedRepositoryInformation(client, host, variables) if err != nil { return nil, err } @@ -201,96 +200,37 @@ func getOrgVariables(client httpClient, host, orgName string, showSelectedRepoIn return variables, nil } -func getEnvVariables(client httpClient, repo ghrepo.Interface, envName string) ([]*Variable, error) { - path := fmt.Sprintf("repositories/%s/environments/%s/variables", ghrepo.FullName(repo), envName) - return getVariables(client, repo.RepoHost(), path) -} - -func getRepoVariables(client httpClient, repo ghrepo.Interface) ([]*Variable, error) { - return getVariables(client, repo.RepoHost(), fmt.Sprintf("repos/%s/actions/variables", - ghrepo.FullName(repo))) -} - -type variablesPayload struct { - Variables []*Variable -} - -type httpClient interface { - Do(*http.Request) (*http.Response, error) -} - -func getVariables(client httpClient, host, path string) ([]*Variable, error) { - var results []*Variable - url := fmt.Sprintf("%s%s?per_page=100", ghinstance.RESTPrefix(host), path) - - for { - var payload variablesPayload - nextURL, err := apiGet(client, url, &payload) +func getVariables(client *http.Client, host, path string) ([]Variable, error) { + var results []Variable + apiClient := api.NewClientFromHTTP(client) + path = fmt.Sprintf("%s?per_page=100", path) + for path != "" { + response := struct { + Variables []Variable + }{} + var err error + path, err = apiClient.RESTWithNext(host, "GET", path, nil, &response) if err != nil { return nil, err } - results = append(results, payload.Variables...) - - if nextURL == "" { - break - } - url = nextURL + results = append(results, response.Variables...) } - return results, nil } -func apiGet(client httpClient, url string, data interface{}) (string, error) { - req, err := http.NewRequest("GET", url, nil) - if err != nil { - return "", err - } - req.Header.Set("Content-Type", "application/json; charset=utf-8") - - resp, err := client.Do(req) - if err != nil { - return "", err - } - defer resp.Body.Close() - - if resp.StatusCode > 299 { - return "", api.HandleHTTPError(resp) - } - - dec := json.NewDecoder(resp.Body) - if err := dec.Decode(data); err != nil { - return "", err - } - - return findNextPage(resp.Header.Get("Link")), nil -} - -var linkRE = regexp.MustCompile(`<([^>]+)>;\s*rel="([^"]+)"`) - -func findNextPage(link string) string { - for _, m := range linkRE.FindAllStringSubmatch(link, -1) { - if len(m) > 2 && m[2] == "next" { - return m[1] - } - } - return "" -} - -func getSelectedRepositoryInformation(client httpClient, variables []*Variable) error { - type responseData struct { - TotalCount int `json:"total_count"` - } - - for _, variable := range variables { +func populateSelectedRepositoryInformation(client *http.Client, host string, variables []Variable) error { + apiClient := api.NewClientFromHTTP(client) + for i, variable := range variables { if variable.SelectedReposURL == "" { continue } - var result responseData - if _, err := apiGet(client, variable.SelectedReposURL, &result); err != nil { + response := struct { + TotalCount int `json:"total_count"` + }{} + if err := apiClient.REST(host, "GET", variable.SelectedReposURL, nil, &response); err != nil { return fmt.Errorf("failed determining selected repositories for %s: %w", variable.Name, err) } - variable.NumSelectedRepos = result.TotalCount + variables[i].NumSelectedRepos = response.TotalCount } - return nil } diff --git a/pkg/cmd/variable/list/list_test.go b/pkg/cmd/variable/list/list_test.go index d8982ef42..21669819e 100644 --- a/pkg/cmd/variable/list/list_test.go +++ b/pkg/cmd/variable/list/list_test.go @@ -3,8 +3,8 @@ package list import ( "bytes" "fmt" - "io" "net/http" + "net/url" "strings" "testing" "time" @@ -19,7 +19,7 @@ import ( "github.com/stretchr/testify/assert" ) -func Test_NewCmdList(t *testing.T) { +func TestNewCmdList(t *testing.T) { tests := []struct { name string cli string @@ -89,10 +89,10 @@ func Test_listRun(t *testing.T) { tty: true, opts: &ListOptions{}, wantOut: []string{ - "NAME VALUE UPDATED AT", - "VARIABLE_ONE one Updated 1988-10-11", - "VARIABLE_TWO two Updated 2020-12-04", - "VARIABLE_THREE three Updated 1975-11-30", + "NAME VALUE UPDATED", + "VARIABLE_ONE one about 34 years ago", + "VARIABLE_TWO two about 2 years ago", + "VARIABLE_THREE three about 47 years ago", }, }, { @@ -100,9 +100,9 @@ func Test_listRun(t *testing.T) { tty: false, opts: &ListOptions{}, wantOut: []string{ - "VARIABLE_ONE\tone\t1988-10-11", - "VARIABLE_TWO\ttwo\t2020-12-04", - "VARIABLE_THREE\tthree\t1975-11-30", + "VARIABLE_ONE\tone\t1988-10-11T00:00:00Z", + "VARIABLE_TWO\ttwo\t2020-12-04T00:00:00Z", + "VARIABLE_THREE\tthree\t1975-11-30T00:00:00Z", }, }, { @@ -112,10 +112,10 @@ func Test_listRun(t *testing.T) { OrgName: "UmbrellaCorporation", }, wantOut: []string{ - "NAME VALUE UPDATED AT VISIBILITY", - "VARIABLE_ONE org_one Updated 1988-10-11 Visible to all repositories", - "VARIABLE_TWO org_two Updated 2020-12-04 Visible to private repositories", - "VARIABLE_THREE org_three Updated 1975-11-30 Visible to 2 selected reposito...", + "NAME VALUE UPDATED VISIBILITY", + "VARIABLE_ONE org_one about 34 years ago Visible to all repositories", + "VARIABLE_TWO org_two about 2 years ago Visible to private repositories", + "VARIABLE_THREE org_three about 47 years ago Visible to 2 selected reposito...", }, }, { @@ -125,9 +125,9 @@ func Test_listRun(t *testing.T) { OrgName: "UmbrellaCorporation", }, wantOut: []string{ - "VARIABLE_ONE\torg_one\t1988-10-11\tALL", - "VARIABLE_TWO\torg_two\t2020-12-04\tPRIVATE", - "VARIABLE_THREE\torg_three\t1975-11-30\tSELECTED", + "VARIABLE_ONE\torg_one\t1988-10-11T00:00:00Z\tALL", + "VARIABLE_TWO\torg_two\t2020-12-04T00:00:00Z\tPRIVATE", + "VARIABLE_THREE\torg_three\t1975-11-30T00:00:00Z\tSELECTED", }, }, { @@ -137,10 +137,10 @@ func Test_listRun(t *testing.T) { EnvName: "Development", }, wantOut: []string{ - "NAME VALUE UPDATED AT", - "VARIABLE_ONE one Updated 1988-10-11", - "VARIABLE_TWO two Updated 2020-12-04", - "VARIABLE_THREE three Updated 1975-11-30", + "NAME VALUE UPDATED", + "VARIABLE_ONE one about 34 years ago", + "VARIABLE_TWO two about 2 years ago", + "VARIABLE_THREE three about 47 years ago", }, }, { @@ -150,9 +150,9 @@ func Test_listRun(t *testing.T) { EnvName: "Development", }, wantOut: []string{ - "VARIABLE_ONE\tone\t1988-10-11", - "VARIABLE_TWO\ttwo\t2020-12-04", - "VARIABLE_THREE\tthree\t1975-11-30", + "VARIABLE_ONE\tone\t1988-10-11T00:00:00Z", + "VARIABLE_TWO\ttwo\t2020-12-04T00:00:00Z", + "VARIABLE_THREE\tthree\t1975-11-30T00:00:00Z", }, }, } @@ -160,35 +160,41 @@ func Test_listRun(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { reg := &httpmock.Registry{} + defer reg.Verify(t) path := "repos/owner/repo/actions/variables" if tt.opts.EnvName != "" { path = fmt.Sprintf("repositories/owner/repo/environments/%s/variables", tt.opts.EnvName) + } else if tt.opts.OrgName != "" { + path = fmt.Sprintf("orgs/%s/actions/variables", tt.opts.OrgName) } t0, _ := time.Parse("2006-01-02", "1988-10-11") t1, _ := time.Parse("2006-01-02", "2020-12-04") t2, _ := time.Parse("2006-01-02", "1975-11-30") - payload := variablesPayload{} - payload.Variables = []*Variable{ - { - Name: "VARIABLE_ONE", - Value: "one", - UpdatedAt: t0, - }, - { - Name: "VARIABLE_TWO", - Value: "two", - UpdatedAt: t1, - }, - { - Name: "VARIABLE_THREE", - Value: "three", - UpdatedAt: t2, + payload := struct { + Variables []Variable + }{ + Variables: []Variable{ + { + Name: "VARIABLE_ONE", + Value: "one", + UpdatedAt: t0, + }, + { + Name: "VARIABLE_TWO", + Value: "two", + UpdatedAt: t1, + }, + { + Name: "VARIABLE_THREE", + Value: "three", + UpdatedAt: t2, + }, }, } if tt.opts.OrgName != "" { - payload.Variables = []*Variable{ + payload.Variables = []Variable{ { Name: "VARIABLE_ONE", Value: "org_one", @@ -209,8 +215,6 @@ func Test_listRun(t *testing.T) { SelectedReposURL: fmt.Sprintf("https://api.github.com/orgs/%s/actions/variables/VARIABLE_THREE/repositories", tt.opts.OrgName), }, } - path = fmt.Sprintf("orgs/%s/actions/variables", tt.opts.OrgName) - if tt.tty { reg.Register( httpmock.REST("GET", fmt.Sprintf("orgs/%s/actions/variables/VARIABLE_THREE/repositories", tt.opts.OrgName)), @@ -236,44 +240,36 @@ func Test_listRun(t *testing.T) { tt.opts.Config = func() (config.Config, error) { return config.NewBlankConfig(), nil } + tt.opts.Now = func() time.Time { + t, _ := time.Parse(time.RFC822, "15 Mar 23 00:00 UTC") + return t + } err := listRun(tt.opts) assert.NoError(t, err) - reg.Verify(t) - outputLines := strings.Split(stdout.String(), "\n") - for i, l := range tt.wantOut { - assert.Equal(t, outputLines[i], l) - } + expected := fmt.Sprintf("%s\n", strings.Join(tt.wantOut, "\n")) + assert.Equal(t, expected, stdout.String()) }) } } func Test_getVariables_pagination(t *testing.T) { - var requests []*http.Request - var client testClient = func(req *http.Request) (*http.Response, error) { - header := make(map[string][]string) - if len(requests) == 0 { - header["Link"] = []string{`; rel="previous", ; rel="next"`} - } - requests = append(requests, req) - return &http.Response{ - Request: req, - Body: io.NopCloser(strings.NewReader(`{"variables":[{},{}]}`)), - Header: header, - }, nil - } - + reg := &httpmock.Registry{} + defer reg.Verify(t) + reg.Register( + httpmock.QueryMatcher("GET", "path/to", url.Values{"per_page": []string{"100"}}), + httpmock.WithHeader( + httpmock.StringResponse(`{"variables":[{},{}]}`), + "Link", + `; rel="previous", ; rel="next"`), + ) + reg.Register( + httpmock.REST("GET", "page/2"), + httpmock.StringResponse(`{"variables":[{},{}]}`), + ) + client := &http.Client{Transport: reg} variables, err := getVariables(client, "github.com", "path/to") assert.NoError(t, err) - assert.Equal(t, 2, len(requests)) assert.Equal(t, 4, len(variables)) - assert.Equal(t, "https://api.github.com/path/to?per_page=100", requests[0].URL.String()) - assert.Equal(t, "http://example.com/page/2", requests[1].URL.String()) -} - -type testClient func(*http.Request) (*http.Response, error) - -func (c testClient) Do(req *http.Request) (*http.Response, error) { - return c(req) } diff --git a/pkg/cmd/variable/set/set.go b/pkg/cmd/variable/set/set.go index 725f9ee40..909233a40 100644 --- a/pkg/cmd/variable/set/set.go +++ b/pkg/cmd/variable/set/set.go @@ -50,7 +50,7 @@ func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command cmd := &cobra.Command{ Use: "set ", - Short: "Set variables", + Short: "Create or update variables", Long: heredoc.Doc(` Set a value for a variable on one of the following levels: - repository (default): available to Actions runs or Dependabot in a repository @@ -219,7 +219,7 @@ func setRun(opts *SetOptions) error { if envName != "" { target += " environment " + envName } - fmt.Fprintf(opts.IO.Out, "%s %s actions variable %s for %s\n", cs.SuccessIcon(), result.Operation, result.Key, target) + fmt.Fprintf(opts.IO.Out, "%s %s Actions variable %s for %s\n", cs.SuccessIcon(), result.Operation, result.Key, target) } return err diff --git a/pkg/cmd/variable/variable.go b/pkg/cmd/variable/variable.go index 1768227c8..50a61f4e9 100644 --- a/pkg/cmd/variable/variable.go +++ b/pkg/cmd/variable/variable.go @@ -14,9 +14,9 @@ func NewCmdVariable(f *cmdutil.Factory) *cobra.Command { Use: "variable ", Short: "Manage GitHub Actions variables", Long: heredoc.Doc(` - Variables can be set at the repository, environment or organization level for use in GitHub Actions. - Run "gh help variable set" to learn how to get started. -`), + Variables can be set at the repository, environment or organization level for use in + GitHub Actions or Dependabot. Run "gh help variable set" to learn how to get started. + `), } cmdutil.EnableRepoOverride(cmd, f) @@ -24,5 +24,6 @@ func NewCmdVariable(f *cmdutil.Factory) *cobra.Command { cmd.AddCommand(cmdSet.NewCmdSet(f, nil)) cmd.AddCommand(cmdList.NewCmdList(f, nil)) cmd.AddCommand(cmdDelete.NewCmdDelete(f, nil)) + return cmd }