From c088951944082eb4c474097edb1861ed07d6c856 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Fri, 2 Aug 2024 15:11:22 -0400 Subject: [PATCH] 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