From c0fc31f7d551350632a2c260331de8353e73deeb Mon Sep 17 00:00:00 2001 From: vilmibm Date: Wed, 9 Sep 2020 15:55:21 -0500 Subject: [PATCH] use CanPrompt in commands --- pkg/cmd/auth/login/login.go | 6 ++++ pkg/cmd/auth/login/login_test.go | 11 ++----- pkg/cmd/auth/logout/logout.go | 16 +++++----- pkg/cmd/auth/logout/logout_test.go | 37 ++++++++++++++++------- pkg/cmd/auth/refresh/refresh.go | 18 ++++++++---- pkg/cmd/auth/refresh/refresh_test.go | 44 +++++++++++++++++++--------- pkg/cmd/issue/create/create.go | 4 +++ pkg/cmd/pr/create/create.go | 5 ++-- pkg/cmd/pr/create/create_test.go | 2 +- pkg/cmd/pr/merge/merge.go | 4 +-- pkg/cmd/pr/merge/merge_test.go | 2 +- pkg/cmd/pr/review/review.go | 4 +-- pkg/cmd/pr/review/review_test.go | 2 +- pkg/cmd/repo/create/create.go | 11 ++++++- pkg/cmd/repo/create/create_test.go | 18 +++++++----- pkg/cmd/repo/fork/fork.go | 2 +- 16 files changed, 119 insertions(+), 67 deletions(-) diff --git a/pkg/cmd/auth/login/login.go b/pkg/cmd/auth/login/login.go index c0a24c5c5..42cd69f51 100644 --- a/pkg/cmd/auth/login/login.go +++ b/pkg/cmd/auth/login/login.go @@ -72,6 +72,12 @@ func NewCmdLogin(f *cmdutil.Factory, runF func(*LoginOptions) error) *cobra.Comm if opts.Hostname == "" { opts.Hostname = ghinstance.Default() } + } else { + if !opts.IO.CanPrompt() { + // TODO this error message kind of sucks since the prompting might be disabled from being + // in a nontty or from the config setting. Is it enough info? + return &cmdutil.FlagError{Err: errors.New("--with-token required when prompts disabled")} + } } if cmd.Flags().Changed("hostname") { diff --git a/pkg/cmd/auth/login/login_test.go b/pkg/cmd/auth/login/login_test.go index d90a0a2d6..5d0ba1942 100644 --- a/pkg/cmd/auth/login/login_test.go +++ b/pkg/cmd/auth/login/login_test.go @@ -49,19 +49,13 @@ func Test_NewCmdLogin(t *testing.T) { name: "nontty, hostname", stdinTTY: false, cli: "--hostname claire.redfield", - wants: LoginOptions{ - Hostname: "claire.redfield", - Token: "", - }, + wantsErr: true, }, { name: "nontty", stdinTTY: false, cli: "", - wants: LoginOptions{ - Hostname: "", - Token: "", - }, + wantsErr: true, }, { name: "nontty, with-token, hostname", @@ -109,6 +103,7 @@ func Test_NewCmdLogin(t *testing.T) { IOStreams: io, } + io.SetStdoutTTY(true) io.SetStdinTTY(tt.stdinTTY) if tt.stdin != "" { stdin.WriteString(tt.stdin) diff --git a/pkg/cmd/auth/logout/logout.go b/pkg/cmd/auth/logout/logout.go index c5810f5b8..4a39f2666 100644 --- a/pkg/cmd/auth/logout/logout.go +++ b/pkg/cmd/auth/logout/logout.go @@ -48,6 +48,10 @@ func NewCmdLogout(f *cmdutil.Factory, runF func(*LogoutOptions) error) *cobra.Co # => log out of specified host `), RunE: func(cmd *cobra.Command, args []string) error { + if opts.Hostname == "" && !opts.IO.CanPrompt() { + return &cmdutil.FlagError{Err: errors.New("--hostname required when prompts disabled")} + } + if runF != nil { return runF(opts) } @@ -62,16 +66,8 @@ func NewCmdLogout(f *cmdutil.Factory, runF func(*LogoutOptions) error) *cobra.Co } func logoutRun(opts *LogoutOptions) error { - isTTY := opts.IO.IsStdinTTY() && opts.IO.IsStdoutTTY() - hostname := opts.Hostname - if !isTTY && hostname == "" { - return errors.New("--hostname required when not attached to a terminal") - } - - showConfirm := isTTY && hostname == "" - cfg, err := opts.Config() if err != nil { return err @@ -131,7 +127,7 @@ func logoutRun(opts *LogoutOptions) error { usernameStr = fmt.Sprintf(" account '%s'", username) } - if showConfirm { + if opts.IO.CanPrompt() { var keepGoing bool err := prompt.SurveyAskOne(&survey.Confirm{ Message: fmt.Sprintf("Are you sure you want to log out of %s%s?", hostname, usernameStr), @@ -152,6 +148,8 @@ func logoutRun(opts *LogoutOptions) error { return fmt.Errorf("failed to write config, authentication configuration not updated: %w", err) } + isTTY := opts.IO.IsStdinTTY() && opts.IO.IsStdoutTTY() + if isTTY { fmt.Fprintf(opts.IO.ErrOut, "%s Logged out of %s%s\n", utils.GreenCheck(), utils.Bold(hostname), usernameStr) diff --git a/pkg/cmd/auth/logout/logout_test.go b/pkg/cmd/auth/logout/logout_test.go index 724705776..a9d14cc91 100644 --- a/pkg/cmd/auth/logout/logout_test.go +++ b/pkg/cmd/auth/logout/logout_test.go @@ -17,24 +17,40 @@ import ( func Test_NewCmdLogout(t *testing.T) { tests := []struct { - name string - cli string - wants LogoutOptions + name string + cli string + wants LogoutOptions + wantsErr bool + tty bool }{ { - name: "with hostname", + name: "tty with hostname", + tty: true, cli: "--hostname harry.mason", wants: LogoutOptions{ Hostname: "harry.mason", }, }, { - name: "no arguments", + name: "tty no arguments", + tty: true, cli: "", wants: LogoutOptions{ Hostname: "", }, }, + { + name: "nontty with hostname", + cli: "--hostname harry.mason", + wants: LogoutOptions{ + Hostname: "harry.mason", + }, + }, + { + name: "nontty no arguments", + cli: "", + wantsErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -42,6 +58,8 @@ func Test_NewCmdLogout(t *testing.T) { f := &cmdutil.Factory{ IOStreams: io, } + io.SetStdinTTY(tt.tty) + io.SetStdoutTTY(tt.tty) argv, err := shlex.Split(tt.cli) assert.NoError(t, err) @@ -60,6 +78,10 @@ func Test_NewCmdLogout(t *testing.T) { cmd.SetErr(&bytes.Buffer{}) _, err = cmd.ExecuteC() + if tt.wantsErr { + assert.Error(t, err) + return + } assert.NoError(t, err) assert.Equal(t, tt.wants.Hostname, gotOpts.Hostname) @@ -185,11 +207,6 @@ func Test_logoutRun_nontty(t *testing.T) { wantErr *regexp.Regexp ghtoken string }{ - { - name: "no arguments", - wantErr: regexp.MustCompile(`hostname required when not`), - opts: &LogoutOptions{}, - }, { name: "hostname, one host", opts: &LogoutOptions{ diff --git a/pkg/cmd/auth/refresh/refresh.go b/pkg/cmd/auth/refresh/refresh.go index 7fe5a8c5f..939b6f5a6 100644 --- a/pkg/cmd/auth/refresh/refresh.go +++ b/pkg/cmd/auth/refresh/refresh.go @@ -1,6 +1,7 @@ package refresh import ( + "errors" "fmt" "github.com/AlecAivazis/survey/v2" @@ -49,6 +50,17 @@ func NewCmdRefresh(f *cmdutil.Factory, runF func(*RefreshOptions) error) *cobra. # => open a browser to ensure your authentication credentials have the correct minimum scopes `), RunE: func(cmd *cobra.Command, args []string) error { + isTTY := opts.IO.IsStdinTTY() && opts.IO.IsStdoutTTY() + + if !isTTY { + return fmt.Errorf("not attached to a terminal; in headless environments, GITHUB_TOKEN is recommended") + } + + if opts.Hostname == "" && !opts.IO.CanPrompt() { + // here, we know we are attached to a TTY but prompts are disabled + return &cmdutil.FlagError{Err: errors.New("--hostname required when prompts disabled")} + } + if runF != nil { return runF(opts) } @@ -64,12 +76,6 @@ func NewCmdRefresh(f *cmdutil.Factory, runF func(*RefreshOptions) error) *cobra. } func refreshRun(opts *RefreshOptions) error { - isTTY := opts.IO.IsStdinTTY() && opts.IO.IsStdoutTTY() - - if !isTTY { - return fmt.Errorf("not attached to a terminal; in headless environments, GITHUB_TOKEN is recommended") - } - cfg, err := opts.Config() if err != nil { return err diff --git a/pkg/cmd/auth/refresh/refresh_test.go b/pkg/cmd/auth/refresh/refresh_test.go index faacb0aa7..cba3c107a 100644 --- a/pkg/cmd/auth/refresh/refresh_test.go +++ b/pkg/cmd/auth/refresh/refresh_test.go @@ -14,34 +14,51 @@ import ( "github.com/stretchr/testify/assert" ) +// TODO prompt cfg test + func Test_NewCmdRefresh(t *testing.T) { tests := []struct { - name string - cli string - wants RefreshOptions + name string + cli string + wants RefreshOptions + wantsErr bool + tty bool }{ { - name: "no arguments", + name: "tty no arguments", + tty: true, wants: RefreshOptions{ Hostname: "", }, }, { - name: "hostname", + name: "nontty no arguments", + wantsErr: true, + }, + { + name: "nontty hostname", + cli: "-h aline.cedrac", + wantsErr: true, + }, + { + name: "tty hostname", + tty: true, cli: "-h aline.cedrac", wants: RefreshOptions{ Hostname: "aline.cedrac", }, }, { - name: "one scope", + name: "tty one scope", + tty: true, cli: "--scopes repo:invite", wants: RefreshOptions{ Scopes: []string{"repo:invite"}, }, }, { - name: "scopes", + name: "tty scopes", + tty: true, cli: "--scopes repo:invite,read:public_key", wants: RefreshOptions{ Scopes: []string{"repo:invite", "read:public_key"}, @@ -55,6 +72,8 @@ func Test_NewCmdRefresh(t *testing.T) { f := &cmdutil.Factory{ IOStreams: io, } + io.SetStdinTTY(tt.tty) + io.SetStdoutTTY(tt.tty) argv, err := shlex.Split(tt.cli) assert.NoError(t, err) @@ -73,11 +92,14 @@ func Test_NewCmdRefresh(t *testing.T) { cmd.SetErr(&bytes.Buffer{}) _, err = cmd.ExecuteC() + if tt.wantsErr { + assert.Error(t, err) + return + } assert.NoError(t, err) assert.Equal(t, tt.wants.Hostname, gotOpts.Hostname) assert.Equal(t, tt.wants.Scopes, gotOpts.Scopes) }) - } } @@ -96,12 +118,6 @@ func Test_refreshRun(t *testing.T) { nontty bool wantAuthArgs authArgs }{ - { - name: "non tty", - opts: &RefreshOptions{}, - nontty: true, - wantErr: regexp.MustCompile(`not attached to a terminal;`), - }, { name: "no hosts configured", opts: &RefreshOptions{}, diff --git a/pkg/cmd/issue/create/create.go b/pkg/cmd/issue/create/create.go index 4190fba3f..b91d5786c 100644 --- a/pkg/cmd/issue/create/create.go +++ b/pkg/cmd/issue/create/create.go @@ -157,6 +157,10 @@ func createRun(opts *CreateOptions) error { return fmt.Errorf("must provide --title and --body when not attached to a terminal") } + if interactive && !opts.IO.CanPrompt() { + return fmt.Errorf("must provide --title and --body when prompts are disabled") + } + if interactive { var legacyTemplateFile *string if opts.RepoOverride == "" { diff --git a/pkg/cmd/pr/create/create.go b/pkg/cmd/pr/create/create.go index 233cdb38e..d4b55e1de 100644 --- a/pkg/cmd/pr/create/create.go +++ b/pkg/cmd/pr/create/create.go @@ -75,9 +75,8 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co opts.BodyProvided = cmd.Flags().Changed("body") opts.RepoOverride, _ = cmd.Flags().GetString("repo") - isTerminal := opts.IO.IsStdinTTY() && opts.IO.IsStdoutTTY() - if !isTerminal && !opts.WebMode && !opts.TitleProvided && !opts.Autofill { - return errors.New("--title or --fill required when not attached to a terminal") + if !opts.IO.CanPrompt() && !opts.WebMode && !opts.TitleProvided && !opts.Autofill { + return errors.New("--title or --fill required when prompts are disabled") } if opts.IsDraft && opts.WebMode { diff --git a/pkg/cmd/pr/create/create_test.go b/pkg/cmd/pr/create/create_test.go index f3426702f..4d915778f 100644 --- a/pkg/cmd/pr/create/create_test.go +++ b/pkg/cmd/pr/create/create_test.go @@ -135,7 +135,7 @@ func TestPRCreate_nontty_insufficient_flags(t *testing.T) { t.Fatal("expected error") } - assert.Equal(t, "--title or --fill required when not attached to a terminal", err.Error()) + assert.Equal(t, "--title or --fill required when prompts are disabled", err.Error()) assert.Equal(t, "", output.String()) } diff --git a/pkg/cmd/pr/merge/merge.go b/pkg/cmd/pr/merge/merge.go index 94e0c959c..ff21a6039 100644 --- a/pkg/cmd/pr/merge/merge.go +++ b/pkg/cmd/pr/merge/merge.go @@ -86,8 +86,8 @@ func NewCmdMerge(f *cmdutil.Factory, runF func(*MergeOptions) error) *cobra.Comm methodFlags++ } if methodFlags == 0 { - if !opts.IO.IsStdoutTTY() || !opts.IO.IsStdinTTY() { - return &cmdutil.FlagError{Err: errors.New("--merge, --rebase, or --squash required when not attached to a terminal")} + if !opts.IO.CanPrompt() { + return &cmdutil.FlagError{Err: errors.New("--merge, --rebase, or --squash required when prompts are disabled")} } opts.InteractiveMode = true } else if methodFlags > 1 { diff --git a/pkg/cmd/pr/merge/merge_test.go b/pkg/cmd/pr/merge/merge_test.go index 6abea963c..1949a2d78 100644 --- a/pkg/cmd/pr/merge/merge_test.go +++ b/pkg/cmd/pr/merge/merge_test.go @@ -53,7 +53,7 @@ func Test_NewCmdMerge(t *testing.T) { name: "insufficient flags in non-interactive mode", args: "123", isTTY: false, - wantErr: "--merge, --rebase, or --squash required when not attached to a terminal", + wantErr: "--merge, --rebase, or --squash required when prompts are disabled", }, { name: "multiple merge methods", diff --git a/pkg/cmd/pr/review/review.go b/pkg/cmd/pr/review/review.go index 19f33ca84..a76a39573 100644 --- a/pkg/cmd/pr/review/review.go +++ b/pkg/cmd/pr/review/review.go @@ -104,8 +104,8 @@ func NewCmdReview(f *cmdutil.Factory, runF func(*ReviewOptions) error) *cobra.Co } if found == 0 && opts.Body == "" { - if !opts.IO.IsStdoutTTY() || !opts.IO.IsStdinTTY() { - return &cmdutil.FlagError{Err: errors.New("--approve, --request-changes, or --comment required when not attached to a tty")} + if !opts.IO.CanPrompt() { + return &cmdutil.FlagError{Err: errors.New("--approve, --request-changes, or --comment required when prompts are disabled")} } opts.InteractiveMode = true } else if found == 0 && opts.Body != "" { diff --git a/pkg/cmd/pr/review/review_test.go b/pkg/cmd/pr/review/review_test.go index 1dfe4a53d..2f4a66f83 100644 --- a/pkg/cmd/pr/review/review_test.go +++ b/pkg/cmd/pr/review/review_test.go @@ -60,7 +60,7 @@ func Test_NewCmdReview(t *testing.T) { name: "no arguments in non-interactive mode", args: "", isTTY: false, - wantErr: "--approve, --request-changes, or --comment required when not attached to a tty", + wantErr: "--approve, --request-changes, or --comment required when prompts are disabled", }, { name: "mutually exclusive review types", diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index fc53b460e..22d1ce3a6 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -73,6 +73,15 @@ func NewCmdCreate(f *cmdutil.Factory, runF func(*CreateOptions) error) *cobra.Co opts.Name = args[0] } + if !opts.IO.CanPrompt() { + if opts.Name == "" { + return &cmdutil.FlagError{Err: errors.New("must pass name argument when prompts disabled")} + } + if !opts.Internal && !opts.Private && !opts.Public { + opts.Public = true + } + } + if runF != nil { return runF(opts) } @@ -264,7 +273,7 @@ func createRun(opts *CreateOptions) error { if isTTY { fmt.Fprintf(stderr, "%s Added remote %s\n", greenCheck, remoteURL) } - } else if isTTY { + } else if opts.IO.CanPrompt() { doSetup := createLocalDirectory if !doSetup { err := prompt.Confirm(fmt.Sprintf("Create a local project directory for %s?", ghrepo.FullName(repo)), &doSetup) diff --git a/pkg/cmd/repo/create/create_test.go b/pkg/cmd/repo/create/create_test.go index 8eb1a2423..1fd99c035 100644 --- a/pkg/cmd/repo/create/create_test.go +++ b/pkg/cmd/repo/create/create_test.go @@ -22,6 +22,8 @@ import ( func runCommand(httpClient *http.Client, cli string) (*test.CmdOut, error) { io, _, stdout, stderr := iostreams.Test() + io.SetStdoutTTY(true) + io.SetStdinTTY(true) fac := &cmdutil.Factory{ IOStreams: io, HttpClient: func() (*http.Client, error) { @@ -109,8 +111,8 @@ func TestRepoCreate(t *testing.T) { t.Errorf("error running command `repo create`: %v", err) } - assert.Equal(t, "https://github.com/OWNER/REPO\n", output.String()) - assert.Equal(t, "", output.Stderr()) + assert.Equal(t, "", output.String()) + assert.Equal(t, "✓ Created repository OWNER/REPO on GitHub\n✓ Added remote https://github.com/OWNER/REPO.git\n", output.Stderr()) if seenCmd == nil { t.Fatal("expected a command to run") @@ -191,8 +193,8 @@ func TestRepoCreate_org(t *testing.T) { t.Errorf("error running command `repo create`: %v", err) } - assert.Equal(t, "https://github.com/ORG/REPO\n", output.String()) - assert.Equal(t, "", output.Stderr()) + assert.Equal(t, "", output.String()) + assert.Equal(t, "✓ Created repository ORG/REPO on GitHub\n✓ Added remote https://github.com/ORG/REPO.git\n", output.Stderr()) if seenCmd == nil { t.Fatal("expected a command to run") @@ -273,8 +275,8 @@ func TestRepoCreate_orgWithTeam(t *testing.T) { t.Errorf("error running command `repo create`: %v", err) } - assert.Equal(t, "https://github.com/ORG/REPO\n", output.String()) - assert.Equal(t, "", output.Stderr()) + assert.Equal(t, "", output.String()) + assert.Equal(t, "✓ Created repository ORG/REPO on GitHub\n✓ Added remote https://github.com/ORG/REPO.git\n", output.Stderr()) if seenCmd == nil { t.Fatal("expected a command to run") @@ -363,8 +365,8 @@ func TestRepoCreate_template(t *testing.T) { t.Errorf("error running command `repo create`: %v", err) } - assert.Equal(t, "https://github.com/OWNER/REPO\n", output.String()) - assert.Equal(t, "", output.Stderr()) + assert.Equal(t, "", output.String()) + assert.Equal(t, "✓ Created repository OWNER/REPO on GitHub\n✓ Added remote https://github.com/OWNER/REPO.git\n", output.Stderr()) if seenCmd == nil { t.Fatal("expected a command to run") diff --git a/pkg/cmd/repo/fork/fork.go b/pkg/cmd/repo/fork/fork.go index 04f76fbb8..b22e2501c 100644 --- a/pkg/cmd/repo/fork/fork.go +++ b/pkg/cmd/repo/fork/fork.go @@ -55,7 +55,7 @@ func NewCmdFork(f *cmdutil.Factory, runF func(*ForkOptions) error) *cobra.Comman With no argument, creates a fork of the current repository. Otherwise, forks the specified repository.`, RunE: func(cmd *cobra.Command, args []string) error { - promptOk := opts.IO.IsStdoutTTY() && opts.IO.IsStdinTTY() + promptOk := opts.IO.CanPrompt() if len(args) > 0 { opts.Repository = args[0] }