From 22a5a4c899cec911152cbb27acb539ee7505655d Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Fri, 2 Aug 2024 10:36:32 -0400 Subject: [PATCH 1/2] Update `gh variable get` to use repo host Closes #9274 This change addresses a bug where repository and repository environment variables are not being retrieved from the appropriate host, causing GHES users to fail without specifying `GH_HOST` to force `gh variable set` to do the right thing. In addition, the tests for this command have been updated to check both github.com and GHES variations to ensure this works. --- pkg/cmd/variable/get/get.go | 24 +++++++++++--------- pkg/cmd/variable/get/get_test.go | 39 ++++++++++++++++++++++++++++++-- pkg/httpmock/stub.go | 9 ++++++++ 3 files changed, 59 insertions(+), 13 deletions(-) diff --git a/pkg/cmd/variable/get/get.go b/pkg/cmd/variable/get/get.go index 32b4c5e62..e4def5a03 100644 --- a/pkg/cmd/variable/get/get.go +++ b/pkg/cmd/variable/get/get.go @@ -92,22 +92,24 @@ func getRun(opts *GetOptions) error { } } - 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() + var path string + var host string + switch variableEntity { + case shared.Organization: + path = fmt.Sprintf("orgs/%s/actions/variables/%s", orgName, opts.VariableName) + host, _ = cfg.Authentication().DefaultHost() + case shared.Environment: + path = fmt.Sprintf("repos/%s/environments/%s/variables/%s", ghrepo.FullName(baseRepo), envName, opts.VariableName) + host = baseRepo.RepoHost() + case shared.Repository: + path = fmt.Sprintf("repos/%s/actions/variables/%s", ghrepo.FullName(baseRepo), opts.VariableName) + host = baseRepo.RepoHost() + } var variable shared.Variable if err = client.REST(host, "GET", path, nil, &variable); err != nil { diff --git a/pkg/cmd/variable/get/get_test.go b/pkg/cmd/variable/get/get_test.go index 4a03beb6a..e85910152 100644 --- a/pkg/cmd/variable/get/get_test.go +++ b/pkg/cmd/variable/get/get_test.go @@ -110,6 +110,7 @@ func Test_getRun(t *testing.T) { tests := []struct { name string opts *GetOptions + host string httpStubs func(*httpmock.Registry) jsonFields []string wantOut string @@ -120,8 +121,23 @@ func Test_getRun(t *testing.T) { opts: &GetOptions{ VariableName: "VARIABLE_ONE", }, + host: "github.com", httpStubs: func(reg *httpmock.Registry) { - reg.Register(httpmock.REST("GET", "repos/owner/repo/actions/variables/VARIABLE_ONE"), + reg.Register(httpmock.WithHost(httpmock.REST("GET", "repos/owner/repo/actions/variables/VARIABLE_ONE"), "api.github.com"), + httpmock.JSONResponse(shared.Variable{ + Value: "repo_var", + })) + }, + wantOut: "repo_var\n", + }, + { + name: "getting GHES repo variable", + opts: &GetOptions{ + VariableName: "VARIABLE_ONE", + }, + host: "ghe.io", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.WithHost(httpmock.REST("GET", "api/v3/repos/owner/repo/actions/variables/VARIABLE_ONE"), "ghe.io"), httpmock.JSONResponse(shared.Variable{ Value: "repo_var", })) @@ -148,8 +164,24 @@ func Test_getRun(t *testing.T) { EnvName: "Development", VariableName: "VARIABLE_ONE", }, + host: "github.com", httpStubs: func(reg *httpmock.Registry) { - reg.Register(httpmock.REST("GET", "repos/owner/repo/environments/Development/variables/VARIABLE_ONE"), + reg.Register(httpmock.WithHost(httpmock.REST("GET", "repos/owner/repo/environments/Development/variables/VARIABLE_ONE"), "api.github.com"), + httpmock.JSONResponse(shared.Variable{ + Value: "env_var", + })) + }, + wantOut: "env_var\n", + }, + { + name: "getting GHES env variable", + opts: &GetOptions{ + EnvName: "Development", + VariableName: "VARIABLE_ONE", + }, + host: "ghe.io", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.WithHost(httpmock.REST("GET", "api/v3/repos/owner/repo/environments/Development/variables/VARIABLE_ONE"), "ghe.io"), httpmock.JSONResponse(shared.Variable{ Value: "env_var", })) @@ -226,6 +258,9 @@ func Test_getRun(t *testing.T) { tt.opts.IO = ios tt.opts.BaseRepo = func() (ghrepo.Interface, error) { + if tt.host != "" { + return ghrepo.FromFullNameWithHost("owner/repo", tt.host) + } return ghrepo.FromFullName("owner/repo") } tt.opts.HttpClient = func() (*http.Client, error) { diff --git a/pkg/httpmock/stub.go b/pkg/httpmock/stub.go index 147d36fc8..e7534d7f8 100644 --- a/pkg/httpmock/stub.go +++ b/pkg/httpmock/stub.go @@ -123,6 +123,15 @@ func StringResponse(body string) Responder { } } +func WithHost(matcher Matcher, host string) Matcher { + return func(req *http.Request) bool { + if !strings.EqualFold(req.Host, host) { + return false + } + return matcher(req) + } +} + func WithHeader(responder Responder, header string, value string) Responder { return func(req *http.Request) (*http.Response, error) { resp, _ := responder(req) From c088951944082eb4c474097edb1861ed07d6c856 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Fri, 2 Aug 2024 15:11:22 -0400 Subject: [PATCH 2/2] Fix host handling in variable and secret delete This change updates `gh variable delete` and `gh secret delete` to use the host associated with the repository rather than the default host. Along with these changes is minor refactoring of the tests to ensure appropriate Dotcom vs GHES targeting works as expected. Minor edit to `gh variable get` testing to simplify some conditional logic in test setup. --- pkg/cmd/secret/delete/delete.go | 29 +++--- pkg/cmd/secret/delete/delete_test.go | 127 ++++++++++++++++++------- pkg/cmd/variable/delete/delete.go | 24 ++--- pkg/cmd/variable/delete/delete_test.go | 60 +++++++++--- pkg/cmd/variable/get/get_test.go | 17 ++-- 5 files changed, 176 insertions(+), 81 deletions(-) diff --git a/pkg/cmd/secret/delete/delete.go b/pkg/cmd/secret/delete/delete.go index 9a299ce4f..564e8633f 100644 --- a/pkg/cmd/secret/delete/delete.go +++ b/pkg/cmd/secret/delete/delete.go @@ -105,24 +105,27 @@ func removeRun(opts *DeleteOptions) error { } } - var path string - switch secretEntity { - case shared.Organization: - path = fmt.Sprintf("orgs/%s/%s/secrets/%s", orgName, secretApp, opts.SecretName) - case shared.Environment: - path = fmt.Sprintf("repos/%s/environments/%s/secrets/%s", ghrepo.FullName(baseRepo), envName, opts.SecretName) - case shared.User: - path = fmt.Sprintf("user/codespaces/secrets/%s", opts.SecretName) - case shared.Repository: - path = fmt.Sprintf("repos/%s/%s/secrets/%s", ghrepo.FullName(baseRepo), secretApp, opts.SecretName) - } - cfg, err := opts.Config() if err != nil { return err } - host, _ := cfg.Authentication().DefaultHost() + var path string + var host string + switch secretEntity { + case shared.Organization: + path = fmt.Sprintf("orgs/%s/%s/secrets/%s", orgName, secretApp, opts.SecretName) + host, _ = cfg.Authentication().DefaultHost() + case shared.Environment: + path = fmt.Sprintf("repos/%s/environments/%s/secrets/%s", ghrepo.FullName(baseRepo), envName, opts.SecretName) + host = baseRepo.RepoHost() + case shared.User: + path = fmt.Sprintf("user/codespaces/secrets/%s", opts.SecretName) + host, _ = cfg.Authentication().DefaultHost() + case shared.Repository: + path = fmt.Sprintf("repos/%s/%s/secrets/%s", ghrepo.FullName(baseRepo), secretApp, opts.SecretName) + host = baseRepo.RepoHost() + } err = client.REST(host, "DELETE", path, nil, nil) if err != nil { diff --git a/pkg/cmd/secret/delete/delete_test.go b/pkg/cmd/secret/delete/delete_test.go index 9d2c078c7..bd5053f4d 100644 --- a/pkg/cmd/secret/delete/delete_test.go +++ b/pkg/cmd/secret/delete/delete_test.go @@ -13,6 +13,7 @@ import ( "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNewCmdDelete(t *testing.T) { @@ -121,9 +122,10 @@ func TestNewCmdDelete(t *testing.T) { func Test_removeRun_repo(t *testing.T) { tests := []struct { - name string - opts *DeleteOptions - wantPath string + name string + opts *DeleteOptions + host string + httpStubs func(*httpmock.Registry) }{ { name: "Actions", @@ -131,7 +133,21 @@ func Test_removeRun_repo(t *testing.T) { Application: "actions", SecretName: "cool_secret", }, - wantPath: "repos/owner/repo/actions/secrets/cool_secret", + host: "github.com", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.WithHost(httpmock.REST("DELETE", "repos/owner/repo/actions/secrets/cool_secret"), "api.github.com"), httpmock.StatusStringResponse(204, "No Content")) + }, + }, + { + name: "Actions GHES", + opts: &DeleteOptions{ + Application: "actions", + SecretName: "cool_secret", + }, + host: "example.com", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.WithHost(httpmock.REST("DELETE", "api/v3/repos/owner/repo/actions/secrets/cool_secret"), "example.com"), httpmock.StatusStringResponse(204, "No Content")) + }, }, { name: "Dependabot", @@ -139,23 +155,38 @@ func Test_removeRun_repo(t *testing.T) { Application: "dependabot", SecretName: "cool_dependabot_secret", }, - wantPath: "repos/owner/repo/dependabot/secrets/cool_dependabot_secret", + host: "github.com", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.WithHost(httpmock.REST("DELETE", "repos/owner/repo/dependabot/secrets/cool_dependabot_secret"), "api.github.com"), httpmock.StatusStringResponse(204, "No Content")) + }, + }, + { + name: "Dependabot GHES", + opts: &DeleteOptions{ + Application: "dependabot", + SecretName: "cool_dependabot_secret", + }, + host: "example.com", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.WithHost(httpmock.REST("DELETE", "api/v3/repos/owner/repo/dependabot/secrets/cool_dependabot_secret"), "example.com"), httpmock.StatusStringResponse(204, "No Content")) + }, }, { name: "defaults to Actions", opts: &DeleteOptions{ SecretName: "cool_secret", }, - wantPath: "repos/owner/repo/actions/secrets/cool_secret", + host: "github.com", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.WithHost(httpmock.REST("DELETE", "repos/owner/repo/actions/secrets/cool_secret"), "api.github.com"), httpmock.StatusStringResponse(204, "No Content")) + }, }, } for _, tt := range tests { reg := &httpmock.Registry{} - - reg.Register( - httpmock.REST("DELETE", tt.wantPath), - httpmock.StatusStringResponse(204, "No Content")) + tt.httpStubs(reg) + defer reg.Verify(t) ios, _, _, _ := iostreams.Test() @@ -167,44 +198,70 @@ func Test_removeRun_repo(t *testing.T) { return config.NewBlankConfig(), nil } tt.opts.BaseRepo = func() (ghrepo.Interface, error) { - return ghrepo.FromFullName("owner/repo") + return ghrepo.FromFullNameWithHost("owner/repo", tt.host) } 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", "repos/owner/repo/environments/development/secrets/cool_secret"), - httpmock.StatusStringResponse(204, "No Content")) - - ios, _, _, _ := iostreams.Test() - - opts := &DeleteOptions{ - IO: ios, - HttpClient: func() (*http.Client, error) { - return &http.Client{Transport: reg}, nil + tests := []struct { + name string + opts *DeleteOptions + host string + httpStubs func(*httpmock.Registry) + }{ + { + name: "delete environment secret", + opts: &DeleteOptions{ + SecretName: "cool_secret", + EnvName: "development", + }, + host: "github.com", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.WithHost(httpmock.REST("DELETE", "repos/owner/repo/environments/development/secrets/cool_secret"), "api.github.com"), + httpmock.StatusStringResponse(204, "No Content")) + }, }, - Config: func() (gh.Config, error) { - return config.NewBlankConfig(), nil + { + name: "delete environment secret GHES", + opts: &DeleteOptions{ + SecretName: "cool_secret", + EnvName: "development", + }, + host: "example.com", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.WithHost(httpmock.REST("DELETE", "api/v3/repos/owner/repo/environments/development/secrets/cool_secret"), "example.com"), + httpmock.StatusStringResponse(204, "No Content")) + }, }, - BaseRepo: func() (ghrepo.Interface, error) { - return ghrepo.FromFullName("owner/repo") - }, - SecretName: "cool_secret", - EnvName: "development", } - err := removeRun(opts) - assert.NoError(t, err) + for _, tt := range tests { + reg := &httpmock.Registry{} + tt.httpStubs(reg) + defer reg.Verify(t) - reg.Verify(t) + ios, _, _, _ := iostreams.Test() + tt.opts.IO = ios + tt.opts.BaseRepo = func() (ghrepo.Interface, error) { + if tt.host != "" { + return ghrepo.FromFullNameWithHost("owner/repo", tt.host) + } + 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 := removeRun(tt.opts) + require.NoError(t, err) + } } func Test_removeRun_org(t *testing.T) { diff --git a/pkg/cmd/variable/delete/delete.go b/pkg/cmd/variable/delete/delete.go index 3617f3188..d51320167 100644 --- a/pkg/cmd/variable/delete/delete.go +++ b/pkg/cmd/variable/delete/delete.go @@ -91,22 +91,24 @@ func removeRun(opts *DeleteOptions) error { } } - 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() + var path string + var host string + switch variableEntity { + case shared.Organization: + path = fmt.Sprintf("orgs/%s/actions/variables/%s", orgName, opts.VariableName) + host, _ = cfg.Authentication().DefaultHost() + case shared.Environment: + path = fmt.Sprintf("repos/%s/environments/%s/variables/%s", ghrepo.FullName(baseRepo), envName, opts.VariableName) + host = baseRepo.RepoHost() + case shared.Repository: + path = fmt.Sprintf("repos/%s/actions/variables/%s", ghrepo.FullName(baseRepo), opts.VariableName) + host = baseRepo.RepoHost() + } err = client.REST(host, "DELETE", path, nil, nil) if err != nil { diff --git a/pkg/cmd/variable/delete/delete_test.go b/pkg/cmd/variable/delete/delete_test.go index e659f96a6..d00bef8fb 100644 --- a/pkg/cmd/variable/delete/delete_test.go +++ b/pkg/cmd/variable/delete/delete_test.go @@ -13,6 +13,7 @@ import ( "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNewCmdDelete(t *testing.T) { @@ -87,16 +88,32 @@ func TestNewCmdDelete(t *testing.T) { func TestRemoveRun(t *testing.T) { tests := []struct { - name string - opts *DeleteOptions - wantPath string + name string + opts *DeleteOptions + host string + httpStubs func(*httpmock.Registry) }{ { name: "repo", opts: &DeleteOptions{ VariableName: "cool_variable", }, - wantPath: "repos/owner/repo/actions/variables/cool_variable", + host: "github.com", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.WithHost(httpmock.REST("DELETE", "repos/owner/repo/actions/variables/cool_variable"), "api.github.com"), + httpmock.StatusStringResponse(204, "No Content")) + }, + }, + { + name: "repo GHES", + opts: &DeleteOptions{ + VariableName: "cool_variable", + }, + host: "example.com", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.WithHost(httpmock.REST("DELETE", "api/v3/repos/owner/repo/actions/variables/cool_variable"), "example.com"), + httpmock.StatusStringResponse(204, "No Content")) + }, }, { name: "env", @@ -104,7 +121,23 @@ func TestRemoveRun(t *testing.T) { VariableName: "cool_variable", EnvName: "development", }, - wantPath: "repos/owner/repo/environments/development/variables/cool_variable", + host: "github.com", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.WithHost(httpmock.REST("DELETE", "repos/owner/repo/environments/development/variables/cool_variable"), "api.github.com"), + httpmock.StatusStringResponse(204, "No Content")) + }, + }, + { + name: "env GHES", + opts: &DeleteOptions{ + VariableName: "cool_variable", + EnvName: "development", + }, + host: "example.com", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.WithHost(httpmock.REST("DELETE", "api/v3/repos/owner/repo/environments/development/variables/cool_variable"), "example.com"), + httpmock.StatusStringResponse(204, "No Content")) + }, }, { name: "org", @@ -112,35 +145,34 @@ func TestRemoveRun(t *testing.T) { VariableName: "cool_variable", OrgName: "UmbrellaCorporation", }, - wantPath: "orgs/UmbrellaCorporation/actions/variables/cool_variable", + host: "github.com", + httpStubs: func(reg *httpmock.Registry) { + reg.Register(httpmock.WithHost(httpmock.REST("DELETE", "orgs/UmbrellaCorporation/actions/variables/cool_variable"), "api.github.com"), + httpmock.StatusStringResponse(204, "No Content")) + }, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { reg := &httpmock.Registry{} + tt.httpStubs(reg) defer reg.Verify(t) - 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() (gh.Config, error) { return config.NewBlankConfig(), nil } - tt.opts.BaseRepo = func() (ghrepo.Interface, error) { - return ghrepo.FromFullName("owner/repo") + return ghrepo.FromFullNameWithHost("owner/repo", tt.host) } err := removeRun(tt.opts) - assert.NoError(t, err) + require.NoError(t, err) }) } } diff --git a/pkg/cmd/variable/get/get_test.go b/pkg/cmd/variable/get/get_test.go index e85910152..82b602c9e 100644 --- a/pkg/cmd/variable/get/get_test.go +++ b/pkg/cmd/variable/get/get_test.go @@ -135,9 +135,9 @@ func Test_getRun(t *testing.T) { opts: &GetOptions{ VariableName: "VARIABLE_ONE", }, - host: "ghe.io", + host: "example.com", httpStubs: func(reg *httpmock.Registry) { - reg.Register(httpmock.WithHost(httpmock.REST("GET", "api/v3/repos/owner/repo/actions/variables/VARIABLE_ONE"), "ghe.io"), + reg.Register(httpmock.WithHost(httpmock.REST("GET", "api/v3/repos/owner/repo/actions/variables/VARIABLE_ONE"), "example.com"), httpmock.JSONResponse(shared.Variable{ Value: "repo_var", })) @@ -150,6 +150,7 @@ func Test_getRun(t *testing.T) { OrgName: "TestOrg", VariableName: "VARIABLE_ONE", }, + host: "github.com", httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", "orgs/TestOrg/actions/variables/VARIABLE_ONE"), httpmock.JSONResponse(shared.Variable{ @@ -179,9 +180,9 @@ func Test_getRun(t *testing.T) { EnvName: "Development", VariableName: "VARIABLE_ONE", }, - host: "ghe.io", + host: "example.com", httpStubs: func(reg *httpmock.Registry) { - reg.Register(httpmock.WithHost(httpmock.REST("GET", "api/v3/repos/owner/repo/environments/Development/variables/VARIABLE_ONE"), "ghe.io"), + reg.Register(httpmock.WithHost(httpmock.REST("GET", "api/v3/repos/owner/repo/environments/Development/variables/VARIABLE_ONE"), "example.com"), httpmock.JSONResponse(shared.Variable{ Value: "env_var", })) @@ -193,6 +194,7 @@ func Test_getRun(t *testing.T) { opts: &GetOptions{ VariableName: "VARIABLE_ONE", }, + host: "github.com", jsonFields: []string{"name", "value", "visibility", "updatedAt", "createdAt", "numSelectedRepos", "selectedReposURL"}, httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", "repos/owner/repo/actions/variables/VARIABLE_ONE"), @@ -225,6 +227,7 @@ func Test_getRun(t *testing.T) { opts: &GetOptions{ VariableName: "VARIABLE_ONE", }, + host: "github.com", httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", "repos/owner/repo/actions/variables/VARIABLE_ONE"), httpmock.StatusStringResponse(404, "not found"), @@ -237,6 +240,7 @@ func Test_getRun(t *testing.T) { opts: &GetOptions{ VariableName: "VARIABLE_ONE", }, + host: "github.com", httpStubs: func(reg *httpmock.Registry) { reg.Register(httpmock.REST("GET", "repos/owner/repo/actions/variables/VARIABLE_ONE"), httpmock.StatusStringResponse(400, "not found"), @@ -258,10 +262,7 @@ func Test_getRun(t *testing.T) { tt.opts.IO = ios tt.opts.BaseRepo = func() (ghrepo.Interface, error) { - if tt.host != "" { - return ghrepo.FromFullNameWithHost("owner/repo", tt.host) - } - return ghrepo.FromFullName("owner/repo") + return ghrepo.FromFullNameWithHost("owner/repo", tt.host) } tt.opts.HttpClient = func() (*http.Client, error) { return &http.Client{Transport: reg}, nil