From 88988374b51ae71952c708a4d01a814b40c564ee Mon Sep 17 00:00:00 2001 From: Wing Date: Wed, 4 Dec 2024 15:24:41 +0100 Subject: [PATCH 01/16] Add remote check to secret commands Co-authored-by: William Martin --- pkg/cmd/secret/delete/delete.go | 12 ++++++++++++ pkg/cmd/secret/list/list.go | 12 ++++++++++++ pkg/cmd/secret/set/set.go | 13 +++++++++++++ pkg/cmdutil/errors.go | 16 ++++++++++++++++ 4 files changed, 53 insertions(+) diff --git a/pkg/cmd/secret/delete/delete.go b/pkg/cmd/secret/delete/delete.go index 564e8633f..75d061e7f 100644 --- a/pkg/cmd/secret/delete/delete.go +++ b/pkg/cmd/secret/delete/delete.go @@ -6,6 +6,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" + ghContext "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmd/secret/shared" @@ -19,12 +20,15 @@ type DeleteOptions struct { IO *iostreams.IOStreams Config func() (gh.Config, error) BaseRepo func() (ghrepo.Interface, error) + Remotes func() (ghContext.Remotes, error) SecretName string OrgName string EnvName string UserSecrets bool Application string + + HasRepoOverride bool } func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Command { @@ -32,6 +36,7 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co IO: f.IOStreams, Config: f.Config, HttpClient: f.HttpClient, + Remotes: f.Remotes, } cmd := &cobra.Command{ @@ -53,6 +58,8 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co return err } + opts.HasRepoOverride = cmd.Flags().Changed("repo") + opts.SecretName = args[0] if runF != nil { @@ -103,6 +110,11 @@ func removeRun(opts *DeleteOptions) error { if err != nil { return err } + + err = cmdutil.ValidateHasOnlyOneRemote(opts.HasRepoOverride, opts.Remotes) + if err != nil { + return err + } } cfg, err := opts.Config() diff --git a/pkg/cmd/secret/list/list.go b/pkg/cmd/secret/list/list.go index 9e1fd305d..b87c31998 100644 --- a/pkg/cmd/secret/list/list.go +++ b/pkg/cmd/secret/list/list.go @@ -9,6 +9,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" + ghContext "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/tableprinter" @@ -23,6 +24,7 @@ type ListOptions struct { IO *iostreams.IOStreams Config func() (gh.Config, error) BaseRepo func() (ghrepo.Interface, error) + Remotes func() (ghContext.Remotes, error) Now func() time.Time Exporter cmdutil.Exporter @@ -30,6 +32,8 @@ type ListOptions struct { EnvName string UserSecrets bool Application string + + HasRepoOverride bool } var secretFields = []string{ @@ -47,6 +51,7 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman IO: f.IOStreams, Config: f.Config, HttpClient: f.HttpClient, + Remotes: f.Remotes, Now: time.Now, } @@ -70,6 +75,8 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman return err } + opts.HasRepoOverride = cmd.Flags().Changed("repo") + if runF != nil { return runF(opts) } @@ -101,6 +108,11 @@ func listRun(opts *ListOptions) error { if err != nil { return err } + + err = cmdutil.ValidateHasOnlyOneRemote(opts.HasRepoOverride, opts.Remotes) + if err != nil { + return err + } } secretEntity, err := shared.GetSecretEntity(orgName, envName, opts.UserSecrets) diff --git a/pkg/cmd/secret/set/set.go b/pkg/cmd/secret/set/set.go index 755bdda1f..dade133af 100644 --- a/pkg/cmd/secret/set/set.go +++ b/pkg/cmd/secret/set/set.go @@ -11,6 +11,7 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" + ghContext "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmd/secret/shared" @@ -27,6 +28,7 @@ type SetOptions struct { IO *iostreams.IOStreams Config func() (gh.Config, error) BaseRepo func() (ghrepo.Interface, error) + Remotes func() (ghContext.Remotes, error) Prompter iprompter RandomOverride func() io.Reader @@ -41,6 +43,8 @@ type SetOptions struct { RepositoryNames []string EnvFile string Application string + + HasRepoOverride bool } type iprompter interface { @@ -52,6 +56,7 @@ func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command IO: f.IOStreams, Config: f.Config, HttpClient: f.HttpClient, + Remotes: f.Remotes, Prompter: f.Prompter, } @@ -144,6 +149,8 @@ func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command } } + opts.HasRepoOverride = cmd.Flags().Changed("repo") + if runF != nil { return runF(opts) } @@ -187,6 +194,12 @@ func setRun(opts *SetOptions) error { if err != nil { return err } + + err = cmdutil.ValidateHasOnlyOneRemote(opts.HasRepoOverride, opts.Remotes) + if err != nil { + return err + } + host = baseRepo.RepoHost() } else { cfg, err := opts.Config() diff --git a/pkg/cmdutil/errors.go b/pkg/cmdutil/errors.go index edb97abcd..389a40d25 100644 --- a/pkg/cmdutil/errors.go +++ b/pkg/cmdutil/errors.go @@ -5,6 +5,7 @@ import ( "fmt" "github.com/AlecAivazis/survey/v2/terminal" + ghContext "github.com/cli/cli/v2/context" ) // FlagErrorf returns a new FlagError that wraps an error produced by @@ -57,6 +58,21 @@ func MutuallyExclusive(message string, conditions ...bool) error { return nil } +func ValidateHasOnlyOneRemote(hasRepoOverride bool, remotes func() (ghContext.Remotes, error)) error { + if !hasRepoOverride { + remotes, err := remotes() + if err != nil { + return err + } + + if remotes.Len() > 1 { + return fmt.Errorf("multiple remotes detected %v. please specify which repo to use by providing the -R or --repo argument", remotes) + } + } + + return nil +} + type NoResultsError struct { message string } From 57c9ee0ad2257d4b68a90764cb2d973f4e04dcbc Mon Sep 17 00:00:00 2001 From: Wing Date: Wed, 4 Dec 2024 15:25:47 +0100 Subject: [PATCH 02/16] Add tests for secret commands Co-authored-by: William Martin --- pkg/cmd/secret/delete/delete_test.go | 118 +++++++++++++++++++ pkg/cmd/secret/list/list_test.go | 169 +++++++++++++++++++++++++++ pkg/cmd/secret/set/set_test.go | 139 ++++++++++++++++++++++ pkg/cmdutil/errors.go | 2 +- 4 files changed, 427 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/secret/delete/delete_test.go b/pkg/cmd/secret/delete/delete_test.go index bd5053f4d..45091a3b8 100644 --- a/pkg/cmd/secret/delete/delete_test.go +++ b/pkg/cmd/secret/delete/delete_test.go @@ -5,6 +5,8 @@ import ( "net/http" "testing" + ghContext "github.com/cli/cli/v2/context" + "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" @@ -351,3 +353,119 @@ func Test_removeRun_user(t *testing.T) { reg.Verify(t) } + +func Test_removeRun_remote_validation(t *testing.T) { + tests := []struct { + name string + opts *DeleteOptions + wantPath string + wantErr bool + errMsg string + }{ + { + name: "single repo detected", + opts: &DeleteOptions{ + Application: "actions", + SecretName: "cool_secret", + Remotes: func() (ghContext.Remotes, error) { + remote := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "repo"), + } + + return ghContext.Remotes{ + remote, + }, nil + }}, + wantPath: "repos/owner/repo/actions/secrets/cool_secret", + }, + { + name: "multi repo detected", + opts: &DeleteOptions{ + Application: "actions", + SecretName: "cool_secret", + Remotes: func() (ghContext.Remotes, error) { + remote := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "repo"), + } + remote2 := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "upstream", + }, + Repo: ghrepo.New("owner", "repo"), + } + + return ghContext.Remotes{ + remote, + remote2, + }, nil + }}, + wantErr: true, + errMsg: "multiple remotes detected [origin upstream]. please specify which repo to use by providing the -R or --repo argument", + }, + { + name: "multi repo detected - single repo given", + opts: &DeleteOptions{ + Application: "actions", + SecretName: "cool_secret", + HasRepoOverride: true, + Remotes: func() (ghContext.Remotes, error) { + remote := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "repo"), + } + remote2 := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "upstream", + }, + Repo: ghrepo.New("owner", "repo"), + } + + return ghContext.Remotes{ + remote, + remote2, + }, nil + }}, + wantPath: "repos/owner/repo/actions/secrets/cool_secret", + }, + } + + for _, tt := range tests { + reg := &httpmock.Registry{} + + if 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() (gh.Config, error) { + return config.NewBlankConfig(), nil + } + tt.opts.BaseRepo = func() (ghrepo.Interface, error) { + return ghrepo.FromFullName("owner/repo") + } + + err := removeRun(tt.opts) + if tt.wantErr { + assert.EqualError(t, err, tt.errMsg) + } else { + assert.NoError(t, err) + } + + reg.Verify(t) + } +} diff --git a/pkg/cmd/secret/list/list_test.go b/pkg/cmd/secret/list/list_test.go index 2e7b19065..a66b913ea 100644 --- a/pkg/cmd/secret/list/list_test.go +++ b/pkg/cmd/secret/list/list_test.go @@ -9,6 +9,8 @@ import ( "testing" "time" + ghContext "github.com/cli/cli/v2/context" + "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" @@ -442,6 +444,173 @@ func Test_listRun(t *testing.T) { } } +func Test_listRunRemoteValidation(t *testing.T) { + tests := []struct { + name string + tty bool + json bool + opts *ListOptions + wantOut []string + wantErr bool + errMsg string + }{ + { + name: "single repo detected", + tty: false, + opts: &ListOptions{ + Remotes: func() (ghContext.Remotes, error) { + remote := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "repo"), + } + + return ghContext.Remotes{ + remote, + }, nil + }, + }, + wantOut: []string{ + "SECRET_ONE\t1988-10-11T00:00:00Z", + "SECRET_TWO\t2020-12-04T00:00:00Z", + "SECRET_THREE\t1975-11-30T00:00:00Z", + }, + }, + { + name: "multi repo detected", + tty: false, + opts: &ListOptions{ + Remotes: func() (ghContext.Remotes, error) { + remote := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "repo"), + } + remote2 := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "upstream", + }, + Repo: ghrepo.New("owner", "repo"), + } + + return ghContext.Remotes{ + remote, + remote2, + }, nil + }, + }, + wantOut: []string{}, + wantErr: true, + errMsg: "multiple remotes detected [origin upstream]. please specify which repo to use by providing the -R or --repo argument", + }, + { + name: "multi repo detected - single repo given", + tty: false, + opts: &ListOptions{ + HasRepoOverride: true, + Remotes: func() (ghContext.Remotes, error) { + remote := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "repo"), + } + remote2 := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "upstream", + }, + Repo: ghrepo.New("owner", "repo"), + } + + return ghContext.Remotes{ + remote, + remote2, + }, nil + }, + }, + wantOut: []string{ + "SECRET_ONE\t1988-10-11T00:00:00Z", + "SECRET_TWO\t2020-12-04T00:00:00Z", + "SECRET_THREE\t1975-11-30T00:00:00Z", + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + reg.Verify(t) + + path := "repos/owner/repo/actions/secrets" + + 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 := struct { + Secrets []Secret + }{ + Secrets: []Secret{ + { + Name: "SECRET_ONE", + UpdatedAt: t0, + }, + { + Name: "SECRET_TWO", + UpdatedAt: t1, + }, + { + Name: "SECRET_THREE", + UpdatedAt: t2, + }, + }, + } + + 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 + } + tt.opts.Now = func() time.Time { + t, _ := time.Parse(time.RFC822, "15 Mar 23 00:00 UTC") + return t + } + + if tt.json { + exporter := cmdutil.NewJSONExporter() + exporter.SetFields(secretFields) + tt.opts.Exporter = exporter + } + + err := listRun(tt.opts) + if tt.wantErr { + assert.EqualError(t, err, tt.errMsg) + + return + } + + assert.NoError(t, err) + + if len(tt.wantOut) > 1 { + expected := fmt.Sprintf("%s\n", strings.Join(tt.wantOut, "\n")) + assert.Equal(t, expected, stdout.String()) + } + }) + } +} + // Test_listRun_populatesNumSelectedReposIfRequired asserts that NumSelectedRepos // field is populated **only** when it's going to be presented in the output. Since // populating this field costs further API requests (one per secret), it's important diff --git a/pkg/cmd/secret/set/set_test.go b/pkg/cmd/secret/set/set_test.go index 5c8d6f693..68c7624dd 100644 --- a/pkg/cmd/secret/set/set_test.go +++ b/pkg/cmd/secret/set/set_test.go @@ -10,6 +10,8 @@ import ( "testing" "github.com/MakeNowJust/heredoc" + ghContext "github.com/cli/cli/v2/context" + "github.com/cli/cli/v2/git" "github.com/cli/cli/v2/internal/config" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" @@ -700,6 +702,143 @@ func Test_getSecretsFromOptions(t *testing.T) { } } +func Test_setRun_remote_validation(t *testing.T) { + tests := []struct { + name string + opts *SetOptions + wantApp string + wantErr bool + errMsg string + }{ + { + name: "single repo detected", + opts: &SetOptions{ + Application: "actions", + Remotes: func() (ghContext.Remotes, error) { + remote := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "repo"), + } + + return ghContext.Remotes{ + remote, + }, nil + }, + }, + wantApp: "actions", + }, + { + name: "multi repo detected", + opts: &SetOptions{ + Application: "actions", + Remotes: func() (ghContext.Remotes, error) { + remote := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "repo"), + } + remote2 := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "upstream", + }, + Repo: ghrepo.New("owner", "repo"), + } + + return ghContext.Remotes{ + remote, + remote2, + }, nil + }, + }, + wantErr: true, + errMsg: "multiple remotes detected [origin upstream]. please specify which repo to use by providing the -R or --repo argument", + }, + { + name: "multi repo detected - single repo given", + opts: &SetOptions{ + Application: "actions", + HasRepoOverride: true, + Remotes: func() (ghContext.Remotes, error) { + remote := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "repo"), + } + remote2 := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "upstream", + }, + Repo: ghrepo.New("owner", "repo"), + } + + return ghContext.Remotes{ + remote, + remote2, + }, nil + }, + }, + wantApp: "actions", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + reg := &httpmock.Registry{} + + if tt.wantApp != "" { + reg.Register(httpmock.REST("GET", fmt.Sprintf("repos/owner/repo/%s/secrets/public-key", tt.wantApp)), + httpmock.JSONResponse(PubKey{ID: "123", Key: "CDjXqf7AJBXWhMczcy+Fs7JlACEptgceysutztHaFQI="})) + + reg.Register(httpmock.REST("PUT", fmt.Sprintf("repos/owner/repo/%s/secrets/cool_secret", tt.wantApp)), + httpmock.StatusStringResponse(201, `{}`)) + } + + ios, _, _, _ := iostreams.Test() + + opts := &SetOptions{ + HttpClient: func() (*http.Client, error) { + return &http.Client{Transport: reg}, nil + }, + Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.FromFullName("owner/repo") + }, + IO: ios, + SecretName: "cool_secret", + Body: "a secret", + RandomOverride: fakeRandom, + Application: tt.opts.Application, + HasRepoOverride: tt.opts.HasRepoOverride, + Remotes: tt.opts.Remotes, + } + + err := setRun(opts) + if tt.wantErr { + assert.EqualError(t, err, tt.errMsg) + } else { + assert.NoError(t, err) + } + + reg.Verify(t) + + if tt.wantApp != "" && !tt.wantErr { + data, err := io.ReadAll(reg.Requests[1].Body) + assert.NoError(t, err) + + var payload SecretPayload + err = json.Unmarshal(data, &payload) + assert.NoError(t, err) + assert.Equal(t, payload.KeyID, "123") + assert.Equal(t, payload.EncryptedValue, "UKYUCbHd0DJemxa3AOcZ6XcsBwALG9d4bpB8ZT0gSV39vl3BHiGSgj8zJapDxgB2BwqNqRhpjC4=") + } + }) + } +} + func fakeRandom() io.Reader { return bytes.NewReader(bytes.Repeat([]byte{5}, 32)) } diff --git a/pkg/cmdutil/errors.go b/pkg/cmdutil/errors.go index 389a40d25..e1a37d0c8 100644 --- a/pkg/cmdutil/errors.go +++ b/pkg/cmdutil/errors.go @@ -59,7 +59,7 @@ func MutuallyExclusive(message string, conditions ...bool) error { } func ValidateHasOnlyOneRemote(hasRepoOverride bool, remotes func() (ghContext.Remotes, error)) error { - if !hasRepoOverride { + if !hasRepoOverride && remotes != nil { remotes, err := remotes() if err != nil { return err From 3de2fd94b3caea368c5bd9d0273c0eb47ffc7091 Mon Sep 17 00:00:00 2001 From: Wing Date: Wed, 4 Dec 2024 15:27:01 +0100 Subject: [PATCH 03/16] Prompt for secret commands Co-authored-by: William Martin --- pkg/cmd/secret/set/set.go | 29 +++++++++++++++++++++++------ pkg/cmdutil/flags.go | 8 ++++++++ pkg/cmdutil/repo_override.go | 33 +++++++++++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/secret/set/set.go b/pkg/cmd/secret/set/set.go index dade133af..9f3797008 100644 --- a/pkg/cmd/secret/set/set.go +++ b/pkg/cmd/secret/set/set.go @@ -4,6 +4,7 @@ import ( "bytes" "encoding/base64" "fmt" + "github.com/cli/cli/v2/internal/prompter" "io" "net/http" "os" @@ -29,7 +30,7 @@ type SetOptions struct { Config func() (gh.Config, error) BaseRepo func() (ghrepo.Interface, error) Remotes func() (ghContext.Remotes, error) - Prompter iprompter + Prompter prompter.Prompter RandomOverride func() io.Reader @@ -45,10 +46,7 @@ type SetOptions struct { Application string HasRepoOverride bool -} - -type iprompter interface { - Password(string) (string, error) + Interactive bool } func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command { @@ -82,6 +80,9 @@ func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command # Read secret value from an environment variable $ gh secret set MYSECRET --body "$ENV_VALUE" + # Set secret for a specific remote repository + $ gh secret set MYSECRET --repo origin/repo --body "$ENV_VALUE" + # Read secret value from a file $ gh secret set MYSECRET < myfile.txt @@ -111,6 +112,8 @@ func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command // support `-R, --repo` override opts.BaseRepo = f.BaseRepo + flagCount := cmdutil.CountSetFlags(cmd.Flags()) + if err := cmdutil.MutuallyExclusive("specify only one of `--org`, `--env`, or `--user`", opts.OrgName != "", opts.EnvName != "", opts.UserSecrets); err != nil { return err } @@ -129,6 +132,10 @@ func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command } } else { opts.SecretName = args[0] + + if flagCount == 0 { + opts.Interactive = true + } } if cmd.Flags().Changed("visibility") { @@ -197,7 +204,17 @@ func setRun(opts *SetOptions) error { err = cmdutil.ValidateHasOnlyOneRemote(opts.HasRepoOverride, opts.Remotes) if err != nil { - return err + if opts.Interactive { + selectedRepo, errSelectedRepo := cmdutil.PromptForRepo(baseRepo, opts.Remotes, opts.Prompter) + + if errSelectedRepo != nil { + return errSelectedRepo + } + + baseRepo = selectedRepo + } else { + return err + } } host = baseRepo.RepoHost() diff --git a/pkg/cmdutil/flags.go b/pkg/cmdutil/flags.go index c0064099c..fadbc5fbe 100644 --- a/pkg/cmdutil/flags.go +++ b/pkg/cmdutil/flags.go @@ -180,3 +180,11 @@ func isIncluded(value string, opts []string) bool { } return false } + +func CountSetFlags(flags *pflag.FlagSet) int { + count := 0 + flags.Visit(func(f *pflag.Flag) { + count++ + }) + return count +} diff --git a/pkg/cmdutil/repo_override.go b/pkg/cmdutil/repo_override.go index 791dd919a..41a4a4799 100644 --- a/pkg/cmdutil/repo_override.go +++ b/pkg/cmdutil/repo_override.go @@ -1,11 +1,13 @@ package cmdutil import ( + ghContext "github.com/cli/cli/v2/context" "os" "sort" "strings" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/prompter" "github.com/spf13/cobra" ) @@ -68,3 +70,34 @@ func OverrideBaseRepoFunc(f *Factory, override string) func() (ghrepo.Interface, } return f.BaseRepo } + +func PromptForRepo(baseRepo ghrepo.Interface, remotes func() (ghContext.Remotes, error), survey prompter.Prompter) (ghrepo.Interface, error) { + var defaultRepo string + var remoteArray []string + + if remotes, _ := remotes(); remotes != nil { + if defaultRemote, _ := remotes.ResolvedRemote(); defaultRemote != nil { + // this is a remote explicitly chosen via `repo set-default` + defaultRepo = ghrepo.FullName(defaultRemote) + } else if len(remotes) > 0 { + // as a fallback, just pick the first remote + defaultRepo = ghrepo.FullName(remotes[0]) + } + + for _, remote := range remotes { + remoteArray = append(remoteArray, ghrepo.FullName(remote)) + } + } + + baseRepoInput, errInput := survey.Select("Select a base repo", defaultRepo, remoteArray) + if errInput != nil { + return baseRepo, errInput + } + + selectedRepo, errSelectedRepo := ghrepo.FromFullName(remoteArray[baseRepoInput]) + if errSelectedRepo != nil { + return baseRepo, errSelectedRepo + } + + return selectedRepo, nil +} From cad59036f5aabb62e70723fb8ac7da61b34ed719 Mon Sep 17 00:00:00 2001 From: Wing Date: Wed, 21 Aug 2024 18:16:06 +0200 Subject: [PATCH 04/16] Update docs for set-default NOTE: gh does not use the default repository for managing repository and environment secrets. --- pkg/cmd/repo/setdefault/setdefault.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/pkg/cmd/repo/setdefault/setdefault.go b/pkg/cmd/repo/setdefault/setdefault.go index eb8fcfb5a..69be625d6 100644 --- a/pkg/cmd/repo/setdefault/setdefault.go +++ b/pkg/cmd/repo/setdefault/setdefault.go @@ -30,7 +30,8 @@ func explainer() string { - viewing and creating issues - viewing and creating releases - working with GitHub Actions - - adding repository and environment secrets`) + + ### NOTE: gh does not use the default repository for managing repository and environment secrets or variables.`) } type iprompter interface { From 0be5720c1cd1ffacebead919be7091e53d1e17b6 Mon Sep 17 00:00:00 2001 From: Wing Date: Wed, 21 Aug 2024 18:31:27 +0200 Subject: [PATCH 05/16] Update setdefault test --- pkg/cmd/repo/setdefault/setdefault.go | 2 +- pkg/cmd/repo/setdefault/setdefault_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/repo/setdefault/setdefault.go b/pkg/cmd/repo/setdefault/setdefault.go index 69be625d6..3e1554a69 100644 --- a/pkg/cmd/repo/setdefault/setdefault.go +++ b/pkg/cmd/repo/setdefault/setdefault.go @@ -31,7 +31,7 @@ func explainer() string { - viewing and creating releases - working with GitHub Actions - ### NOTE: gh does not use the default repository for managing repository and environment secrets or variables.`) + ### NOTE: gh does not use the default repository for managing repository and environment secrets.`) } type iprompter interface { diff --git a/pkg/cmd/repo/setdefault/setdefault_test.go b/pkg/cmd/repo/setdefault/setdefault_test.go index 55a0193d5..dfd72d83e 100644 --- a/pkg/cmd/repo/setdefault/setdefault_test.go +++ b/pkg/cmd/repo/setdefault/setdefault_test.go @@ -382,7 +382,7 @@ func TestDefaultRun(t *testing.T) { } } }, - wantStdout: "This command sets the default remote repository to use when querying the\nGitHub API for the locally cloned repository.\n\ngh uses the default repository for things like:\n\n - viewing and creating pull requests\n - viewing and creating issues\n - viewing and creating releases\n - working with GitHub Actions\n - adding repository and environment secrets\n\n✓ Set OWNER2/REPO2 as the default repository for the current directory\n", + wantStdout: "This command sets the default remote repository to use when querying the\nGitHub API for the locally cloned repository.\n\ngh uses the default repository for things like:\n\n - viewing and creating pull requests\n - viewing and creating issues\n - viewing and creating releases\n - working with GitHub Actions\n\n### NOTE: gh does not use the default repository for managing repository and environment secrets.\n\n✓ Set OWNER2/REPO2 as the default repository for the current directory\n", }, { name: "interactive mode only one known host", @@ -456,7 +456,7 @@ func TestDefaultRun(t *testing.T) { } } }, - wantStdout: "This command sets the default remote repository to use when querying the\nGitHub API for the locally cloned repository.\n\ngh uses the default repository for things like:\n\n - viewing and creating pull requests\n - viewing and creating issues\n - viewing and creating releases\n - working with GitHub Actions\n - adding repository and environment secrets\n\n✓ Set OWNER2/REPO2 as the default repository for the current directory\n", + wantStdout: "This command sets the default remote repository to use when querying the\nGitHub API for the locally cloned repository.\n\ngh uses the default repository for things like:\n\n - viewing and creating pull requests\n - viewing and creating issues\n - viewing and creating releases\n - working with GitHub Actions\n\n### NOTE: gh does not use the default repository for managing repository and environment secrets.\n\n✓ Set OWNER2/REPO2 as the default repository for the current directory\n", }, } From df8bb51c9cf92ac3ae72fb914799e004f54d6b10 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 9 Dec 2024 15:59:01 +0100 Subject: [PATCH 06/16] Always prompt on secret set when multiple remotes --- pkg/cmd/secret/set/set.go | 32 +++++++++++++------------------- 1 file changed, 13 insertions(+), 19 deletions(-) diff --git a/pkg/cmd/secret/set/set.go b/pkg/cmd/secret/set/set.go index 9f3797008..74bd7b33c 100644 --- a/pkg/cmd/secret/set/set.go +++ b/pkg/cmd/secret/set/set.go @@ -4,12 +4,13 @@ import ( "bytes" "encoding/base64" "fmt" - "github.com/cli/cli/v2/internal/prompter" "io" "net/http" "os" "strings" + "github.com/cli/cli/v2/internal/prompter" + "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" ghContext "github.com/cli/cli/v2/context" @@ -46,7 +47,7 @@ type SetOptions struct { Application string HasRepoOverride bool - Interactive bool + CanPrompt bool } func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command { @@ -112,8 +113,6 @@ func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command // support `-R, --repo` override opts.BaseRepo = f.BaseRepo - flagCount := cmdutil.CountSetFlags(cmd.Flags()) - if err := cmdutil.MutuallyExclusive("specify only one of `--org`, `--env`, or `--user`", opts.OrgName != "", opts.EnvName != "", opts.UserSecrets); err != nil { return err } @@ -132,10 +131,6 @@ func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command } } else { opts.SecretName = args[0] - - if flagCount == 0 { - opts.Interactive = true - } } if cmd.Flags().Changed("visibility") { @@ -157,6 +152,7 @@ func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command } opts.HasRepoOverride = cmd.Flags().Changed("repo") + opts.CanPrompt = opts.IO.CanPrompt() if runF != nil { return runF(opts) @@ -202,19 +198,17 @@ func setRun(opts *SetOptions) error { return err } - err = cmdutil.ValidateHasOnlyOneRemote(opts.HasRepoOverride, opts.Remotes) - if err != nil { - if opts.Interactive { - selectedRepo, errSelectedRepo := cmdutil.PromptForRepo(baseRepo, opts.Remotes, opts.Prompter) - - if errSelectedRepo != nil { - return errSelectedRepo - } - - baseRepo = selectedRepo - } else { + if err = cmdutil.ValidateHasOnlyOneRemote(opts.HasRepoOverride, opts.Remotes); err != nil { + if !opts.CanPrompt { return err } + + selectedRepo, errSelectingRepo := cmdutil.PromptForRepo(baseRepo, opts.Remotes, opts.Prompter) + if errSelectingRepo != nil { + return errSelectingRepo + } + + baseRepo = selectedRepo } host = baseRepo.RepoHost() From 73244c010eddcb89534b090203059d1ee3dd4dd5 Mon Sep 17 00:00:00 2001 From: William Martin Date: Mon, 9 Dec 2024 16:04:41 +0100 Subject: [PATCH 07/16] Move secret repo validation into secrets subpackage --- pkg/cmd/secret/delete/delete.go | 2 +- pkg/cmd/secret/list/list.go | 2 +- pkg/cmd/secret/set/set.go | 4 +-- pkg/cmd/secret/shared/base_repo.go | 55 ++++++++++++++++++++++++++++++ pkg/cmdutil/errors.go | 16 --------- pkg/cmdutil/repo_override.go | 33 ------------------ 6 files changed, 59 insertions(+), 53 deletions(-) create mode 100644 pkg/cmd/secret/shared/base_repo.go diff --git a/pkg/cmd/secret/delete/delete.go b/pkg/cmd/secret/delete/delete.go index 75d061e7f..70aab4be5 100644 --- a/pkg/cmd/secret/delete/delete.go +++ b/pkg/cmd/secret/delete/delete.go @@ -111,7 +111,7 @@ func removeRun(opts *DeleteOptions) error { return err } - err = cmdutil.ValidateHasOnlyOneRemote(opts.HasRepoOverride, opts.Remotes) + err = shared.ValidateHasOnlyOneRemote(opts.HasRepoOverride, opts.Remotes) if err != nil { return err } diff --git a/pkg/cmd/secret/list/list.go b/pkg/cmd/secret/list/list.go index b87c31998..5c13234d0 100644 --- a/pkg/cmd/secret/list/list.go +++ b/pkg/cmd/secret/list/list.go @@ -109,7 +109,7 @@ func listRun(opts *ListOptions) error { return err } - err = cmdutil.ValidateHasOnlyOneRemote(opts.HasRepoOverride, opts.Remotes) + err = shared.ValidateHasOnlyOneRemote(opts.HasRepoOverride, opts.Remotes) if err != nil { return err } diff --git a/pkg/cmd/secret/set/set.go b/pkg/cmd/secret/set/set.go index 74bd7b33c..b4630e067 100644 --- a/pkg/cmd/secret/set/set.go +++ b/pkg/cmd/secret/set/set.go @@ -198,12 +198,12 @@ func setRun(opts *SetOptions) error { return err } - if err = cmdutil.ValidateHasOnlyOneRemote(opts.HasRepoOverride, opts.Remotes); err != nil { + if err = shared.ValidateHasOnlyOneRemote(opts.HasRepoOverride, opts.Remotes); err != nil { if !opts.CanPrompt { return err } - selectedRepo, errSelectingRepo := cmdutil.PromptForRepo(baseRepo, opts.Remotes, opts.Prompter) + selectedRepo, errSelectingRepo := shared.PromptForRepo(baseRepo, opts.Remotes, opts.Prompter) if errSelectingRepo != nil { return errSelectingRepo } diff --git a/pkg/cmd/secret/shared/base_repo.go b/pkg/cmd/secret/shared/base_repo.go new file mode 100644 index 000000000..db2dfc8f1 --- /dev/null +++ b/pkg/cmd/secret/shared/base_repo.go @@ -0,0 +1,55 @@ +package shared + +import ( + "fmt" + + ghContext "github.com/cli/cli/v2/context" + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/prompter" +) + +func ValidateHasOnlyOneRemote(hasRepoOverride bool, remotes func() (ghContext.Remotes, error)) error { + if !hasRepoOverride && remotes != nil { + remotes, err := remotes() + if err != nil { + return err + } + + if remotes.Len() > 1 { + return fmt.Errorf("multiple remotes detected %v. please specify which repo to use by providing the -R or --repo argument", remotes) + } + } + + return nil +} + +func PromptForRepo(baseRepo ghrepo.Interface, remotes func() (ghContext.Remotes, error), survey prompter.Prompter) (ghrepo.Interface, error) { + var defaultRepo string + var remoteArray []string + + if remotes, _ := remotes(); remotes != nil { + if defaultRemote, _ := remotes.ResolvedRemote(); defaultRemote != nil { + // this is a remote explicitly chosen via `repo set-default` + defaultRepo = ghrepo.FullName(defaultRemote) + } else if len(remotes) > 0 { + // as a fallback, just pick the first remote + defaultRepo = ghrepo.FullName(remotes[0]) + } + + for _, remote := range remotes { + remoteArray = append(remoteArray, ghrepo.FullName(remote)) + } + } + + baseRepoInput, errInput := survey.Select("Select a base repo", defaultRepo, remoteArray) + if errInput != nil { + return baseRepo, errInput + } + + selectedRepo, errSelectedRepo := ghrepo.FromFullName(remoteArray[baseRepoInput]) + if errSelectedRepo != nil { + return baseRepo, errSelectedRepo + } + + return selectedRepo, nil +} diff --git a/pkg/cmdutil/errors.go b/pkg/cmdutil/errors.go index e1a37d0c8..edb97abcd 100644 --- a/pkg/cmdutil/errors.go +++ b/pkg/cmdutil/errors.go @@ -5,7 +5,6 @@ import ( "fmt" "github.com/AlecAivazis/survey/v2/terminal" - ghContext "github.com/cli/cli/v2/context" ) // FlagErrorf returns a new FlagError that wraps an error produced by @@ -58,21 +57,6 @@ func MutuallyExclusive(message string, conditions ...bool) error { return nil } -func ValidateHasOnlyOneRemote(hasRepoOverride bool, remotes func() (ghContext.Remotes, error)) error { - if !hasRepoOverride && remotes != nil { - remotes, err := remotes() - if err != nil { - return err - } - - if remotes.Len() > 1 { - return fmt.Errorf("multiple remotes detected %v. please specify which repo to use by providing the -R or --repo argument", remotes) - } - } - - return nil -} - type NoResultsError struct { message string } diff --git a/pkg/cmdutil/repo_override.go b/pkg/cmdutil/repo_override.go index 41a4a4799..791dd919a 100644 --- a/pkg/cmdutil/repo_override.go +++ b/pkg/cmdutil/repo_override.go @@ -1,13 +1,11 @@ package cmdutil import ( - ghContext "github.com/cli/cli/v2/context" "os" "sort" "strings" "github.com/cli/cli/v2/internal/ghrepo" - "github.com/cli/cli/v2/internal/prompter" "github.com/spf13/cobra" ) @@ -70,34 +68,3 @@ func OverrideBaseRepoFunc(f *Factory, override string) func() (ghrepo.Interface, } return f.BaseRepo } - -func PromptForRepo(baseRepo ghrepo.Interface, remotes func() (ghContext.Remotes, error), survey prompter.Prompter) (ghrepo.Interface, error) { - var defaultRepo string - var remoteArray []string - - if remotes, _ := remotes(); remotes != nil { - if defaultRemote, _ := remotes.ResolvedRemote(); defaultRemote != nil { - // this is a remote explicitly chosen via `repo set-default` - defaultRepo = ghrepo.FullName(defaultRemote) - } else if len(remotes) > 0 { - // as a fallback, just pick the first remote - defaultRepo = ghrepo.FullName(remotes[0]) - } - - for _, remote := range remotes { - remoteArray = append(remoteArray, ghrepo.FullName(remote)) - } - } - - baseRepoInput, errInput := survey.Select("Select a base repo", defaultRepo, remoteArray) - if errInput != nil { - return baseRepo, errInput - } - - selectedRepo, errSelectedRepo := ghrepo.FromFullName(remoteArray[baseRepoInput]) - if errSelectedRepo != nil { - return baseRepo, errSelectedRepo - } - - return selectedRepo, nil -} From 870da79886e418e7c095247653440635edc0fd99 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 18 Dec 2024 13:36:53 +0100 Subject: [PATCH 08/16] Use smarter base repo funcs for secret commands --- pkg/cmd/secret/delete/delete.go | 25 ++- pkg/cmd/secret/delete/delete_test.go | 221 +++++++++---------- pkg/cmd/secret/list/list.go | 33 +-- pkg/cmd/secret/list/list_test.go | 271 +++++++++--------------- pkg/cmd/secret/set/set.go | 35 ++- pkg/cmd/secret/set/set_test.go | 240 +++++++++------------ pkg/cmd/secret/shared/base_repo.go | 75 +++++-- pkg/cmd/secret/shared/base_repo_test.go | 239 +++++++++++++++++++++ pkg/cmdutil/flags.go | 8 - 9 files changed, 647 insertions(+), 500 deletions(-) create mode 100644 pkg/cmd/secret/shared/base_repo_test.go diff --git a/pkg/cmd/secret/delete/delete.go b/pkg/cmd/secret/delete/delete.go index 70aab4be5..64b02c9d9 100644 --- a/pkg/cmd/secret/delete/delete.go +++ b/pkg/cmd/secret/delete/delete.go @@ -6,7 +6,6 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" - ghContext "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmd/secret/shared" @@ -20,15 +19,12 @@ type DeleteOptions struct { IO *iostreams.IOStreams Config func() (gh.Config, error) BaseRepo func() (ghrepo.Interface, error) - Remotes func() (ghContext.Remotes, error) SecretName string OrgName string EnvName string UserSecrets bool Application string - - HasRepoOverride bool } func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Command { @@ -36,7 +32,6 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co IO: f.IOStreams, Config: f.Config, HttpClient: f.HttpClient, - Remotes: f.Remotes, } cmd := &cobra.Command{ @@ -51,15 +46,24 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co `), Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override + // 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 they haven't specified a repo directly, then we will wrap the BaseRepoFunc in one that error if + // there might be multiple valid remotes. + opts.BaseRepo = shared.RequireNoAmbiguityBaseRepoFunc(opts.BaseRepo, f.Remotes) + // But if we are able to prompt, then we will wrap that up in a BaseRepoFunc that can prompt the user to + // resolve the ambiguity. + if opts.IO.CanPrompt() { + opts.BaseRepo = shared.PromptWhenMultipleRemotesBaseRepoFunc(opts.BaseRepo, f.Prompter) + } + } if err := cmdutil.MutuallyExclusive("specify only one of `--org`, `--env`, or `--user`", opts.OrgName != "", opts.EnvName != "", opts.UserSecrets); err != nil { return err } - opts.HasRepoOverride = cmd.Flags().Changed("repo") - opts.SecretName = args[0] if runF != nil { @@ -110,11 +114,6 @@ func removeRun(opts *DeleteOptions) error { if err != nil { return err } - - err = shared.ValidateHasOnlyOneRemote(opts.HasRepoOverride, opts.Remotes) - if err != nil { - return err - } } cfg, err := opts.Config() diff --git a/pkg/cmd/secret/delete/delete_test.go b/pkg/cmd/secret/delete/delete_test.go index 45091a3b8..91d688a89 100644 --- a/pkg/cmd/secret/delete/delete_test.go +++ b/pkg/cmd/secret/delete/delete_test.go @@ -2,6 +2,7 @@ package delete import ( "bytes" + "io" "net/http" "testing" @@ -10,6 +11,8 @@ import ( "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/internal/prompter" + "github.com/cli/cli/v2/pkg/cmd/secret/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" "github.com/cli/cli/v2/pkg/iostreams" @@ -122,6 +125,108 @@ func TestNewCmdDelete(t *testing.T) { } } +func TestNewCmdDeleteBaseRepoFuncs(t *testing.T) { + remotes := ghContext.Remotes{ + &ghContext.Remote{ + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "fork"), + }, + &ghContext.Remote{ + Remote: &git.Remote{ + Name: "upstream", + }, + Repo: ghrepo.New("owner", "repo"), + }, + } + + tests := []struct { + name string + args string + prompterStubs func(*prompter.MockPrompter) + wantRepo ghrepo.Interface + wantErr error + }{ + { + name: "when there is a repo flag provided, the factory base repo func is used", + args: "SECRET_NAME --repo owner/repo", + wantRepo: ghrepo.New("owner", "repo"), + }, + { + name: "when there is no repo flag provided, and no prompting, the base func requiring no ambiguity is used", + args: "SECRET_NAME", + wantErr: shared.MultipleRemotesError{ + Remotes: remotes, + }, + }, + { + name: "when there is no repo flag provided, and can prompt, the base func resolving ambiguity is used", + args: "SECRET_NAME", + prompterStubs: func(pm *prompter.MockPrompter) { + pm.RegisterSelect( + "Select a base repo", + []string{"owner/fork", "owner/repo"}, + func(_, _ string, opts []string) (int, error) { + return prompter.IndexFor(opts, "owner/fork") + }, + ) + }, + wantRepo: ghrepo.New("owner", "fork"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + var pm *prompter.MockPrompter + if tt.prompterStubs != nil { + ios.SetStdinTTY(true) + ios.SetStdoutTTY(true) + ios.SetStderrTTY(true) + pm = prompter.NewMockPrompter(t) + tt.prompterStubs(pm) + } + + f := &cmdutil.Factory{ + IOStreams: ios, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.FromFullName("owner/repo") + }, + Prompter: pm, + Remotes: func() (ghContext.Remotes, error) { + return remotes, nil + }, + } + + argv, err := shlex.Split(tt.args) + assert.NoError(t, err) + + var gotOpts *DeleteOptions + cmd := NewCmdDelete(f, func(opts *DeleteOptions) error { + gotOpts = opts + return nil + }) + // Require to support --repo flag + cmdutil.EnableRepoOverride(cmd, f) + cmd.SetArgs(argv) + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + _, err = cmd.ExecuteC() + require.NoError(t, err) + + baseRepo, err := gotOpts.BaseRepo() + if tt.wantErr != nil { + require.Equal(t, tt.wantErr, err) + return + } + require.True(t, ghrepo.IsSame(tt.wantRepo, baseRepo)) + }) + } +} + func Test_removeRun_repo(t *testing.T) { tests := []struct { name string @@ -353,119 +458,3 @@ func Test_removeRun_user(t *testing.T) { reg.Verify(t) } - -func Test_removeRun_remote_validation(t *testing.T) { - tests := []struct { - name string - opts *DeleteOptions - wantPath string - wantErr bool - errMsg string - }{ - { - name: "single repo detected", - opts: &DeleteOptions{ - Application: "actions", - SecretName: "cool_secret", - Remotes: func() (ghContext.Remotes, error) { - remote := &ghContext.Remote{ - Remote: &git.Remote{ - Name: "origin", - }, - Repo: ghrepo.New("owner", "repo"), - } - - return ghContext.Remotes{ - remote, - }, nil - }}, - wantPath: "repos/owner/repo/actions/secrets/cool_secret", - }, - { - name: "multi repo detected", - opts: &DeleteOptions{ - Application: "actions", - SecretName: "cool_secret", - Remotes: func() (ghContext.Remotes, error) { - remote := &ghContext.Remote{ - Remote: &git.Remote{ - Name: "origin", - }, - Repo: ghrepo.New("owner", "repo"), - } - remote2 := &ghContext.Remote{ - Remote: &git.Remote{ - Name: "upstream", - }, - Repo: ghrepo.New("owner", "repo"), - } - - return ghContext.Remotes{ - remote, - remote2, - }, nil - }}, - wantErr: true, - errMsg: "multiple remotes detected [origin upstream]. please specify which repo to use by providing the -R or --repo argument", - }, - { - name: "multi repo detected - single repo given", - opts: &DeleteOptions{ - Application: "actions", - SecretName: "cool_secret", - HasRepoOverride: true, - Remotes: func() (ghContext.Remotes, error) { - remote := &ghContext.Remote{ - Remote: &git.Remote{ - Name: "origin", - }, - Repo: ghrepo.New("owner", "repo"), - } - remote2 := &ghContext.Remote{ - Remote: &git.Remote{ - Name: "upstream", - }, - Repo: ghrepo.New("owner", "repo"), - } - - return ghContext.Remotes{ - remote, - remote2, - }, nil - }}, - wantPath: "repos/owner/repo/actions/secrets/cool_secret", - }, - } - - for _, tt := range tests { - reg := &httpmock.Registry{} - - if 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() (gh.Config, error) { - return config.NewBlankConfig(), nil - } - tt.opts.BaseRepo = func() (ghrepo.Interface, error) { - return ghrepo.FromFullName("owner/repo") - } - - err := removeRun(tt.opts) - if tt.wantErr { - assert.EqualError(t, err, tt.errMsg) - } else { - assert.NoError(t, err) - } - - reg.Verify(t) - } -} diff --git a/pkg/cmd/secret/list/list.go b/pkg/cmd/secret/list/list.go index 5c13234d0..49ad47aff 100644 --- a/pkg/cmd/secret/list/list.go +++ b/pkg/cmd/secret/list/list.go @@ -9,9 +9,9 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" - ghContext "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/prompter" "github.com/cli/cli/v2/internal/tableprinter" "github.com/cli/cli/v2/pkg/cmd/secret/shared" "github.com/cli/cli/v2/pkg/cmdutil" @@ -24,16 +24,15 @@ type ListOptions struct { IO *iostreams.IOStreams Config func() (gh.Config, error) BaseRepo func() (ghrepo.Interface, error) - Remotes func() (ghContext.Remotes, error) - Now func() time.Time - Exporter cmdutil.Exporter + Prompter prompter.Prompter + + Now func() time.Time + Exporter cmdutil.Exporter OrgName string EnvName string UserSecrets bool Application string - - HasRepoOverride bool } var secretFields = []string{ @@ -51,8 +50,8 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman IO: f.IOStreams, Config: f.Config, HttpClient: f.HttpClient, - Remotes: f.Remotes, Now: time.Now, + Prompter: f.Prompter, } cmd := &cobra.Command{ @@ -68,15 +67,24 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman Aliases: []string{"ls"}, Args: cobra.NoArgs, RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override + // 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 they haven't specified a repo directly, then we will wrap the BaseRepoFunc in one that error if + // there might be multiple valid remotes. + opts.BaseRepo = shared.RequireNoAmbiguityBaseRepoFunc(opts.BaseRepo, f.Remotes) + // But if we are able to prompt, then we will wrap that up in a BaseRepoFunc that can prompt the user to + // resolve the ambiguity. + if opts.IO.CanPrompt() { + opts.BaseRepo = shared.PromptWhenMultipleRemotesBaseRepoFunc(opts.BaseRepo, f.Prompter) + } + } if err := cmdutil.MutuallyExclusive("specify only one of `--org`, `--env`, or `--user`", opts.OrgName != "", opts.EnvName != "", opts.UserSecrets); err != nil { return err } - opts.HasRepoOverride = cmd.Flags().Changed("repo") - if runF != nil { return runF(opts) } @@ -108,11 +116,6 @@ func listRun(opts *ListOptions) error { if err != nil { return err } - - err = shared.ValidateHasOnlyOneRemote(opts.HasRepoOverride, opts.Remotes) - if err != nil { - return err - } } secretEntity, err := shared.GetSecretEntity(orgName, envName, opts.UserSecrets) diff --git a/pkg/cmd/secret/list/list_test.go b/pkg/cmd/secret/list/list_test.go index a66b913ea..63a899b77 100644 --- a/pkg/cmd/secret/list/list_test.go +++ b/pkg/cmd/secret/list/list_test.go @@ -3,6 +3,7 @@ package list import ( "bytes" "fmt" + "io" "net/http" "net/url" "strings" @@ -14,6 +15,7 @@ import ( "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/internal/prompter" "github.com/cli/cli/v2/pkg/cmd/secret/shared" "github.com/cli/cli/v2/pkg/cmdutil" "github.com/cli/cli/v2/pkg/httpmock" @@ -103,6 +105,108 @@ func Test_NewCmdList(t *testing.T) { } } +func TestNewCmdListBaseRepoFuncs(t *testing.T) { + remotes := ghContext.Remotes{ + &ghContext.Remote{ + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "fork"), + }, + &ghContext.Remote{ + Remote: &git.Remote{ + Name: "upstream", + }, + Repo: ghrepo.New("owner", "repo"), + }, + } + + tests := []struct { + name string + args string + prompterStubs func(*prompter.MockPrompter) + wantRepo ghrepo.Interface + wantErr error + }{ + { + name: "when there is a repo flag provided, the factory base repo func is used", + args: "--repo owner/repo", + wantRepo: ghrepo.New("owner", "repo"), + }, + { + name: "when there is no repo flag provided, and no prompting, the base func requiring no ambiguity is used", + args: "", + wantErr: shared.MultipleRemotesError{ + Remotes: remotes, + }, + }, + { + name: "when there is no repo flag provided, and can prompt, the base func resolving ambiguity is used", + args: "", + prompterStubs: func(pm *prompter.MockPrompter) { + pm.RegisterSelect( + "Select a base repo", + []string{"owner/fork", "owner/repo"}, + func(_, _ string, opts []string) (int, error) { + return prompter.IndexFor(opts, "owner/fork") + }, + ) + }, + wantRepo: ghrepo.New("owner", "fork"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + var pm *prompter.MockPrompter + if tt.prompterStubs != nil { + ios.SetStdinTTY(true) + ios.SetStdoutTTY(true) + ios.SetStderrTTY(true) + pm = prompter.NewMockPrompter(t) + tt.prompterStubs(pm) + } + + f := &cmdutil.Factory{ + IOStreams: ios, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.FromFullName("owner/repo") + }, + Prompter: pm, + Remotes: func() (ghContext.Remotes, error) { + return remotes, nil + }, + } + + argv, err := shlex.Split(tt.args) + assert.NoError(t, err) + + var gotOpts *ListOptions + cmd := NewCmdList(f, func(opts *ListOptions) error { + gotOpts = opts + return nil + }) + // Require to support --repo flag + cmdutil.EnableRepoOverride(cmd, f) + cmd.SetArgs(argv) + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + _, err = cmd.ExecuteC() + require.NoError(t, err) + + baseRepo, err := gotOpts.BaseRepo() + if tt.wantErr != nil { + require.Equal(t, tt.wantErr, err) + return + } + require.True(t, ghrepo.IsSame(tt.wantRepo, baseRepo)) + }) + } +} + func Test_listRun(t *testing.T) { tests := []struct { name string @@ -444,173 +548,6 @@ func Test_listRun(t *testing.T) { } } -func Test_listRunRemoteValidation(t *testing.T) { - tests := []struct { - name string - tty bool - json bool - opts *ListOptions - wantOut []string - wantErr bool - errMsg string - }{ - { - name: "single repo detected", - tty: false, - opts: &ListOptions{ - Remotes: func() (ghContext.Remotes, error) { - remote := &ghContext.Remote{ - Remote: &git.Remote{ - Name: "origin", - }, - Repo: ghrepo.New("owner", "repo"), - } - - return ghContext.Remotes{ - remote, - }, nil - }, - }, - wantOut: []string{ - "SECRET_ONE\t1988-10-11T00:00:00Z", - "SECRET_TWO\t2020-12-04T00:00:00Z", - "SECRET_THREE\t1975-11-30T00:00:00Z", - }, - }, - { - name: "multi repo detected", - tty: false, - opts: &ListOptions{ - Remotes: func() (ghContext.Remotes, error) { - remote := &ghContext.Remote{ - Remote: &git.Remote{ - Name: "origin", - }, - Repo: ghrepo.New("owner", "repo"), - } - remote2 := &ghContext.Remote{ - Remote: &git.Remote{ - Name: "upstream", - }, - Repo: ghrepo.New("owner", "repo"), - } - - return ghContext.Remotes{ - remote, - remote2, - }, nil - }, - }, - wantOut: []string{}, - wantErr: true, - errMsg: "multiple remotes detected [origin upstream]. please specify which repo to use by providing the -R or --repo argument", - }, - { - name: "multi repo detected - single repo given", - tty: false, - opts: &ListOptions{ - HasRepoOverride: true, - Remotes: func() (ghContext.Remotes, error) { - remote := &ghContext.Remote{ - Remote: &git.Remote{ - Name: "origin", - }, - Repo: ghrepo.New("owner", "repo"), - } - remote2 := &ghContext.Remote{ - Remote: &git.Remote{ - Name: "upstream", - }, - Repo: ghrepo.New("owner", "repo"), - } - - return ghContext.Remotes{ - remote, - remote2, - }, nil - }, - }, - wantOut: []string{ - "SECRET_ONE\t1988-10-11T00:00:00Z", - "SECRET_TWO\t2020-12-04T00:00:00Z", - "SECRET_THREE\t1975-11-30T00:00:00Z", - }, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - reg := &httpmock.Registry{} - reg.Verify(t) - - path := "repos/owner/repo/actions/secrets" - - 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 := struct { - Secrets []Secret - }{ - Secrets: []Secret{ - { - Name: "SECRET_ONE", - UpdatedAt: t0, - }, - { - Name: "SECRET_TWO", - UpdatedAt: t1, - }, - { - Name: "SECRET_THREE", - UpdatedAt: t2, - }, - }, - } - - 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 - } - tt.opts.Now = func() time.Time { - t, _ := time.Parse(time.RFC822, "15 Mar 23 00:00 UTC") - return t - } - - if tt.json { - exporter := cmdutil.NewJSONExporter() - exporter.SetFields(secretFields) - tt.opts.Exporter = exporter - } - - err := listRun(tt.opts) - if tt.wantErr { - assert.EqualError(t, err, tt.errMsg) - - return - } - - assert.NoError(t, err) - - if len(tt.wantOut) > 1 { - expected := fmt.Sprintf("%s\n", strings.Join(tt.wantOut, "\n")) - assert.Equal(t, expected, stdout.String()) - } - }) - } -} - // Test_listRun_populatesNumSelectedReposIfRequired asserts that NumSelectedRepos // field is populated **only** when it's going to be presented in the output. Since // populating this field costs further API requests (one per secret), it's important diff --git a/pkg/cmd/secret/set/set.go b/pkg/cmd/secret/set/set.go index b4630e067..ba9b023a6 100644 --- a/pkg/cmd/secret/set/set.go +++ b/pkg/cmd/secret/set/set.go @@ -13,7 +13,6 @@ import ( "github.com/MakeNowJust/heredoc" "github.com/cli/cli/v2/api" - ghContext "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/internal/gh" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/pkg/cmd/secret/shared" @@ -30,7 +29,6 @@ type SetOptions struct { IO *iostreams.IOStreams Config func() (gh.Config, error) BaseRepo func() (ghrepo.Interface, error) - Remotes func() (ghContext.Remotes, error) Prompter prompter.Prompter RandomOverride func() io.Reader @@ -45,9 +43,6 @@ type SetOptions struct { RepositoryNames []string EnvFile string Application string - - HasRepoOverride bool - CanPrompt bool } func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command { @@ -55,7 +50,6 @@ func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command IO: f.IOStreams, Config: f.Config, HttpClient: f.HttpClient, - Remotes: f.Remotes, Prompter: f.Prompter, } @@ -110,8 +104,19 @@ func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command `), Args: cobra.MaximumNArgs(1), RunE: func(cmd *cobra.Command, args []string) error { - // support `-R, --repo` override + // 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 they haven't specified a repo directly, then we will wrap the BaseRepoFunc in one that error if + // there might be multiple valid remotes. + opts.BaseRepo = shared.RequireNoAmbiguityBaseRepoFunc(opts.BaseRepo, f.Remotes) + // But if we are able to prompt, then we will wrap that up in a BaseRepoFunc that can prompt the user to + // resolve the ambiguity. + if opts.IO.CanPrompt() { + opts.BaseRepo = shared.PromptWhenMultipleRemotesBaseRepoFunc(opts.BaseRepo, f.Prompter) + } + } if err := cmdutil.MutuallyExclusive("specify only one of `--org`, `--env`, or `--user`", opts.OrgName != "", opts.EnvName != "", opts.UserSecrets); err != nil { return err @@ -151,9 +156,6 @@ func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command } } - opts.HasRepoOverride = cmd.Flags().Changed("repo") - opts.CanPrompt = opts.IO.CanPrompt() - if runF != nil { return runF(opts) } @@ -198,19 +200,6 @@ func setRun(opts *SetOptions) error { return err } - if err = shared.ValidateHasOnlyOneRemote(opts.HasRepoOverride, opts.Remotes); err != nil { - if !opts.CanPrompt { - return err - } - - selectedRepo, errSelectingRepo := shared.PromptForRepo(baseRepo, opts.Remotes, opts.Prompter) - if errSelectingRepo != nil { - return errSelectingRepo - } - - baseRepo = selectedRepo - } - host = baseRepo.RepoHost() } else { cfg, err := opts.Config() diff --git a/pkg/cmd/secret/set/set_test.go b/pkg/cmd/secret/set/set_test.go index 68c7624dd..63834521f 100644 --- a/pkg/cmd/secret/set/set_test.go +++ b/pkg/cmd/secret/set/set_test.go @@ -22,6 +22,7 @@ import ( "github.com/cli/cli/v2/pkg/iostreams" "github.com/google/shlex" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestNewCmdSet(t *testing.T) { @@ -223,6 +224,108 @@ func TestNewCmdSet(t *testing.T) { } } +func TestNewCmdSetBaseRepoFuncs(t *testing.T) { + remotes := ghContext.Remotes{ + &ghContext.Remote{ + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "fork"), + }, + &ghContext.Remote{ + Remote: &git.Remote{ + Name: "upstream", + }, + Repo: ghrepo.New("owner", "repo"), + }, + } + + tests := []struct { + name string + args string + prompterStubs func(*prompter.MockPrompter) + wantRepo ghrepo.Interface + wantErr error + }{ + { + name: "when there is a repo flag provided, the factory base repo func is used", + args: "SECRET_NAME --repo owner/repo", + wantRepo: ghrepo.New("owner", "repo"), + }, + { + name: "when there is no repo flag provided, and no prompting, the base func requiring no ambiguity is used", + args: "SECRET_NAME", + wantErr: shared.MultipleRemotesError{ + Remotes: remotes, + }, + }, + { + name: "when there is no repo flag provided, and can prompt, the base func resolving ambiguity is used", + args: "SECRET_NAME", + prompterStubs: func(pm *prompter.MockPrompter) { + pm.RegisterSelect( + "Select a base repo", + []string{"owner/fork", "owner/repo"}, + func(_, _ string, opts []string) (int, error) { + return prompter.IndexFor(opts, "owner/fork") + }, + ) + }, + wantRepo: ghrepo.New("owner", "fork"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ios, _, _, _ := iostreams.Test() + var pm *prompter.MockPrompter + if tt.prompterStubs != nil { + ios.SetStdinTTY(true) + ios.SetStdoutTTY(true) + ios.SetStderrTTY(true) + pm = prompter.NewMockPrompter(t) + tt.prompterStubs(pm) + } + + f := &cmdutil.Factory{ + IOStreams: ios, + BaseRepo: func() (ghrepo.Interface, error) { + return ghrepo.FromFullName("owner/repo") + }, + Prompter: pm, + Remotes: func() (ghContext.Remotes, error) { + return remotes, nil + }, + } + + argv, err := shlex.Split(tt.args) + assert.NoError(t, err) + + var gotOpts *SetOptions + cmd := NewCmdSet(f, func(opts *SetOptions) error { + gotOpts = opts + return nil + }) + // Require to support --repo flag + cmdutil.EnableRepoOverride(cmd, f) + cmd.SetArgs(argv) + cmd.SetIn(&bytes.Buffer{}) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) + + _, err = cmd.ExecuteC() + require.NoError(t, err) + + baseRepo, err := gotOpts.BaseRepo() + if tt.wantErr != nil { + require.Equal(t, tt.wantErr, err) + return + } + require.True(t, ghrepo.IsSame(tt.wantRepo, baseRepo)) + }) + } +} + func Test_setRun_repo(t *testing.T) { tests := []struct { name string @@ -702,143 +805,6 @@ func Test_getSecretsFromOptions(t *testing.T) { } } -func Test_setRun_remote_validation(t *testing.T) { - tests := []struct { - name string - opts *SetOptions - wantApp string - wantErr bool - errMsg string - }{ - { - name: "single repo detected", - opts: &SetOptions{ - Application: "actions", - Remotes: func() (ghContext.Remotes, error) { - remote := &ghContext.Remote{ - Remote: &git.Remote{ - Name: "origin", - }, - Repo: ghrepo.New("owner", "repo"), - } - - return ghContext.Remotes{ - remote, - }, nil - }, - }, - wantApp: "actions", - }, - { - name: "multi repo detected", - opts: &SetOptions{ - Application: "actions", - Remotes: func() (ghContext.Remotes, error) { - remote := &ghContext.Remote{ - Remote: &git.Remote{ - Name: "origin", - }, - Repo: ghrepo.New("owner", "repo"), - } - remote2 := &ghContext.Remote{ - Remote: &git.Remote{ - Name: "upstream", - }, - Repo: ghrepo.New("owner", "repo"), - } - - return ghContext.Remotes{ - remote, - remote2, - }, nil - }, - }, - wantErr: true, - errMsg: "multiple remotes detected [origin upstream]. please specify which repo to use by providing the -R or --repo argument", - }, - { - name: "multi repo detected - single repo given", - opts: &SetOptions{ - Application: "actions", - HasRepoOverride: true, - Remotes: func() (ghContext.Remotes, error) { - remote := &ghContext.Remote{ - Remote: &git.Remote{ - Name: "origin", - }, - Repo: ghrepo.New("owner", "repo"), - } - remote2 := &ghContext.Remote{ - Remote: &git.Remote{ - Name: "upstream", - }, - Repo: ghrepo.New("owner", "repo"), - } - - return ghContext.Remotes{ - remote, - remote2, - }, nil - }, - }, - wantApp: "actions", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - reg := &httpmock.Registry{} - - if tt.wantApp != "" { - reg.Register(httpmock.REST("GET", fmt.Sprintf("repos/owner/repo/%s/secrets/public-key", tt.wantApp)), - httpmock.JSONResponse(PubKey{ID: "123", Key: "CDjXqf7AJBXWhMczcy+Fs7JlACEptgceysutztHaFQI="})) - - reg.Register(httpmock.REST("PUT", fmt.Sprintf("repos/owner/repo/%s/secrets/cool_secret", tt.wantApp)), - httpmock.StatusStringResponse(201, `{}`)) - } - - ios, _, _, _ := iostreams.Test() - - opts := &SetOptions{ - HttpClient: func() (*http.Client, error) { - return &http.Client{Transport: reg}, nil - }, - Config: func() (gh.Config, error) { return config.NewBlankConfig(), nil }, - BaseRepo: func() (ghrepo.Interface, error) { - return ghrepo.FromFullName("owner/repo") - }, - IO: ios, - SecretName: "cool_secret", - Body: "a secret", - RandomOverride: fakeRandom, - Application: tt.opts.Application, - HasRepoOverride: tt.opts.HasRepoOverride, - Remotes: tt.opts.Remotes, - } - - err := setRun(opts) - if tt.wantErr { - assert.EqualError(t, err, tt.errMsg) - } else { - assert.NoError(t, err) - } - - reg.Verify(t) - - if tt.wantApp != "" && !tt.wantErr { - data, err := io.ReadAll(reg.Requests[1].Body) - assert.NoError(t, err) - - var payload SecretPayload - err = json.Unmarshal(data, &payload) - assert.NoError(t, err) - assert.Equal(t, payload.KeyID, "123") - assert.Equal(t, payload.EncryptedValue, "UKYUCbHd0DJemxa3AOcZ6XcsBwALG9d4bpB8ZT0gSV39vl3BHiGSgj8zJapDxgB2BwqNqRhpjC4=") - } - }) - } -} - func fakeRandom() io.Reader { return bytes.NewReader(bytes.Repeat([]byte{5}, 32)) } diff --git a/pkg/cmd/secret/shared/base_repo.go b/pkg/cmd/secret/shared/base_repo.go index db2dfc8f1..3420f6da1 100644 --- a/pkg/cmd/secret/shared/base_repo.go +++ b/pkg/cmd/secret/shared/base_repo.go @@ -1,47 +1,80 @@ package shared import ( - "fmt" + "errors" ghContext "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/prompter" ) -func ValidateHasOnlyOneRemote(hasRepoOverride bool, remotes func() (ghContext.Remotes, error)) error { - if !hasRepoOverride && remotes != nil { +type MultipleRemotesError struct { + Remotes ghContext.Remotes +} + +func (e MultipleRemotesError) Error() string { + return "multiple remotes detected. please specify which repo to use by providing the -R or --repo argument" +} + +type baseRepoFn func() (ghrepo.Interface, error) +type remotesFn func() (ghContext.Remotes, error) + +func PromptWhenMultipleRemotesBaseRepoFunc(baseRepoFn baseRepoFn, prompter prompter.Prompter) baseRepoFn { + return func() (ghrepo.Interface, error) { + baseRepo, err := baseRepoFn() + if err != nil { + var multipleRemotesError MultipleRemotesError + if !errors.As(err, &multipleRemotesError) { + return nil, err + } + + // prompt for the base repo + baseRepo, err = promptForRepo(baseRepo, multipleRemotesError.Remotes, prompter) + if err != nil { + return nil, err + } + } + + return baseRepo, nil + } +} + +// RequireNoAmbiguityBaseRepoFunc returns a function to resolve the base repo, ensuring that +// there was only one remote. +func RequireNoAmbiguityBaseRepoFunc(baseRepo baseRepoFn, remotes remotesFn) baseRepoFn { + return func() (ghrepo.Interface, error) { + // TODO: Is this really correct? Some remotes may not be in the same network. We probably need to resolve the + // network rather than looking at the remotes? remotes, err := remotes() if err != nil { - return err + return nil, err } if remotes.Len() > 1 { - return fmt.Errorf("multiple remotes detected %v. please specify which repo to use by providing the -R or --repo argument", remotes) + return nil, MultipleRemotesError{Remotes: remotes} } - } - return nil + return baseRepo() + } } -func PromptForRepo(baseRepo ghrepo.Interface, remotes func() (ghContext.Remotes, error), survey prompter.Prompter) (ghrepo.Interface, error) { +func promptForRepo(baseRepo ghrepo.Interface, remotes ghContext.Remotes, prompter prompter.Prompter) (ghrepo.Interface, error) { var defaultRepo string var remoteArray []string - if remotes, _ := remotes(); remotes != nil { - if defaultRemote, _ := remotes.ResolvedRemote(); defaultRemote != nil { - // this is a remote explicitly chosen via `repo set-default` - defaultRepo = ghrepo.FullName(defaultRemote) - } else if len(remotes) > 0 { - // as a fallback, just pick the first remote - defaultRepo = ghrepo.FullName(remotes[0]) - } - - for _, remote := range remotes { - remoteArray = append(remoteArray, ghrepo.FullName(remote)) - } + if defaultRemote, _ := remotes.ResolvedRemote(); defaultRemote != nil { + // this is a remote explicitly chosen via `repo set-default` + defaultRepo = ghrepo.FullName(defaultRemote) + } else if len(remotes) > 0 { + // as a fallback, just pick the first remote + defaultRepo = ghrepo.FullName(remotes[0]) } - baseRepoInput, errInput := survey.Select("Select a base repo", defaultRepo, remoteArray) + for _, remote := range remotes { + remoteArray = append(remoteArray, ghrepo.FullName(remote)) + } + + baseRepoInput, errInput := prompter.Select("Select a base repo", defaultRepo, remoteArray) if errInput != nil { return baseRepo, errInput } diff --git a/pkg/cmd/secret/shared/base_repo_test.go b/pkg/cmd/secret/shared/base_repo_test.go new file mode 100644 index 000000000..64b2a3d55 --- /dev/null +++ b/pkg/cmd/secret/shared/base_repo_test.go @@ -0,0 +1,239 @@ +package shared_test + +import ( + "errors" + "testing" + + ghContext "github.com/cli/cli/v2/context" + "github.com/cli/cli/v2/pkg/cmd/secret/shared" + "github.com/stretchr/testify/require" + + "github.com/cli/cli/v2/git" + "github.com/cli/cli/v2/internal/ghrepo" + "github.com/cli/cli/v2/internal/prompter" +) + +func TestRequireNoAmbiguityBaseRepoFunc(t *testing.T) { + t.Parallel() + + t.Run("succeeds when there is only one remote", func(t *testing.T) { + t.Parallel() + + // Given there is only one remote + baseRepoFn := shared.RequireNoAmbiguityBaseRepoFunc(baseRepoStubFn, oneRemoteStubFn) + + // When fetching the base repo + baseRepo, err := baseRepoFn() + + // It succeeds and returns the inner base repo + require.NoError(t, err) + require.True(t, ghrepo.IsSame(ghrepo.New("owner", "repo"), baseRepo)) + }) + + t.Run("returns specific error when there are multiple remotes", func(t *testing.T) { + t.Parallel() + + // Given there are multiple remotes + baseRepoFn := shared.RequireNoAmbiguityBaseRepoFunc(baseRepoStubFn, twoRemotesStubFn) + + // When fetching the base repo + _, err := baseRepoFn() + + // It succeeds and returns the inner base repo + var multipleRemotesError shared.MultipleRemotesError + require.ErrorAs(t, err, &multipleRemotesError) + require.Equal(t, ghContext.Remotes{ + { + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "fork"), + }, + { + Remote: &git.Remote{ + Name: "upstream", + }, + Repo: ghrepo.New("owner", "repo"), + }, + }, multipleRemotesError.Remotes) + }) + + t.Run("when the remote fetching function fails, it returns the error", func(t *testing.T) { + t.Parallel() + + // Given the remote fetching function fails + baseRepoFn := shared.RequireNoAmbiguityBaseRepoFunc(baseRepoStubFn, errRemoteStubFn) + + // When fetching the base repo + _, err := baseRepoFn() + + // It returns the error + require.Equal(t, errors.New("test remote error"), err) + }) + + t.Run("when the wrapped base repo function fails, it returns the error", func(t *testing.T) { + t.Parallel() + + // Given the wrapped base repo function fails + baseRepoFn := shared.RequireNoAmbiguityBaseRepoFunc(errBaseRepoStubFn, oneRemoteStubFn) + + // When fetching the base repo + _, err := baseRepoFn() + + // It returns the error + require.Equal(t, errors.New("test base repo error"), err) + }) +} + +func TestPromptWhenMultipleRemotesBaseRepoFunc(t *testing.T) { + t.Parallel() + + t.Run("when there is no error from wrapped base repo func, then it succeeds without prompting", func(t *testing.T) { + t.Parallel() + + // Given the base repo function succeeds + baseRepoFn := shared.PromptWhenMultipleRemotesBaseRepoFunc(baseRepoStubFn, nil) + + // When fetching the base repo + baseRepo, err := baseRepoFn() + + // It succeeds and returns the inner base repo + require.NoError(t, err) + require.True(t, ghrepo.IsSame(ghrepo.New("owner", "repo"), baseRepo)) + }) + + t.Run("when the wrapped base repo func returns a specific error, then the prompter is used for disambiguation, with the resolved remote as the default", func(t *testing.T) { + t.Parallel() + + pm := prompter.NewMockPrompter(t) + pm.RegisterSelect( + "Select a base repo", + []string{"owner/fork", "owner/repo"}, + func(_, def string, opts []string) (int, error) { + t.Helper() + require.Equal(t, "owner/repo", def) + return prompter.IndexFor(opts, "owner/repo") + }, + ) + + // Given the wrapped base repo func returns a specific error + baseRepoFn := shared.PromptWhenMultipleRemotesBaseRepoFunc(errMultipleRemotesStubFn, pm) + + // When fetching the base repo + baseRepo, err := baseRepoFn() + + // It uses the prompter for disambiguation + require.NoError(t, err) + require.True(t, ghrepo.IsSame(ghrepo.New("owner", "repo"), baseRepo)) + }) + + t.Run("when the prompter returns an error, then it is returned", func(t *testing.T) { + t.Parallel() + + // Given the prompter returns an error + pm := prompter.NewMockPrompter(t) + pm.RegisterSelect( + "Select a base repo", + []string{"owner/fork", "owner/repo"}, + func(_, _ string, opts []string) (int, error) { + return 0, errors.New("test prompt error") + }, + ) + + // Given the wrapped base repo func returns a specific error + baseRepoFn := shared.PromptWhenMultipleRemotesBaseRepoFunc(errMultipleRemotesStubFn, pm) + + // When fetching the base repo + _, err := baseRepoFn() + + // It returns the error + require.Equal(t, errors.New("test prompt error"), err) + }) + + t.Run("when the wrapped base repo func returns a non-specific error, then it is returned", func(t *testing.T) { + t.Parallel() + + // Given the wrapped base repo func returns a non-specific error + baseRepoFn := shared.PromptWhenMultipleRemotesBaseRepoFunc(errBaseRepoStubFn, nil) + + // When fetching the base repo + _, err := baseRepoFn() + + // It returns the error + require.Equal(t, errors.New("test base repo error"), err) + }) +} + +func TestMultipleRemotesErrorMessage(t *testing.T) { + err := shared.MultipleRemotesError{} + require.EqualError(t, err, "multiple remotes detected. please specify which repo to use by providing the -R or --repo argument") +} + +func errMultipleRemotesStubFn() (ghrepo.Interface, error) { + remote1 := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "fork"), + } + + remote2 := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "upstream", + Resolved: "base", + }, + Repo: ghrepo.New("owner", "repo"), + } + + return nil, shared.MultipleRemotesError{ + Remotes: ghContext.Remotes{ + remote1, + remote2, + }, + } +} + +func baseRepoStubFn() (ghrepo.Interface, error) { + return ghrepo.New("owner", "repo"), nil +} + +func oneRemoteStubFn() (ghContext.Remotes, error) { + remote := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "repo"), + } + + return ghContext.Remotes{ + remote, + }, nil +} + +func twoRemotesStubFn() (ghContext.Remotes, error) { + remote1 := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "origin", + }, + Repo: ghrepo.New("owner", "fork"), + } + + remote2 := &ghContext.Remote{ + Remote: &git.Remote{ + Name: "upstream", + }, + Repo: ghrepo.New("owner", "repo"), + } + return ghContext.Remotes{ + remote1, + remote2, + }, nil +} + +func errRemoteStubFn() (ghContext.Remotes, error) { + return nil, errors.New("test remote error") +} + +func errBaseRepoStubFn() (ghrepo.Interface, error) { + return nil, errors.New("test base repo error") +} diff --git a/pkg/cmdutil/flags.go b/pkg/cmdutil/flags.go index fadbc5fbe..c0064099c 100644 --- a/pkg/cmdutil/flags.go +++ b/pkg/cmdutil/flags.go @@ -180,11 +180,3 @@ func isIncluded(value string, opts []string) bool { } return false } - -func CountSetFlags(flags *pflag.FlagSet) int { - count := 0 - flags.Visit(func(f *pflag.Flag) { - count++ - }) - return count -} From 4da4c82090d82c40ce13b43e0941d59e16181e04 Mon Sep 17 00:00:00 2001 From: William Martin Date: Fri, 20 Dec 2024 14:33:47 +0100 Subject: [PATCH 09/16] Rename secret BaseRepo func --- pkg/cmd/factory/remote_resolver.go | 1 + pkg/cmd/secret/delete/delete.go | 2 +- pkg/cmd/secret/delete/delete_test.go | 2 +- pkg/cmd/secret/list/list.go | 2 +- pkg/cmd/secret/list/list_test.go | 2 +- pkg/cmd/secret/set/set.go | 2 +- pkg/cmd/secret/set/set_test.go | 2 +- pkg/cmd/secret/shared/base_repo.go | 17 ++++++++--------- pkg/cmd/secret/shared/base_repo_test.go | 16 ++++++++-------- 9 files changed, 23 insertions(+), 23 deletions(-) diff --git a/pkg/cmd/factory/remote_resolver.go b/pkg/cmd/factory/remote_resolver.go index 7e27834e2..b008ade7e 100644 --- a/pkg/cmd/factory/remote_resolver.go +++ b/pkg/cmd/factory/remote_resolver.go @@ -69,6 +69,7 @@ func (rr *remoteResolver) Resolver() func() (context.Remotes, error) { sort.Sort(resolvedRemotes) // Filter remotes by hosts + // Note that this is not caching correctly: https://github.com/cli/cli/issues/10103 cachedRemotes := resolvedRemotes.FilterByHosts(hosts) // Filter again by default host if one is set diff --git a/pkg/cmd/secret/delete/delete.go b/pkg/cmd/secret/delete/delete.go index 64b02c9d9..87b96bee1 100644 --- a/pkg/cmd/secret/delete/delete.go +++ b/pkg/cmd/secret/delete/delete.go @@ -56,7 +56,7 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co // But if we are able to prompt, then we will wrap that up in a BaseRepoFunc that can prompt the user to // resolve the ambiguity. if opts.IO.CanPrompt() { - opts.BaseRepo = shared.PromptWhenMultipleRemotesBaseRepoFunc(opts.BaseRepo, f.Prompter) + opts.BaseRepo = shared.PromptWhenAmbiguousBaseRepoFunc(opts.BaseRepo, f.Prompter) } } diff --git a/pkg/cmd/secret/delete/delete_test.go b/pkg/cmd/secret/delete/delete_test.go index 91d688a89..f1f19a591 100644 --- a/pkg/cmd/secret/delete/delete_test.go +++ b/pkg/cmd/secret/delete/delete_test.go @@ -156,7 +156,7 @@ func TestNewCmdDeleteBaseRepoFuncs(t *testing.T) { { name: "when there is no repo flag provided, and no prompting, the base func requiring no ambiguity is used", args: "SECRET_NAME", - wantErr: shared.MultipleRemotesError{ + wantErr: shared.AmbiguousBaseRepoError{ Remotes: remotes, }, }, diff --git a/pkg/cmd/secret/list/list.go b/pkg/cmd/secret/list/list.go index 49ad47aff..4da9bc569 100644 --- a/pkg/cmd/secret/list/list.go +++ b/pkg/cmd/secret/list/list.go @@ -77,7 +77,7 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman // But if we are able to prompt, then we will wrap that up in a BaseRepoFunc that can prompt the user to // resolve the ambiguity. if opts.IO.CanPrompt() { - opts.BaseRepo = shared.PromptWhenMultipleRemotesBaseRepoFunc(opts.BaseRepo, f.Prompter) + opts.BaseRepo = shared.PromptWhenAmbiguousBaseRepoFunc(opts.BaseRepo, f.Prompter) } } diff --git a/pkg/cmd/secret/list/list_test.go b/pkg/cmd/secret/list/list_test.go index 63a899b77..729984deb 100644 --- a/pkg/cmd/secret/list/list_test.go +++ b/pkg/cmd/secret/list/list_test.go @@ -136,7 +136,7 @@ func TestNewCmdListBaseRepoFuncs(t *testing.T) { { name: "when there is no repo flag provided, and no prompting, the base func requiring no ambiguity is used", args: "", - wantErr: shared.MultipleRemotesError{ + wantErr: shared.AmbiguousBaseRepoError{ Remotes: remotes, }, }, diff --git a/pkg/cmd/secret/set/set.go b/pkg/cmd/secret/set/set.go index ba9b023a6..87deee71c 100644 --- a/pkg/cmd/secret/set/set.go +++ b/pkg/cmd/secret/set/set.go @@ -114,7 +114,7 @@ func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command // But if we are able to prompt, then we will wrap that up in a BaseRepoFunc that can prompt the user to // resolve the ambiguity. if opts.IO.CanPrompt() { - opts.BaseRepo = shared.PromptWhenMultipleRemotesBaseRepoFunc(opts.BaseRepo, f.Prompter) + opts.BaseRepo = shared.PromptWhenAmbiguousBaseRepoFunc(opts.BaseRepo, f.Prompter) } } diff --git a/pkg/cmd/secret/set/set_test.go b/pkg/cmd/secret/set/set_test.go index 63834521f..dd3124e82 100644 --- a/pkg/cmd/secret/set/set_test.go +++ b/pkg/cmd/secret/set/set_test.go @@ -255,7 +255,7 @@ func TestNewCmdSetBaseRepoFuncs(t *testing.T) { { name: "when there is no repo flag provided, and no prompting, the base func requiring no ambiguity is used", args: "SECRET_NAME", - wantErr: shared.MultipleRemotesError{ + wantErr: shared.AmbiguousBaseRepoError{ Remotes: remotes, }, }, diff --git a/pkg/cmd/secret/shared/base_repo.go b/pkg/cmd/secret/shared/base_repo.go index 3420f6da1..39a59149e 100644 --- a/pkg/cmd/secret/shared/base_repo.go +++ b/pkg/cmd/secret/shared/base_repo.go @@ -8,28 +8,27 @@ import ( "github.com/cli/cli/v2/internal/prompter" ) -type MultipleRemotesError struct { +type AmbiguousBaseRepoError struct { Remotes ghContext.Remotes } -func (e MultipleRemotesError) Error() string { +func (e AmbiguousBaseRepoError) Error() string { return "multiple remotes detected. please specify which repo to use by providing the -R or --repo argument" } type baseRepoFn func() (ghrepo.Interface, error) type remotesFn func() (ghContext.Remotes, error) -func PromptWhenMultipleRemotesBaseRepoFunc(baseRepoFn baseRepoFn, prompter prompter.Prompter) baseRepoFn { +func PromptWhenAmbiguousBaseRepoFunc(baseRepoFn baseRepoFn, prompter prompter.Prompter) baseRepoFn { return func() (ghrepo.Interface, error) { baseRepo, err := baseRepoFn() if err != nil { - var multipleRemotesError MultipleRemotesError - if !errors.As(err, &multipleRemotesError) { + var ambiguousBaseRepoErr AmbiguousBaseRepoError + if !errors.As(err, &ambiguousBaseRepoErr) { return nil, err } - // prompt for the base repo - baseRepo, err = promptForRepo(baseRepo, multipleRemotesError.Remotes, prompter) + baseRepo, err = promptForRepo(baseRepo, ambiguousBaseRepoErr.Remotes, prompter) if err != nil { return nil, err } @@ -40,7 +39,7 @@ func PromptWhenMultipleRemotesBaseRepoFunc(baseRepoFn baseRepoFn, prompter promp } // RequireNoAmbiguityBaseRepoFunc returns a function to resolve the base repo, ensuring that -// there was only one remote. +// there was only one option, regardless of whether the base repo had been set. func RequireNoAmbiguityBaseRepoFunc(baseRepo baseRepoFn, remotes remotesFn) baseRepoFn { return func() (ghrepo.Interface, error) { // TODO: Is this really correct? Some remotes may not be in the same network. We probably need to resolve the @@ -51,7 +50,7 @@ func RequireNoAmbiguityBaseRepoFunc(baseRepo baseRepoFn, remotes remotesFn) base } if remotes.Len() > 1 { - return nil, MultipleRemotesError{Remotes: remotes} + return nil, AmbiguousBaseRepoError{Remotes: remotes} } return baseRepo() diff --git a/pkg/cmd/secret/shared/base_repo_test.go b/pkg/cmd/secret/shared/base_repo_test.go index 64b2a3d55..469a9722c 100644 --- a/pkg/cmd/secret/shared/base_repo_test.go +++ b/pkg/cmd/secret/shared/base_repo_test.go @@ -40,7 +40,7 @@ func TestRequireNoAmbiguityBaseRepoFunc(t *testing.T) { _, err := baseRepoFn() // It succeeds and returns the inner base repo - var multipleRemotesError shared.MultipleRemotesError + var multipleRemotesError shared.AmbiguousBaseRepoError require.ErrorAs(t, err, &multipleRemotesError) require.Equal(t, ghContext.Remotes{ { @@ -92,7 +92,7 @@ func TestPromptWhenMultipleRemotesBaseRepoFunc(t *testing.T) { t.Parallel() // Given the base repo function succeeds - baseRepoFn := shared.PromptWhenMultipleRemotesBaseRepoFunc(baseRepoStubFn, nil) + baseRepoFn := shared.PromptWhenAmbiguousBaseRepoFunc(baseRepoStubFn, nil) // When fetching the base repo baseRepo, err := baseRepoFn() @@ -117,7 +117,7 @@ func TestPromptWhenMultipleRemotesBaseRepoFunc(t *testing.T) { ) // Given the wrapped base repo func returns a specific error - baseRepoFn := shared.PromptWhenMultipleRemotesBaseRepoFunc(errMultipleRemotesStubFn, pm) + baseRepoFn := shared.PromptWhenAmbiguousBaseRepoFunc(errMultipleRemotesStubFn, pm) // When fetching the base repo baseRepo, err := baseRepoFn() @@ -141,7 +141,7 @@ func TestPromptWhenMultipleRemotesBaseRepoFunc(t *testing.T) { ) // Given the wrapped base repo func returns a specific error - baseRepoFn := shared.PromptWhenMultipleRemotesBaseRepoFunc(errMultipleRemotesStubFn, pm) + baseRepoFn := shared.PromptWhenAmbiguousBaseRepoFunc(errMultipleRemotesStubFn, pm) // When fetching the base repo _, err := baseRepoFn() @@ -154,7 +154,7 @@ func TestPromptWhenMultipleRemotesBaseRepoFunc(t *testing.T) { t.Parallel() // Given the wrapped base repo func returns a non-specific error - baseRepoFn := shared.PromptWhenMultipleRemotesBaseRepoFunc(errBaseRepoStubFn, nil) + baseRepoFn := shared.PromptWhenAmbiguousBaseRepoFunc(errBaseRepoStubFn, nil) // When fetching the base repo _, err := baseRepoFn() @@ -165,8 +165,8 @@ func TestPromptWhenMultipleRemotesBaseRepoFunc(t *testing.T) { } func TestMultipleRemotesErrorMessage(t *testing.T) { - err := shared.MultipleRemotesError{} - require.EqualError(t, err, "multiple remotes detected. please specify which repo to use by providing the -R or --repo argument") + err := shared.AmbiguousBaseRepoError{} + require.EqualError(t, err, "multiple remotes detected. please specify which repo to use by providing the -R, --repo argument") } func errMultipleRemotesStubFn() (ghrepo.Interface, error) { @@ -185,7 +185,7 @@ func errMultipleRemotesStubFn() (ghrepo.Interface, error) { Repo: ghrepo.New("owner", "repo"), } - return nil, shared.MultipleRemotesError{ + return nil, shared.AmbiguousBaseRepoError{ Remotes: ghContext.Remotes{ remote1, remote2, From d831e3e1dbb67e81cf1cbc057f1fe6b86028b5af Mon Sep 17 00:00:00 2001 From: William Martin Date: Thu, 9 Jan 2025 17:33:26 +0100 Subject: [PATCH 10/16] Remove validated TODO and add review warning --- pkg/cmd/secret/shared/base_repo.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/secret/shared/base_repo.go b/pkg/cmd/secret/shared/base_repo.go index 39a59149e..652d92064 100644 --- a/pkg/cmd/secret/shared/base_repo.go +++ b/pkg/cmd/secret/shared/base_repo.go @@ -13,7 +13,7 @@ type AmbiguousBaseRepoError struct { } func (e AmbiguousBaseRepoError) Error() string { - return "multiple remotes detected. please specify which repo to use by providing the -R or --repo argument" + return "multiple remotes detected. please specify which repo to use by providing the -R, --repo argument" } type baseRepoFn func() (ghrepo.Interface, error) @@ -42,8 +42,6 @@ func PromptWhenAmbiguousBaseRepoFunc(baseRepoFn baseRepoFn, prompter prompter.Pr // there was only one option, regardless of whether the base repo had been set. func RequireNoAmbiguityBaseRepoFunc(baseRepo baseRepoFn, remotes remotesFn) baseRepoFn { return func() (ghrepo.Interface, error) { - // TODO: Is this really correct? Some remotes may not be in the same network. We probably need to resolve the - // network rather than looking at the remotes? remotes, err := remotes() if err != nil { return nil, err @@ -57,10 +55,13 @@ func RequireNoAmbiguityBaseRepoFunc(baseRepo baseRepoFn, remotes remotesFn) base } } +// REVIEW WARNING: I have not had a close look at this function yet, I do not vouch for it. func promptForRepo(baseRepo ghrepo.Interface, remotes ghContext.Remotes, prompter prompter.Prompter) (ghrepo.Interface, error) { var defaultRepo string var remoteArray []string + // TODO: consider whether we should just go with the default order of remotes because then + // users that are familiar can just hit enter and achieve the behaviour they had before. if defaultRemote, _ := remotes.ResolvedRemote(); defaultRemote != nil { // this is a remote explicitly chosen via `repo set-default` defaultRepo = ghrepo.FullName(defaultRemote) From 5630252f7861cf7b4b6bea82c10842f7e7931302 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 15 Jan 2025 13:22:10 +0100 Subject: [PATCH 11/16] Add acceptance test for secrets remote disambiguation --- ...secret-require-remote-disambiguation.txtar | 36 +++++++++++++++++++ 1 file changed, 36 insertions(+) create mode 100644 acceptance/testdata/secret/secret-require-remote-disambiguation.txtar diff --git a/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar b/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar new file mode 100644 index 000000000..f4d1bbb4a --- /dev/null +++ b/acceptance/testdata/secret/secret-require-remote-disambiguation.txtar @@ -0,0 +1,36 @@ +# Set up env vars +env REPO=${SCRIPT_NAME}-${RANDOM_STRING} + +# Use gh as a credential helper +exec gh auth setup-git + +# Create a repository with a file so it has a default branch +exec gh repo create ${ORG}/${REPO} --add-readme --private + +# Defer repo cleanup +defer gh repo delete --yes ${ORG}/${REPO} + +# Create a fork +exec gh repo fork ${ORG}/${REPO} --org ${ORG} --fork-name ${REPO}-fork + +# Defer fork cleanup +defer gh repo delete --yes ${ORG}/${REPO}-fork + +# Sleep to allow the fork to be created before cloning +sleep 2 + +# Clone and move into the fork repo +exec gh repo clone ${ORG}/${REPO}-fork +cd ${REPO}-fork + +# Secret list requires disambiguation +! exec gh secret list +stderr 'multiple remotes detected. please specify which repo to use by providing the -R, --repo argument' + +# Secret set requires disambiguation +! exec gh secret set 'TEST_SECRET_NAME' --body 'TEST_SECRET_VALUE' +stderr 'multiple remotes detected. please specify which repo to use by providing the -R, --repo argument' + +# 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' From ce47fabc27b1fbb1ffb275c33ce4097233dd0ddb Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 15 Jan 2025 13:55:36 +0100 Subject: [PATCH 12/16] Move secret base repo prompting earlier --- pkg/cmd/secret/delete/delete.go | 16 ++++++++-------- pkg/cmd/secret/set/set.go | 23 ++++++++++++----------- 2 files changed, 20 insertions(+), 19 deletions(-) diff --git a/pkg/cmd/secret/delete/delete.go b/pkg/cmd/secret/delete/delete.go index 87b96bee1..8f17fdf59 100644 --- a/pkg/cmd/secret/delete/delete.go +++ b/pkg/cmd/secret/delete/delete.go @@ -99,6 +99,14 @@ func removeRun(opts *DeleteOptions) error { return err } + var baseRepo ghrepo.Interface + if secretEntity == shared.Repository || secretEntity == shared.Environment { + baseRepo, err = opts.BaseRepo() + if err != nil { + return err + } + } + secretApp, err := shared.GetSecretApp(opts.Application, secretEntity) if err != nil { return err @@ -108,14 +116,6 @@ func removeRun(opts *DeleteOptions) error { return fmt.Errorf("%s secrets are not supported for %s", secretEntity, secretApp) } - var baseRepo ghrepo.Interface - if secretEntity == shared.Repository || secretEntity == shared.Environment { - baseRepo, err = opts.BaseRepo() - if err != nil { - return err - } - } - cfg, err := opts.Config() if err != nil { return err diff --git a/pkg/cmd/secret/set/set.go b/pkg/cmd/secret/set/set.go index 87deee71c..a35c40aa3 100644 --- a/pkg/cmd/secret/set/set.go +++ b/pkg/cmd/secret/set/set.go @@ -178,23 +178,13 @@ func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command } func setRun(opts *SetOptions) error { - secrets, err := getSecretsFromOptions(opts) - if err != nil { - return err - } - - 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 var host string var baseRepo ghrepo.Interface if orgName == "" && !opts.UserSecrets { + var err error baseRepo, err = opts.BaseRepo() if err != nil { return err @@ -209,6 +199,17 @@ func setRun(opts *SetOptions) error { host, _ = cfg.Authentication().DefaultHost() } + secrets, err := getSecretsFromOptions(opts) + if err != nil { + return err + } + + c, err := opts.HttpClient() + if err != nil { + return fmt.Errorf("could not create http client: %w", err) + } + client := api.NewClientFromHTTP(c) + secretEntity, err := shared.GetSecretEntity(orgName, envName, opts.UserSecrets) if err != nil { return err From a47327aee6396df90349e3b61a20da3d11c2ed6c Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 15 Jan 2025 14:11:24 +0100 Subject: [PATCH 13/16] Secret base repo prompting should not use resolved remote This is because the secret commands don't use the SmartBaseRepo behaviour, and therefore don't care about the resolved remote. --- pkg/cmd/secret/shared/base_repo.go | 46 +++++++------------------ pkg/cmd/secret/shared/base_repo_test.go | 8 ++--- 2 files changed, 16 insertions(+), 38 deletions(-) diff --git a/pkg/cmd/secret/shared/base_repo.go b/pkg/cmd/secret/shared/base_repo.go index 652d92064..c46752794 100644 --- a/pkg/cmd/secret/shared/base_repo.go +++ b/pkg/cmd/secret/shared/base_repo.go @@ -28,10 +28,22 @@ func PromptWhenAmbiguousBaseRepoFunc(baseRepoFn baseRepoFn, prompter prompter.Pr return nil, err } - baseRepo, err = promptForRepo(baseRepo, ambiguousBaseRepoErr.Remotes, prompter) + baseRepoOptions := make([]string, len(ambiguousBaseRepoErr.Remotes)) + for i, remote := range ambiguousBaseRepoErr.Remotes { + baseRepoOptions[i] = ghrepo.FullName(remote) + } + + selectedBaseRepo, err := prompter.Select("Select a base repo", baseRepoOptions[0], baseRepoOptions) if err != nil { return nil, err } + + selectedRepo, err := ghrepo.FromFullName(baseRepoOptions[selectedBaseRepo]) + if err != nil { + return nil, err + } + + return selectedRepo, nil } return baseRepo, nil @@ -54,35 +66,3 @@ func RequireNoAmbiguityBaseRepoFunc(baseRepo baseRepoFn, remotes remotesFn) base return baseRepo() } } - -// REVIEW WARNING: I have not had a close look at this function yet, I do not vouch for it. -func promptForRepo(baseRepo ghrepo.Interface, remotes ghContext.Remotes, prompter prompter.Prompter) (ghrepo.Interface, error) { - var defaultRepo string - var remoteArray []string - - // TODO: consider whether we should just go with the default order of remotes because then - // users that are familiar can just hit enter and achieve the behaviour they had before. - if defaultRemote, _ := remotes.ResolvedRemote(); defaultRemote != nil { - // this is a remote explicitly chosen via `repo set-default` - defaultRepo = ghrepo.FullName(defaultRemote) - } else if len(remotes) > 0 { - // as a fallback, just pick the first remote - defaultRepo = ghrepo.FullName(remotes[0]) - } - - for _, remote := range remotes { - remoteArray = append(remoteArray, ghrepo.FullName(remote)) - } - - baseRepoInput, errInput := prompter.Select("Select a base repo", defaultRepo, remoteArray) - if errInput != nil { - return baseRepo, errInput - } - - selectedRepo, errSelectedRepo := ghrepo.FromFullName(remoteArray[baseRepoInput]) - if errSelectedRepo != nil { - return baseRepo, errSelectedRepo - } - - return selectedRepo, nil -} diff --git a/pkg/cmd/secret/shared/base_repo_test.go b/pkg/cmd/secret/shared/base_repo_test.go index 469a9722c..d5cdc5e23 100644 --- a/pkg/cmd/secret/shared/base_repo_test.go +++ b/pkg/cmd/secret/shared/base_repo_test.go @@ -102,7 +102,7 @@ func TestPromptWhenMultipleRemotesBaseRepoFunc(t *testing.T) { require.True(t, ghrepo.IsSame(ghrepo.New("owner", "repo"), baseRepo)) }) - t.Run("when the wrapped base repo func returns a specific error, then the prompter is used for disambiguation, with the resolved remote as the default", func(t *testing.T) { + t.Run("when the wrapped base repo func returns a specific error, then the prompter is used for disambiguation, with the remote ordering remaining unchanged", func(t *testing.T) { t.Parallel() pm := prompter.NewMockPrompter(t) @@ -110,8 +110,7 @@ func TestPromptWhenMultipleRemotesBaseRepoFunc(t *testing.T) { "Select a base repo", []string{"owner/fork", "owner/repo"}, func(_, def string, opts []string) (int, error) { - t.Helper() - require.Equal(t, "owner/repo", def) + require.Equal(t, "owner/fork", def) return prompter.IndexFor(opts, "owner/repo") }, ) @@ -179,8 +178,7 @@ func errMultipleRemotesStubFn() (ghrepo.Interface, error) { remote2 := &ghContext.Remote{ Remote: &git.Remote{ - Name: "upstream", - Resolved: "base", + Name: "upstream", }, Repo: ghrepo.New("owner", "repo"), } From b382b2472823a929117c751cbbfdf20e4c232c61 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 15 Jan 2025 14:22:43 +0100 Subject: [PATCH 14/16] Print informative message before prompting for secret repo --- pkg/cmd/secret/delete/delete.go | 2 +- pkg/cmd/secret/list/list.go | 2 +- pkg/cmd/secret/set/set.go | 2 +- pkg/cmd/secret/shared/base_repo.go | 5 ++++- pkg/cmd/secret/shared/base_repo_test.go | 22 +++++++++++++++++----- 5 files changed, 24 insertions(+), 9 deletions(-) diff --git a/pkg/cmd/secret/delete/delete.go b/pkg/cmd/secret/delete/delete.go index 8f17fdf59..78550f928 100644 --- a/pkg/cmd/secret/delete/delete.go +++ b/pkg/cmd/secret/delete/delete.go @@ -56,7 +56,7 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co // But if we are able to prompt, then we will wrap that up in a BaseRepoFunc that can prompt the user to // resolve the ambiguity. if opts.IO.CanPrompt() { - opts.BaseRepo = shared.PromptWhenAmbiguousBaseRepoFunc(opts.BaseRepo, f.Prompter) + opts.BaseRepo = shared.PromptWhenAmbiguousBaseRepoFunc(opts.BaseRepo, f.IOStreams, f.Prompter) } } diff --git a/pkg/cmd/secret/list/list.go b/pkg/cmd/secret/list/list.go index 4da9bc569..41cf88e64 100644 --- a/pkg/cmd/secret/list/list.go +++ b/pkg/cmd/secret/list/list.go @@ -77,7 +77,7 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman // But if we are able to prompt, then we will wrap that up in a BaseRepoFunc that can prompt the user to // resolve the ambiguity. if opts.IO.CanPrompt() { - opts.BaseRepo = shared.PromptWhenAmbiguousBaseRepoFunc(opts.BaseRepo, f.Prompter) + opts.BaseRepo = shared.PromptWhenAmbiguousBaseRepoFunc(opts.BaseRepo, f.IOStreams, f.Prompter) } } diff --git a/pkg/cmd/secret/set/set.go b/pkg/cmd/secret/set/set.go index a35c40aa3..ba90e1f67 100644 --- a/pkg/cmd/secret/set/set.go +++ b/pkg/cmd/secret/set/set.go @@ -114,7 +114,7 @@ func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command // But if we are able to prompt, then we will wrap that up in a BaseRepoFunc that can prompt the user to // resolve the ambiguity. if opts.IO.CanPrompt() { - opts.BaseRepo = shared.PromptWhenAmbiguousBaseRepoFunc(opts.BaseRepo, f.Prompter) + opts.BaseRepo = shared.PromptWhenAmbiguousBaseRepoFunc(opts.BaseRepo, f.IOStreams, f.Prompter) } } diff --git a/pkg/cmd/secret/shared/base_repo.go b/pkg/cmd/secret/shared/base_repo.go index c46752794..d902105aa 100644 --- a/pkg/cmd/secret/shared/base_repo.go +++ b/pkg/cmd/secret/shared/base_repo.go @@ -2,10 +2,12 @@ package shared import ( "errors" + "fmt" ghContext "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/internal/ghrepo" "github.com/cli/cli/v2/internal/prompter" + "github.com/cli/cli/v2/pkg/iostreams" ) type AmbiguousBaseRepoError struct { @@ -19,7 +21,7 @@ func (e AmbiguousBaseRepoError) Error() string { type baseRepoFn func() (ghrepo.Interface, error) type remotesFn func() (ghContext.Remotes, error) -func PromptWhenAmbiguousBaseRepoFunc(baseRepoFn baseRepoFn, prompter prompter.Prompter) baseRepoFn { +func PromptWhenAmbiguousBaseRepoFunc(baseRepoFn baseRepoFn, ios *iostreams.IOStreams, prompter prompter.Prompter) baseRepoFn { return func() (ghrepo.Interface, error) { baseRepo, err := baseRepoFn() if err != nil { @@ -33,6 +35,7 @@ func PromptWhenAmbiguousBaseRepoFunc(baseRepoFn baseRepoFn, prompter prompter.Pr baseRepoOptions[i] = ghrepo.FullName(remote) } + fmt.Fprintf(ios.Out, "%s Multiple remotes detected. Due to the sensitive nature of secrets, requiring disambiguation.\n", ios.ColorScheme().WarningIcon()) selectedBaseRepo, err := prompter.Select("Select a base repo", baseRepoOptions[0], baseRepoOptions) if err != nil { return nil, err diff --git a/pkg/cmd/secret/shared/base_repo_test.go b/pkg/cmd/secret/shared/base_repo_test.go index d5cdc5e23..6b7aa02ff 100644 --- a/pkg/cmd/secret/shared/base_repo_test.go +++ b/pkg/cmd/secret/shared/base_repo_test.go @@ -6,6 +6,7 @@ import ( ghContext "github.com/cli/cli/v2/context" "github.com/cli/cli/v2/pkg/cmd/secret/shared" + "github.com/cli/cli/v2/pkg/iostreams" "github.com/stretchr/testify/require" "github.com/cli/cli/v2/git" @@ -91,8 +92,10 @@ func TestPromptWhenMultipleRemotesBaseRepoFunc(t *testing.T) { t.Run("when there is no error from wrapped base repo func, then it succeeds without prompting", func(t *testing.T) { t.Parallel() + ios, _, _, _ := iostreams.Test() + // Given the base repo function succeeds - baseRepoFn := shared.PromptWhenAmbiguousBaseRepoFunc(baseRepoStubFn, nil) + baseRepoFn := shared.PromptWhenAmbiguousBaseRepoFunc(baseRepoStubFn, ios, nil) // When fetching the base repo baseRepo, err := baseRepoFn() @@ -105,6 +108,8 @@ func TestPromptWhenMultipleRemotesBaseRepoFunc(t *testing.T) { t.Run("when the wrapped base repo func returns a specific error, then the prompter is used for disambiguation, with the remote ordering remaining unchanged", func(t *testing.T) { t.Parallel() + ios, _, stdout, _ := iostreams.Test() + pm := prompter.NewMockPrompter(t) pm.RegisterSelect( "Select a base repo", @@ -116,12 +121,15 @@ func TestPromptWhenMultipleRemotesBaseRepoFunc(t *testing.T) { ) // Given the wrapped base repo func returns a specific error - baseRepoFn := shared.PromptWhenAmbiguousBaseRepoFunc(errMultipleRemotesStubFn, pm) + baseRepoFn := shared.PromptWhenAmbiguousBaseRepoFunc(errMultipleRemotesStubFn, ios, pm) // When fetching the base repo baseRepo, err := baseRepoFn() - // It uses the prompter for disambiguation + // It prints an informative message + require.Equal(t, "! Multiple remotes detected. Due to the sensitive nature of secrets, requiring disambiguation.\n", stdout.String()) + + // And it uses the prompter for disambiguation require.NoError(t, err) require.True(t, ghrepo.IsSame(ghrepo.New("owner", "repo"), baseRepo)) }) @@ -129,6 +137,8 @@ func TestPromptWhenMultipleRemotesBaseRepoFunc(t *testing.T) { t.Run("when the prompter returns an error, then it is returned", func(t *testing.T) { t.Parallel() + ios, _, _, _ := iostreams.Test() + // Given the prompter returns an error pm := prompter.NewMockPrompter(t) pm.RegisterSelect( @@ -140,7 +150,7 @@ func TestPromptWhenMultipleRemotesBaseRepoFunc(t *testing.T) { ) // Given the wrapped base repo func returns a specific error - baseRepoFn := shared.PromptWhenAmbiguousBaseRepoFunc(errMultipleRemotesStubFn, pm) + baseRepoFn := shared.PromptWhenAmbiguousBaseRepoFunc(errMultipleRemotesStubFn, ios, pm) // When fetching the base repo _, err := baseRepoFn() @@ -152,8 +162,10 @@ func TestPromptWhenMultipleRemotesBaseRepoFunc(t *testing.T) { t.Run("when the wrapped base repo func returns a non-specific error, then it is returned", func(t *testing.T) { t.Parallel() + ios, _, _, _ := iostreams.Test() + // Given the wrapped base repo func returns a non-specific error - baseRepoFn := shared.PromptWhenAmbiguousBaseRepoFunc(errBaseRepoStubFn, nil) + baseRepoFn := shared.PromptWhenAmbiguousBaseRepoFunc(errBaseRepoStubFn, ios, nil) // When fetching the base repo _, err := baseRepoFn() From 88a64d2a1135db1a3cfa508d43ebdb9c9b1cf4f8 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 15 Jan 2025 14:31:32 +0100 Subject: [PATCH 15/16] Change wording on secret repo prompt --- pkg/cmd/secret/delete/delete_test.go | 2 +- pkg/cmd/secret/list/list_test.go | 2 +- pkg/cmd/secret/set/set_test.go | 2 +- pkg/cmd/secret/shared/base_repo.go | 2 +- pkg/cmd/secret/shared/base_repo_test.go | 4 ++-- 5 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pkg/cmd/secret/delete/delete_test.go b/pkg/cmd/secret/delete/delete_test.go index f1f19a591..377879cf2 100644 --- a/pkg/cmd/secret/delete/delete_test.go +++ b/pkg/cmd/secret/delete/delete_test.go @@ -165,7 +165,7 @@ func TestNewCmdDeleteBaseRepoFuncs(t *testing.T) { args: "SECRET_NAME", prompterStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect( - "Select a base repo", + "Select a repo", []string{"owner/fork", "owner/repo"}, func(_, _ string, opts []string) (int, error) { return prompter.IndexFor(opts, "owner/fork") diff --git a/pkg/cmd/secret/list/list_test.go b/pkg/cmd/secret/list/list_test.go index 729984deb..601de2572 100644 --- a/pkg/cmd/secret/list/list_test.go +++ b/pkg/cmd/secret/list/list_test.go @@ -145,7 +145,7 @@ func TestNewCmdListBaseRepoFuncs(t *testing.T) { args: "", prompterStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect( - "Select a base repo", + "Select a repo", []string{"owner/fork", "owner/repo"}, func(_, _ string, opts []string) (int, error) { return prompter.IndexFor(opts, "owner/fork") diff --git a/pkg/cmd/secret/set/set_test.go b/pkg/cmd/secret/set/set_test.go index dd3124e82..99c8c6cbe 100644 --- a/pkg/cmd/secret/set/set_test.go +++ b/pkg/cmd/secret/set/set_test.go @@ -264,7 +264,7 @@ func TestNewCmdSetBaseRepoFuncs(t *testing.T) { args: "SECRET_NAME", prompterStubs: func(pm *prompter.MockPrompter) { pm.RegisterSelect( - "Select a base repo", + "Select a repo", []string{"owner/fork", "owner/repo"}, func(_, _ string, opts []string) (int, error) { return prompter.IndexFor(opts, "owner/fork") diff --git a/pkg/cmd/secret/shared/base_repo.go b/pkg/cmd/secret/shared/base_repo.go index d902105aa..308117ac6 100644 --- a/pkg/cmd/secret/shared/base_repo.go +++ b/pkg/cmd/secret/shared/base_repo.go @@ -36,7 +36,7 @@ func PromptWhenAmbiguousBaseRepoFunc(baseRepoFn baseRepoFn, ios *iostreams.IOStr } fmt.Fprintf(ios.Out, "%s Multiple remotes detected. Due to the sensitive nature of secrets, requiring disambiguation.\n", ios.ColorScheme().WarningIcon()) - selectedBaseRepo, err := prompter.Select("Select a base repo", baseRepoOptions[0], baseRepoOptions) + selectedBaseRepo, err := prompter.Select("Select a repo", baseRepoOptions[0], baseRepoOptions) if err != nil { return nil, err } diff --git a/pkg/cmd/secret/shared/base_repo_test.go b/pkg/cmd/secret/shared/base_repo_test.go index 6b7aa02ff..d78fad7e5 100644 --- a/pkg/cmd/secret/shared/base_repo_test.go +++ b/pkg/cmd/secret/shared/base_repo_test.go @@ -112,7 +112,7 @@ func TestPromptWhenMultipleRemotesBaseRepoFunc(t *testing.T) { pm := prompter.NewMockPrompter(t) pm.RegisterSelect( - "Select a base repo", + "Select a repo", []string{"owner/fork", "owner/repo"}, func(_, def string, opts []string) (int, error) { require.Equal(t, "owner/fork", def) @@ -142,7 +142,7 @@ func TestPromptWhenMultipleRemotesBaseRepoFunc(t *testing.T) { // Given the prompter returns an error pm := prompter.NewMockPrompter(t) pm.RegisterSelect( - "Select a base repo", + "Select a repo", []string{"owner/fork", "owner/repo"}, func(_, _ string, opts []string) (int, error) { return 0, errors.New("test prompt error") From e7ffb1e435e4faa081af3c6638f7f648e94f94f0 Mon Sep 17 00:00:00 2001 From: William Martin Date: Wed, 15 Jan 2025 14:38:05 +0100 Subject: [PATCH 16/16] Fix typo in secret base repo selection comment --- pkg/cmd/secret/delete/delete.go | 2 +- pkg/cmd/secret/list/list.go | 2 +- pkg/cmd/secret/set/set.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/secret/delete/delete.go b/pkg/cmd/secret/delete/delete.go index 78550f928..84a5e7a83 100644 --- a/pkg/cmd/secret/delete/delete.go +++ b/pkg/cmd/secret/delete/delete.go @@ -50,7 +50,7 @@ func NewCmdDelete(f *cmdutil.Factory, runF func(*DeleteOptions) error) *cobra.Co // So there's no reason to use the specialised BaseRepoFunc that requires remote disambiguation. opts.BaseRepo = f.BaseRepo if !cmd.Flags().Changed("repo") { - // If they haven't specified a repo directly, then we will wrap the BaseRepoFunc in one that error if + // 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) // But if we are able to prompt, then we will wrap that up in a BaseRepoFunc that can prompt the user to diff --git a/pkg/cmd/secret/list/list.go b/pkg/cmd/secret/list/list.go index 41cf88e64..af3526386 100644 --- a/pkg/cmd/secret/list/list.go +++ b/pkg/cmd/secret/list/list.go @@ -71,7 +71,7 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman // So there's no reason to use the specialised BaseRepoFunc that requires remote disambiguation. opts.BaseRepo = f.BaseRepo if !cmd.Flags().Changed("repo") { - // If they haven't specified a repo directly, then we will wrap the BaseRepoFunc in one that error if + // 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) // But if we are able to prompt, then we will wrap that up in a BaseRepoFunc that can prompt the user to diff --git a/pkg/cmd/secret/set/set.go b/pkg/cmd/secret/set/set.go index ba90e1f67..3c12b4192 100644 --- a/pkg/cmd/secret/set/set.go +++ b/pkg/cmd/secret/set/set.go @@ -108,7 +108,7 @@ func NewCmdSet(f *cmdutil.Factory, runF func(*SetOptions) error) *cobra.Command // So there's no reason to use the specialised BaseRepoFunc that requires remote disambiguation. opts.BaseRepo = f.BaseRepo if !cmd.Flags().Changed("repo") { - // If they haven't specified a repo directly, then we will wrap the BaseRepoFunc in one that error if + // 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) // But if we are able to prompt, then we will wrap that up in a BaseRepoFunc that can prompt the user to