From 14196b48fedecf4498743f70d5a543915fa8a610 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Arne=20J=C3=B8rgensen?= Date: Tue, 21 May 2024 21:44:41 +0200 Subject: [PATCH 1/2] Add a `gh variable get FOO` command Closes #9103. --- pkg/cmd/variable/get/get.go | 128 +++++++++++++++++++++ pkg/cmd/variable/get/get_test.go | 188 +++++++++++++++++++++++++++++++ pkg/cmd/variable/variable.go | 2 + 3 files changed, 318 insertions(+) create mode 100644 pkg/cmd/variable/get/get.go create mode 100644 pkg/cmd/variable/get/get_test.go diff --git a/pkg/cmd/variable/get/get.go b/pkg/cmd/variable/get/get.go new file mode 100644 index 00000000000..b7bc7652b16 --- /dev/null +++ b/pkg/cmd/variable/get/get.go @@ -0,0 +1,128 @@ +package get + +import ( + "fmt" + "net/http" + "time" + + "github.com/MakeNowJust/heredoc" + "github.com/cli/cli/v2/api" + "github.com/cli/cli/v2/internal/gh" + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/cmd/variable/shared" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/spf13/cobra" +) + +type GetOptions struct { + HttpClient func() (*http.Client, error) + IO *iostreams.IOStreams + Config func() (gh.Config, error) + BaseRepo func() (ghrepo.Interface, error) + + VariableName string + OrgName string + EnvName string +} + +type Variable struct { + Name string `json:"name"` + Value string `json:"value"` + UpdatedAt time.Time `json:"updated_at"` + Visibility shared.Visibility `json:"visibility"` + SelectedReposURL string `json:"selected_repositories_url"` + NumSelectedRepos int `json:"num_selected_repos"` +} + +func NewCmdGet(f *cmdutil.Factory, runF func(*GetOptions) error) *cobra.Command { + opts := &GetOptions{ + IO: f.IOStreams, + Config: f.Config, + HttpClient: f.HttpClient, + } + + cmd := &cobra.Command{ + Use: "get ", + Short: "Get variables", + Long: heredoc.Doc(` + Get a variable on one of the following levels: + - repository (default): available to GitHub Actions runs or Dependabot in a repository + - environment: available to GitHub Actions runs for a deployment environment in a repository + - organization: available to GitHub Actions runs or Dependabot within an organization + `), + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + // support `-R, --repo` override + opts.BaseRepo = f.BaseRepo + + if err := cmdutil.MutuallyExclusive("specify only one of `--org` or `--env`", opts.OrgName != "", opts.EnvName != ""); err != nil { + return err + } + + opts.VariableName = args[0] + + if runF != nil { + return runF(opts) + } + + return getRun(opts) + }, + } + cmd.Flags().StringVarP(&opts.OrgName, "org", "o", "", "Get a variable for an organization") + cmd.Flags().StringVarP(&opts.EnvName, "env", "e", "", "Get a variable for an environment") + + return cmd +} + +func getRun(opts *GetOptions) error { + c, err := opts.HttpClient() + if err != nil { + return fmt.Errorf("could not create http client: %w", err) + } + client := api.NewClientFromHTTP(c) + + orgName := opts.OrgName + envName := opts.EnvName + + variableEntity, err := shared.GetVariableEntity(orgName, envName) + if err != nil { + return err + } + + var baseRepo ghrepo.Interface + if variableEntity == shared.Repository || variableEntity == shared.Environment { + baseRepo, err = opts.BaseRepo() + if err != nil { + return err + } + } + + var path string + switch variableEntity { + case shared.Organization: + path = fmt.Sprintf("orgs/%s/actions/variables/%s", orgName, opts.VariableName) + case shared.Environment: + path = fmt.Sprintf("repos/%s/environments/%s/variables/%s", ghrepo.FullName(baseRepo), envName, opts.VariableName) + case shared.Repository: + path = fmt.Sprintf("repos/%s/actions/variables/%s", ghrepo.FullName(baseRepo), opts.VariableName) + } + + cfg, err := opts.Config() + if err != nil { + return err + } + + host, _ := cfg.Authentication().DefaultHost() + + response := &Variable{} + + err = client.REST(host, "GET", path, nil, &response) + if err != nil { + return fmt.Errorf("failed to get variable %s: %w", opts.VariableName, err) + } + + fmt.Fprintf(opts.IO.Out, "%s", response.Value) + + return nil +} diff --git a/pkg/cmd/variable/get/get_test.go b/pkg/cmd/variable/get/get_test.go new file mode 100644 index 00000000000..9ee697ba5ce --- /dev/null +++ b/pkg/cmd/variable/get/get_test.go @@ -0,0 +1,188 @@ +package get + +import ( + "bytes" + "fmt" + "net/http" + "testing" + + "github.com/cli/cli/v2/internal/config" + "github.com/cli/cli/v2/internal/gh" + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/pkg/cmdutil" + "github.com/cli/cli/v2/pkg/httpmock" + "github.com/cli/cli/v2/pkg/iostreams" + "github.com/google/shlex" + "github.com/stretchr/testify/assert" +) + +func TestNewCmdList(t *testing.T) { + tests := []struct { + name string + cli string + wants GetOptions + }{ + { + name: "repo", + cli: "FOO", + wants: GetOptions{ + OrgName: "", + VariableName: "FOO", + }, + }, + { + name: "org", + cli: "-oUmbrellaCorporation BAR", + wants: GetOptions{ + OrgName: "UmbrellaCorporation", + VariableName: "BAR", + }, + }, + { + name: "env", + cli: "-eDevelopment BAZ", + wants: GetOptions{ + EnvName: "Development", + VariableName: "BAZ", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + f := &cmdutil.Factory{ + IOStreams: ios, + } + + argv, err := shlex.Split(tt.cli) + assert.NoError(t, err) + + var gotOpts *GetOptions + cmd := NewCmdGet(f, func(opts *GetOptions) error { + gotOpts = opts + return nil + }) + cmd.SetArgs(argv) + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + + _, err = cmd.ExecuteC() + assert.NoError(t, err) + + assert.Equal(t, tt.wants.OrgName, gotOpts.OrgName) + assert.Equal(t, tt.wants.EnvName, gotOpts.EnvName) + assert.Equal(t, tt.wants.VariableName, gotOpts.VariableName) + }) + } +} + +func Test_getRun(t *testing.T) { + tests := []struct { + name string + tty bool + opts *GetOptions + wantOut string + }{ + { + name: "repo tty", + tty: true, + opts: &GetOptions{ + VariableName: "VARIABLE_ONE", + }, + wantOut: "one", + }, + { + name: "repo not tty", + tty: false, + opts: &GetOptions{ + VariableName: "VARIABLE_ONE", + }, + wantOut: "one", + }, + { + name: "org tty", + tty: true, + opts: &GetOptions{ + OrgName: "UmbrellaCorporation", + VariableName: "VARIABLE_ONE", + }, + wantOut: "org_one", + }, + { + name: "org not tty", + tty: false, + opts: &GetOptions{ + OrgName: "UmbrellaCorporation", + VariableName: "VARIABLE_ONE", + }, + wantOut: "org_one", + }, + { + name: "env tty", + tty: true, + opts: &GetOptions{ + EnvName: "Development", + VariableName: "VARIABLE_ONE", + }, + wantOut: "one", + }, + { + name: "env not tty", + tty: false, + opts: &GetOptions{ + EnvName: "Development", + VariableName: "VARIABLE_ONE", + }, + wantOut: "one", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + defer reg.Verify(t) + + path := fmt.Sprintf("repos/owner/repo/actions/variables/%s", tt.opts.VariableName) + if tt.opts.EnvName != "" { + path = fmt.Sprintf("repos/owner/repo/environments/%s/variables/%s", tt.opts.EnvName, tt.opts.VariableName) + } else if tt.opts.OrgName != "" { + path = fmt.Sprintf("orgs/%s/actions/variables/%s", tt.opts.OrgName, tt.opts.VariableName) + } + + payload := Variable{ + Name: "VARIABLE_ONE", + Value: "one", + } + if tt.opts.OrgName != "" { + payload = Variable{ + Name: "VARIABLE_ONE", + Value: "org_one", + } + } + + reg.Register(httpmock.REST("GET", path), httpmock.JSONResponse(payload)) + + ios, _, stdout, _ := iostreams.Test() + + ios.SetStdoutTTY(tt.tty) + + tt.opts.IO = ios + 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.Config = func() (gh.Config, error) { + return config.NewBlankConfig(), nil + } + + err := getRun(tt.opts) + assert.NoError(t, err) + + assert.Equal(t, tt.wantOut, stdout.String()) + }) + } +} diff --git a/pkg/cmd/variable/variable.go b/pkg/cmd/variable/variable.go index f064a81e359..15ff387444f 100644 --- a/pkg/cmd/variable/variable.go +++ b/pkg/cmd/variable/variable.go @@ -3,6 +3,7 @@ package variable import ( "github.com/MakeNowJust/heredoc" cmdDelete "github.com/cli/cli/v2/pkg/cmd/variable/delete" + cmdGet "github.com/cli/cli/v2/pkg/cmd/variable/get" cmdList "github.com/cli/cli/v2/pkg/cmd/variable/list" cmdSet "github.com/cli/cli/v2/pkg/cmd/variable/set" "github.com/cli/cli/v2/pkg/cmdutil" @@ -21,6 +22,7 @@ func NewCmdVariable(f *cmdutil.Factory) *cobra.Command { cmdutil.EnableRepoOverride(cmd, f) + cmd.AddCommand(cmdGet.NewCmdGet(f, nil)) cmd.AddCommand(cmdSet.NewCmdSet(f, nil)) cmd.AddCommand(cmdList.NewCmdList(f, nil)) cmd.AddCommand(cmdDelete.NewCmdDelete(f, nil)) From 1cb777156bca84950360953560118d4ba29c1b5d Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 23 May 2024 16:55:16 +0200 Subject: [PATCH 2/2] Adjust tests and minor implementation details --- pkg/cmd/variable/get/get.go | 28 ++--- pkg/cmd/variable/get/get_test.go | 174 +++++++++++++++++-------------- 2 files changed, 110 insertions(+), 92 deletions(-) diff --git a/pkg/cmd/variable/get/get.go b/pkg/cmd/variable/get/get.go index b7bc7652b16..a946411340c 100644 --- a/pkg/cmd/variable/get/get.go +++ b/pkg/cmd/variable/get/get.go @@ -1,9 +1,9 @@ package get import ( + "errors" "fmt" "net/http" - "time" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" @@ -26,13 +26,14 @@ type GetOptions struct { EnvName string } -type Variable struct { - Name string `json:"name"` - Value string `json:"value"` - UpdatedAt time.Time `json:"updated_at"` - Visibility shared.Visibility `json:"visibility"` - SelectedReposURL string `json:"selected_repositories_url"` - NumSelectedRepos int `json:"num_selected_repos"` +type getVariableResponse struct { + Value string `json:"value"` + // Other available but unused fields + // Name string `json:"name"` + // UpdatedAt time.Time `json:"updated_at"` + // Visibility shared.Visibility `json:"visibility"` + // SelectedReposURL string `json:"selected_repositories_url"` + // NumSelectedRepos int `json:"num_selected_repos"` } func NewCmdGet(f *cmdutil.Factory, runF func(*GetOptions) error) *cobra.Command { @@ -115,14 +116,17 @@ func getRun(opts *GetOptions) error { host, _ := cfg.Authentication().DefaultHost() - response := &Variable{} + var response getVariableResponse + if err = client.REST(host, "GET", path, nil, &response); err != nil { + var httpErr api.HTTPError + if errors.As(err, &httpErr) && httpErr.StatusCode == http.StatusNotFound { + return fmt.Errorf("variable %s was not found", opts.VariableName) + } - err = client.REST(host, "GET", path, nil, &response) - if err != nil { return fmt.Errorf("failed to get variable %s: %w", opts.VariableName, err) } - fmt.Fprintf(opts.IO.Out, "%s", response.Value) + fmt.Fprintf(opts.IO.Out, "%s\n", response.Value) return nil } diff --git a/pkg/cmd/variable/get/get_test.go b/pkg/cmd/variable/get/get_test.go index 9ee697ba5ce..60e2d8fa6ed 100644 --- a/pkg/cmd/variable/get/get_test.go +++ b/pkg/cmd/variable/get/get_test.go @@ -14,13 +14,15 @@ import ( "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) -func TestNewCmdList(t *testing.T) { +func TestNewCmdGet(t *testing.T) { tests := []struct { - name string - cli string - wants GetOptions + name string + cli string + wants GetOptions + wantErr error }{ { name: "repo", @@ -32,20 +34,25 @@ func TestNewCmdList(t *testing.T) { }, { name: "org", - cli: "-oUmbrellaCorporation BAR", + cli: "-o TestOrg BAR", wants: GetOptions{ - OrgName: "UmbrellaCorporation", + OrgName: "TestOrg", VariableName: "BAR", }, }, { name: "env", - cli: "-eDevelopment BAZ", + cli: "-e Development BAZ", wants: GetOptions{ EnvName: "Development", VariableName: "BAZ", }, }, + { + name: "org and env", + cli: "-o TestOrg -e Development QUX", + wantErr: cmdutil.FlagErrorf("%s", "specify only one of `--org` or `--env`"), + }, } for _, tt := range tests { @@ -69,120 +76,127 @@ func TestNewCmdList(t *testing.T) { cmd.SetErr(&bytes.Buffer{}) _, err = cmd.ExecuteC() - assert.NoError(t, err) + if tt.wantErr != nil { + require.Equal(t, err, tt.wantErr) + return + } + require.NoError(t, err) - assert.Equal(t, tt.wants.OrgName, gotOpts.OrgName) - assert.Equal(t, tt.wants.EnvName, gotOpts.EnvName) - assert.Equal(t, tt.wants.VariableName, gotOpts.VariableName) + require.Equal(t, tt.wants.OrgName, gotOpts.OrgName) + require.Equal(t, tt.wants.EnvName, gotOpts.EnvName) + require.Equal(t, tt.wants.VariableName, gotOpts.VariableName) }) } } func Test_getRun(t *testing.T) { tests := []struct { - name string - tty bool - opts *GetOptions - wantOut string + name string + opts *GetOptions + httpStubs func(*httpmock.Registry) + wantOut string + wantErr error }{ { - name: "repo tty", - tty: true, + name: "getting repo variable", opts: &GetOptions{ VariableName: "VARIABLE_ONE", }, - wantOut: "one", - }, - { - name: "repo not tty", - tty: false, - opts: &GetOptions{ - VariableName: "VARIABLE_ONE", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("GET", "repos/owner/repo/actions/variables/VARIABLE_ONE"), + httpmock.JSONResponse(getVariableResponse{ + Value: "repo_var", + })) }, - wantOut: "one", + wantOut: "repo_var\n", }, { - name: "org tty", - tty: true, + name: "getting org variable", opts: &GetOptions{ - OrgName: "UmbrellaCorporation", + OrgName: "TestOrg", VariableName: "VARIABLE_ONE", }, - wantOut: "org_one", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("GET", "orgs/TestOrg/actions/variables/VARIABLE_ONE"), + httpmock.JSONResponse(getVariableResponse{ + Value: "org_var", + })) + }, + wantOut: "org_var\n", }, { - name: "org not tty", - tty: false, + name: "getting env variable", opts: &GetOptions{ - OrgName: "UmbrellaCorporation", + EnvName: "Development", VariableName: "VARIABLE_ONE", }, - wantOut: "org_one", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("GET", "repos/owner/repo/environments/Development/variables/VARIABLE_ONE"), + httpmock.JSONResponse(getVariableResponse{ + Value: "env_var", + })) + }, + wantOut: "env_var\n", }, { - name: "env tty", - tty: true, + name: "when the variable is not found, an error is returned", opts: &GetOptions{ - EnvName: "Development", VariableName: "VARIABLE_ONE", }, - wantOut: "one", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("GET", "repos/owner/repo/actions/variables/VARIABLE_ONE"), + httpmock.StatusStringResponse(404, "not found"), + ) + }, + wantErr: fmt.Errorf("variable VARIABLE_ONE was not found"), }, { - name: "env not tty", - tty: false, + name: "when getting any variable from API fails, the error is bubbled with context", opts: &GetOptions{ - EnvName: "Development", VariableName: "VARIABLE_ONE", }, - wantOut: "one", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.REST("GET", "repos/owner/repo/actions/variables/VARIABLE_ONE"), + httpmock.StatusStringResponse(400, "not found"), + ) + }, + wantErr: fmt.Errorf("failed to get variable VARIABLE_ONE: HTTP 400 (https://api.github.com/repos/owner/repo/actions/variables/VARIABLE_ONE)"), }, } for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - reg := &httpmock.Registry{} - defer reg.Verify(t) - - path := fmt.Sprintf("repos/owner/repo/actions/variables/%s", tt.opts.VariableName) - if tt.opts.EnvName != "" { - path = fmt.Sprintf("repos/owner/repo/environments/%s/variables/%s", tt.opts.EnvName, tt.opts.VariableName) - } else if tt.opts.OrgName != "" { - path = fmt.Sprintf("orgs/%s/actions/variables/%s", tt.opts.OrgName, tt.opts.VariableName) - } - - payload := Variable{ - Name: "VARIABLE_ONE", - Value: "one", - } - if tt.opts.OrgName != "" { - payload = Variable{ - Name: "VARIABLE_ONE", - Value: "org_one", + var runTest = func(tty bool) func(t *testing.T) { + return func(t *testing.T) { + reg := &httpmock.Registry{} + tt.httpStubs(reg) + defer reg.Verify(t) + + ios, _, stdout, _ := iostreams.Test() + ios.SetStdoutTTY(tty) + + tt.opts.IO = ios + 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.Config = func() (gh.Config, error) { + return config.NewBlankConfig(), nil } - } - - reg.Register(httpmock.REST("GET", path), httpmock.JSONResponse(payload)) - - ios, _, stdout, _ := iostreams.Test() - ios.SetStdoutTTY(tt.tty) + err := getRun(tt.opts) + if err != nil { + require.EqualError(t, tt.wantErr, err.Error()) + return + } - tt.opts.IO = ios - 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 + require.NoError(t, err) + require.Equal(t, tt.wantOut, stdout.String()) } - tt.opts.Config = func() (gh.Config, error) { - return config.NewBlankConfig(), nil - } - - err := getRun(tt.opts) - assert.NoError(t, err) + } - assert.Equal(t, tt.wantOut, stdout.String()) - }) + t.Run(tt.name+" tty", runTest(true)) + t.Run(tt.name+" no-tty", runTest(false)) } }