From d67d65e3049b7533aecb3005158c749966ae72cd Mon Sep 17 00:00:00 2001 From: Azeem Sajid Date: Sun, 2 Mar 2025 11:25:31 +0500 Subject: [PATCH 1/5] [gh secret] Check `GH_REPO` too in addition to `--repo` for disambiguation --- pkg/cmd/secret/delete/delete.go | 6 ++++-- pkg/cmd/secret/list/list.go | 6 ++++-- pkg/cmd/secret/set/set.go | 5 +++-- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/secret/delete/delete.go b/pkg/cmd/secret/delete/delete.go index 84a5e7a83..1cbfbca9d 100644 --- a/pkg/cmd/secret/delete/delete.go +++ b/pkg/cmd/secret/delete/delete.go @@ -3,6 +3,7 @@ package delete import ( "fmt" "net/http" + "os" "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" @@ -48,8 +49,9 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co RunE: func(cmd *cobra.Command, args []string) error { // If the user specified a repo directly, then we're using the OverrideBaseRepoFunc set by EnableRepoOverride // So there's no reason to use the specialised BaseRepoFunc that requires remote disambiguation. - opts.BaseRepo = f.BaseRepo - if !cmd.Flags().Changed("repo") { + if cmd.Flags().Changed("repo") || os.Getenv("GH_REPO") != "" { + opts.BaseRepo = f.BaseRepo + } else { // If they haven't specified a repo directly, then we will wrap the BaseRepoFunc in one that errors if // there might be multiple valid remotes. opts.BaseRepo = shared.RequireNoAmbiguityBaseRepoFunc(opts.BaseRepo, f.Remotes) diff --git a/pkg/cmd/secret/list/list.go b/pkg/cmd/secret/list/list.go index af3526386..c976bd60e 100644 --- a/pkg/cmd/secret/list/list.go +++ b/pkg/cmd/secret/list/list.go @@ -3,6 +3,7 @@ package list import ( "fmt" "net/http" + "os" "slices" "strings" "time" @@ -69,8 +70,9 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman RunE: func(cmd *cobra.Command, args []string) error { // If the user specified a repo directly, then we're using the OverrideBaseRepoFunc set by EnableRepoOverride // So there's no reason to use the specialised BaseRepoFunc that requires remote disambiguation. - opts.BaseRepo = f.BaseRepo - if !cmd.Flags().Changed("repo") { + if cmd.Flags().Changed("repo") || os.Getenv("GH_REPO") != "" { + opts.BaseRepo = f.BaseRepo + } else { // If they haven't specified a repo directly, then we will wrap the BaseRepoFunc in one that errors if // there might be multiple valid remotes. opts.BaseRepo = shared.RequireNoAmbiguityBaseRepoFunc(opts.BaseRepo, f.Remotes) diff --git a/pkg/cmd/secret/set/set.go b/pkg/cmd/secret/set/set.go index 3c12b4192..89f11f1cf 100644 --- a/pkg/cmd/secret/set/set.go +++ b/pkg/cmd/secret/set/set.go @@ -106,8 +106,9 @@ func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command RunE: func(cmd *cobra.Command, args []string) error { // If the user specified a repo directly, then we're using the OverrideBaseRepoFunc set by EnableRepoOverride // So there's no reason to use the specialised BaseRepoFunc that requires remote disambiguation. - opts.BaseRepo = f.BaseRepo - if !cmd.Flags().Changed("repo") { + if cmd.Flags().Changed("repo") || os.Getenv("GH_REPO") != "" { + opts.BaseRepo = f.BaseRepo + } else { // If they haven't specified a repo directly, then we will wrap the BaseRepoFunc in one that errors if // there might be multiple valid remotes. opts.BaseRepo = shared.RequireNoAmbiguityBaseRepoFunc(opts.BaseRepo, f.Remotes) From 23ea61d325410f2cbfd61874078af4358b535ead Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 4 Mar 2025 13:39:38 -0500 Subject: [PATCH 2/5] Expand gh secret base repo tests Building on top of the work done in commands to account for GH_REPO environment variable, this commit expands existing tests around handling base repo functions to include new test scenarios. These tests fail in the same way as reported on the issue when run against `trunk` without the other branch's changes, demonstrating they will help avoid regression. --- pkg/cmd/secret/delete/delete_test.go | 17 +++++++++++++++-- pkg/cmd/secret/list/list_test.go | 16 ++++++++++++++-- pkg/cmd/secret/set/set_test.go | 17 +++++++++++++++-- 3 files changed, 44 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/secret/delete/delete_test.go b/pkg/cmd/secret/delete/delete_test.go index 377879cf2..2f6cad431 100644 --- a/pkg/cmd/secret/delete/delete_test.go +++ b/pkg/cmd/secret/delete/delete_test.go @@ -144,6 +144,7 @@ func TestNewCmdDeleteBaseRepoFuncs(t *testing.T) { tests := []struct { name string args string + env map[string]string prompterStubs func(*prompter.MockPrompter) wantRepo ghrepo.Interface wantErr error @@ -154,14 +155,22 @@ func TestNewCmdDeleteBaseRepoFuncs(t *testing.T) { wantRepo: ghrepo.New("owner", "repo"), }, { - name: "when there is no repo flag provided, and no prompting, the base func requiring no ambiguity is used", + name: "when GH_REPO env var is provided, the factory base repo func is used", + args: "SECRET_NAME", + env: map[string]string{ + "GH_REPO": "owner/repo", + }, + wantRepo: ghrepo.New("owner", "repo"), + }, + { + name: "when there is no repo flag or GH_REPO env var provided, and no prompting, the base func requiring no ambiguity is used", args: "SECRET_NAME", wantErr: shared.AmbiguousBaseRepoError{ Remotes: remotes, }, }, { - name: "when there is no repo flag provided, and can prompt, the base func resolving ambiguity is used", + name: "when there is no repo flag or GH_REPO env var provided, and can prompt, the base func resolving ambiguity is used", args: "SECRET_NAME", prompterStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect( @@ -199,6 +208,10 @@ func TestNewCmdDeleteBaseRepoFuncs(t *testing.T) { }, } + for k, v := range tt.env { + t.Setenv(k, v) + } + argv, err := shlex.Split(tt.args) assert.NoError(t, err) diff --git a/pkg/cmd/secret/list/list_test.go b/pkg/cmd/secret/list/list_test.go index 601de2572..f4ecde73e 100644 --- a/pkg/cmd/secret/list/list_test.go +++ b/pkg/cmd/secret/list/list_test.go @@ -124,6 +124,7 @@ func TestNewCmdListBaseRepoFuncs(t *testing.T) { tests := []struct { name string args string + env map[string]string prompterStubs func(*prompter.MockPrompter) wantRepo ghrepo.Interface wantErr error @@ -134,14 +135,21 @@ func TestNewCmdListBaseRepoFuncs(t *testing.T) { wantRepo: ghrepo.New("owner", "repo"), }, { - name: "when there is no repo flag provided, and no prompting, the base func requiring no ambiguity is used", + name: "when GH_REPO env var is provided, the factory base repo func is used", + env: map[string]string{ + "GH_REPO": "owner/repo", + }, + wantRepo: ghrepo.New("owner", "repo"), + }, + { + name: "when there is no repo flag or GH_REPO env var provided, and no prompting, the base func requiring no ambiguity is used", args: "", wantErr: shared.AmbiguousBaseRepoError{ Remotes: remotes, }, }, { - name: "when there is no repo flag provided, and can prompt, the base func resolving ambiguity is used", + name: "when there is no repo flag or GH_REPO env var provided, and can prompt, the base func resolving ambiguity is used", args: "", prompterStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect( @@ -179,6 +187,10 @@ func TestNewCmdListBaseRepoFuncs(t *testing.T) { }, } + for k, v := range tt.env { + t.Setenv(k, v) + } + argv, err := shlex.Split(tt.args) assert.NoError(t, err) diff --git a/pkg/cmd/secret/set/set_test.go b/pkg/cmd/secret/set/set_test.go index 99c8c6cbe..22c7aaab8 100644 --- a/pkg/cmd/secret/set/set_test.go +++ b/pkg/cmd/secret/set/set_test.go @@ -243,6 +243,7 @@ func TestNewCmdSetBaseRepoFuncs(t *testing.T) { tests := []struct { name string args string + env map[string]string prompterStubs func(*prompter.MockPrompter) wantRepo ghrepo.Interface wantErr error @@ -253,14 +254,22 @@ func TestNewCmdSetBaseRepoFuncs(t *testing.T) { wantRepo: ghrepo.New("owner", "repo"), }, { - name: "when there is no repo flag provided, and no prompting, the base func requiring no ambiguity is used", + name: "when GH_REPO env var is provided, the factory base repo func is used", + args: "SECRET_NAME", + env: map[string]string{ + "GH_REPO": "owner/repo", + }, + wantRepo: ghrepo.New("owner", "repo"), + }, + { + name: "when there is no repo flag or GH_REPO env var provided, and no prompting, the base func requiring no ambiguity is used", args: "SECRET_NAME", wantErr: shared.AmbiguousBaseRepoError{ Remotes: remotes, }, }, { - name: "when there is no repo flag provided, and can prompt, the base func resolving ambiguity is used", + name: "when there is no repo flag or GH_REPO env var provided, and can prompt, the base func resolving ambiguity is used", args: "SECRET_NAME", prompterStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect( @@ -298,6 +307,10 @@ func TestNewCmdSetBaseRepoFuncs(t *testing.T) { }, } + for k, v := range tt.env { + t.Setenv(k, v) + } + argv, err := shlex.Split(tt.args) assert.NoError(t, err) From 0da037ca8f2ddc59eb95a94afcf32331d1c8af7d Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 4 Mar 2025 14:16:00 -0500 Subject: [PATCH 3/5] Expand gh secret acceptance tests This commit builds on top of work previously done within acceptance tests around gh secret and remote disambiguation. The choice of expanding this existing test rather than creating a new test was in order to keep context together within a single acceptance test rather than splitting it over multiple. --- ...secret-require-remote-disambiguation.txtar | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar b/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar index f4d1bbb4a..ebf5c79db 100644 --- a/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar +++ b/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar @@ -34,3 +34,26 @@ stderr 'multiple remotes detected. please specify which repo to use by providing # Secret delete requires disambiguation ! exec gh secret delete 'TEST_SECRET_NAME' stderr 'multiple remotes detected. please specify which repo to use by providing the -R, --repo argument' + +# Move out of the fork repo to test whether secret commands work without local repository context +cd .. + +# Secret set using --repo flag does not require disambiguation +exec gh secret set 'TEST_SECRET_NAME' --body 'TEST_SECRET_VALUE' --repo ${ORG}/${REPO}-fork + +# Secret list using --repo flag does not require disambiguation +exec gh secret list --repo ${ORG}/${REPO}-fork +stdout 'TEST_SECRET_NAME' + +# Secret delete using --repo flag does not require disambiguation +exec gh secret delete 'TEST_SECRET_NAME' --repo ${ORG}/${REPO}-fork + +# Secret set using GH_REPO env var does not require disambiguation +exec gh secret set 'TEST_SECRET_NAME2' --body 'TEST_SECRET_VALUE2' --repo ${ORG}/${REPO}-fork + +# Secret list using --repo flag does not require disambiguation +exec gh secret list --repo ${ORG}/${REPO}-fork +stdout 'TEST_SECRET_NAME2' + +# Secret delete using --repo flag does not require disambiguation +exec gh secret delete 'TEST_SECRET_NAME2' --repo ${ORG}/${REPO}-fork From 50780fc46991a618c3f0fc7f23160c9aa750ea28 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Tue, 4 Mar 2025 14:31:12 -0500 Subject: [PATCH 4/5] Fix gh secret disambiguity test The previous commit failed to update the 2nd set of tests to rely upon GH_REPO instead of --repo flag. These have been tested against `trunk` and fail because not being in a git directory when they should pass. --- .../secret/secret-require-remote-disambiguation.txtar | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar b/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar index ebf5c79db..e6bfd1c7a 100644 --- a/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar +++ b/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar @@ -48,12 +48,15 @@ stdout 'TEST_SECRET_NAME' # Secret delete using --repo flag does not require disambiguation exec gh secret delete 'TEST_SECRET_NAME' --repo ${ORG}/${REPO}-fork +# Setup GH_REPO for testing environment variable behavior +env GH_REPO=${ORG}/${REPO}-fork + # Secret set using GH_REPO env var does not require disambiguation -exec gh secret set 'TEST_SECRET_NAME2' --body 'TEST_SECRET_VALUE2' --repo ${ORG}/${REPO}-fork +exec gh secret set 'TEST_SECRET_NAME2' --body 'TEST_SECRET_VALUE2' # Secret list using --repo flag does not require disambiguation -exec gh secret list --repo ${ORG}/${REPO}-fork +exec gh secret list stdout 'TEST_SECRET_NAME2' # Secret delete using --repo flag does not require disambiguation -exec gh secret delete 'TEST_SECRET_NAME2' --repo ${ORG}/${REPO}-fork +exec gh secret delete 'TEST_SECRET_NAME2' From a18a9594be6b221025db98ad84fa462a4640ca65 Mon Sep 17 00:00:00 2001 From: Andy Feller Date: Wed, 5 Mar 2025 08:27:50 -0500 Subject: [PATCH 5/5] Fix acceptance test descriptions --- .../secret/secret-require-remote-disambiguation.txtar | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar b/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar index e6bfd1c7a..02dec06a0 100644 --- a/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar +++ b/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar @@ -54,9 +54,9 @@ env GH_REPO=${ORG}/${REPO}-fork # Secret set using GH_REPO env var does not require disambiguation exec gh secret set 'TEST_SECRET_NAME2' --body 'TEST_SECRET_VALUE2' -# Secret list using --repo flag does not require disambiguation +# Secret list using GH_REPO env var does not require disambiguation exec gh secret list stdout 'TEST_SECRET_NAME2' -# Secret delete using --repo flag does not require disambiguation +# Secret delete using GH_REPO env var does not require disambiguation exec gh secret delete 'TEST_SECRET_NAME2'