From b514d22f1ec142cf0d2964733e3abb57eb29d551 Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Tue, 29 Jun 2021 13:59:51 -0700 Subject: [PATCH 01/15] Fix issue in FindEntry that causes extensions and alias crash --- internal/config/config_map.go | 13 ++++-- internal/config/config_map_test.go | 65 ++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 internal/config/config_map_test.go diff --git a/internal/config/config_map.go b/internal/config/config_map.go index f6b25fcb6..8afaf3a4c 100644 --- a/internal/config/config_map.go +++ b/internal/config/config_map.go @@ -68,13 +68,18 @@ func (cm *ConfigMap) FindEntry(key string) (ce *ConfigEntry, err error) { ce = &ConfigEntry{} - topLevelKeys := cm.Root.Content - for i, v := range topLevelKeys { + // Content slice goes [key1, value1, key2, value2, ...] + topLevelPairs := cm.Root.Content + for i, v := range topLevelPairs { + // Skip every other slice item since we only want to check against keys + if i%2 != 0 { + continue + } if v.Value == key { ce.KeyNode = v ce.Index = i - if i+1 < len(topLevelKeys) { - ce.ValueNode = topLevelKeys[i+1] + if i+1 < len(topLevelPairs) { + ce.ValueNode = topLevelPairs[i+1] } return } diff --git a/internal/config/config_map_test.go b/internal/config/config_map_test.go new file mode 100644 index 000000000..c504e4cbc --- /dev/null +++ b/internal/config/config_map_test.go @@ -0,0 +1,65 @@ +package config + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + "gopkg.in/yaml.v3" +) + +func TestFindEntry(t *testing.T) { + tests := []struct { + name string + key string + output string + wantErr bool + }{ + { + name: "find key", + key: "valid", + output: "present", + }, + { + name: "find key that is not present", + key: "invalid", + wantErr: true, + }, + { + name: "find key with blank value", + key: "blank", + output: "", + }, + { + name: "find key that has same content as a value", + key: "same", + output: "logical", + }, + } + + for _, tt := range tests { + cm := ConfigMap{Root: testYaml()} + t.Run(tt.name, func(t *testing.T) { + out, err := cm.FindEntry(tt.key) + if tt.wantErr { + assert.EqualError(t, err, "not found") + return + } + assert.NoError(t, err) + fmt.Println(out) + assert.Equal(t, tt.output, out.ValueNode.Value) + }) + } +} + +func testYaml() *yaml.Node { + var root yaml.Node + var data = ` +valid: present +erroneous: same +blank: +same: logical +` + _ = yaml.Unmarshal([]byte(data), &root) + return root.Content[0] +} From 4c412bc88ccd2067e1f6ae50fcc28fc6e4e63791 Mon Sep 17 00:00:00 2001 From: bchadwic Date: Tue, 29 Jun 2021 22:26:41 -0700 Subject: [PATCH 02/15] Added in label rgb functionality for both prs and issues --- pkg/cmd/issue/shared/display.go | 2 +- pkg/cmd/pr/view/view.go | 2 +- pkg/iostreams/color.go | 8 ++++++++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/issue/shared/display.go b/pkg/cmd/issue/shared/display.go index ff9b83e1e..b7342fa9d 100644 --- a/pkg/cmd/issue/shared/display.go +++ b/pkg/cmd/issue/shared/display.go @@ -63,7 +63,7 @@ func IssueLabelList(issue api.Issue) string { labelNames := make([]string, 0, len(issue.Labels.Nodes)) for _, label := range issue.Labels.Nodes { - labelNames = append(labelNames, label.Name) + labelNames = append(labelNames, iostreams.RGB(label.Color, label.Name)) } return strings.Join(labelNames, ", ") diff --git a/pkg/cmd/pr/view/view.go b/pkg/cmd/pr/view/view.go index 905f7476d..9b98ef23d 100644 --- a/pkg/cmd/pr/view/view.go +++ b/pkg/cmd/pr/view/view.go @@ -374,7 +374,7 @@ func prLabelList(pr api.PullRequest) string { labelNames := make([]string, 0, len(pr.Labels.Nodes)) for _, label := range pr.Labels.Nodes { - labelNames = append(labelNames, label.Name) + labelNames = append(labelNames, iostreams.RGB(label.Color, label.Name)) } list := strings.Join(labelNames, ", ") diff --git a/pkg/iostreams/color.go b/pkg/iostreams/color.go index 2dedbdbd5..13d6db8e6 100644 --- a/pkg/iostreams/color.go +++ b/pkg/iostreams/color.go @@ -3,6 +3,7 @@ package iostreams import ( "fmt" "os" + "strconv" "strings" "github.com/mgutz/ansi" @@ -202,3 +203,10 @@ func (c *ColorScheme) ColorFromString(s string) func(string) string { return fn } + +func RGB(hex string, x string) string { + r, _ := strconv.ParseInt(hex[0:2], 16, 64) + g, _ := strconv.ParseInt(hex[2:4], 16, 64) + b, _ := strconv.ParseInt(hex[4:6], 16, 64) + return fmt.Sprintf("\033[38;2;%d;%d;%dm%s\033[0m", r, g, b, x) +} From af2499cb69d721c6a4418642216aed9669b5a6ed Mon Sep 17 00:00:00 2001 From: bchadwic Date: Tue, 29 Jun 2021 22:32:07 -0700 Subject: [PATCH 03/15] renamed func RGB to HexToRGB --- pkg/cmd/issue/shared/display.go | 2 +- pkg/cmd/pr/view/view.go | 2 +- pkg/iostreams/color.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/issue/shared/display.go b/pkg/cmd/issue/shared/display.go index b7342fa9d..983f22e16 100644 --- a/pkg/cmd/issue/shared/display.go +++ b/pkg/cmd/issue/shared/display.go @@ -63,7 +63,7 @@ func IssueLabelList(issue api.Issue) string { labelNames := make([]string, 0, len(issue.Labels.Nodes)) for _, label := range issue.Labels.Nodes { - labelNames = append(labelNames, iostreams.RGB(label.Color, label.Name)) + labelNames = append(labelNames, iostreams.HexToRGB(label.Color, label.Name)) } return strings.Join(labelNames, ", ") diff --git a/pkg/cmd/pr/view/view.go b/pkg/cmd/pr/view/view.go index 9b98ef23d..7cc9e0193 100644 --- a/pkg/cmd/pr/view/view.go +++ b/pkg/cmd/pr/view/view.go @@ -374,7 +374,7 @@ func prLabelList(pr api.PullRequest) string { labelNames := make([]string, 0, len(pr.Labels.Nodes)) for _, label := range pr.Labels.Nodes { - labelNames = append(labelNames, iostreams.RGB(label.Color, label.Name)) + labelNames = append(labelNames, iostreams.HexToRGB(label.Color, label.Name)) } list := strings.Join(labelNames, ", ") diff --git a/pkg/iostreams/color.go b/pkg/iostreams/color.go index 13d6db8e6..b8d64fffc 100644 --- a/pkg/iostreams/color.go +++ b/pkg/iostreams/color.go @@ -204,7 +204,7 @@ func (c *ColorScheme) ColorFromString(s string) func(string) string { return fn } -func RGB(hex string, x string) string { +func HexToRGB(hex string, x string) string { r, _ := strconv.ParseInt(hex[0:2], 16, 64) g, _ := strconv.ParseInt(hex[2:4], 16, 64) b, _ := strconv.ParseInt(hex[4:6], 16, 64) From f82750dfe62ef83cf7b868f14467bc9c3c7ec613 Mon Sep 17 00:00:00 2001 From: chemotaxis Date: Thu, 1 Jul 2021 03:04:02 -0400 Subject: [PATCH 04/15] Update `goreleaser` config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `nfpms.files` is deprecated: ```shell goreleaser version 0.172.1 commit: 32a44ab928879bb32c1e266b80de32e07d5d6721 ``` Before this commit, `goreleaser check` prints this: ```shell $goreleaser check • loading config file file=.goreleaser.yml ⨯ command failed error=yaml: unmarshal errors: line 67: field files not found in type config.NFPM ``` --- .goreleaser.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.goreleaser.yml b/.goreleaser.yml index 2f6d16f32..137981363 100644 --- a/.goreleaser.yml +++ b/.goreleaser.yml @@ -64,5 +64,6 @@ nfpms: formats: - deb - rpm - files: - "./share/man/man1/gh*.1": "/usr/share/man/man1" + contents: + - src: "/share/man/man1/gh*.1" + dst: "/usr/share/man/man1" From 103e18cab548c7a9116a0bfe139f1285f0edfa49 Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Thu, 1 Jul 2021 13:45:29 -0700 Subject: [PATCH 05/15] Disallow installing extensions with same name as gh command --- pkg/cmd/extensions/command.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/pkg/cmd/extensions/command.go b/pkg/cmd/extensions/command.go index f764e22e5..7abba2586 100644 --- a/pkg/cmd/extensions/command.go +++ b/pkg/cmd/extensions/command.go @@ -80,6 +80,17 @@ func NewCmdExtensions(f *cmdutil.Factory) *cobra.Command { if !strings.HasPrefix(repo.RepoName(), "gh-") { return errors.New("the repository name must start with `gh-`") } + + commandName := strings.TrimPrefix(repo.RepoName(), "gh-") + rootCmd := cmd.Root() + c, _, err := rootCmd.Traverse([]string{commandName}) + if err != nil { + return err + } + if c != rootCmd { + return fmt.Errorf("could not install %s: %s is already a gh command", args[0], commandName) + } + cfg, err := f.Config() if err != nil { return err From ab675a33f319df327e6bbfc0b9603e003ce83cff Mon Sep 17 00:00:00 2001 From: chemotaxis Date: Thu, 1 Jul 2021 18:41:13 -0400 Subject: [PATCH 06/15] Upgrade GoReleaser Now that the config file is updated, upgrade from v0.169.0 to v0.172.1. --- .github/workflows/releases.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/releases.yml b/.github/workflows/releases.yml index b8f880964..d1f8e69f2 100644 --- a/.github/workflows/releases.yml +++ b/.github/workflows/releases.yml @@ -23,7 +23,7 @@ jobs: - name: Run GoReleaser uses: goreleaser/goreleaser-action@v2 with: - version: v0.169.0 # pinning bc our config breaks on latest + version: v0.172.1 # pinning to prevent breaking on latest args: release --release-notes=CHANGELOG.md env: GITHUB_TOKEN: ${{secrets.GITHUB_TOKEN}} From 0482e5cd9b6c30bdf3f6e7cfe365dcb8233383ad Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Thu, 1 Jul 2021 13:51:13 -0700 Subject: [PATCH 07/15] Disallow installing extensions with the same name --- pkg/cmd/extensions/command.go | 7 ++++- pkg/cmd/extensions/command_test.go | 43 +++++++++++++++++++++--------- 2 files changed, 36 insertions(+), 14 deletions(-) diff --git a/pkg/cmd/extensions/command.go b/pkg/cmd/extensions/command.go index 7abba2586..d06d9dba8 100644 --- a/pkg/cmd/extensions/command.go +++ b/pkg/cmd/extensions/command.go @@ -88,7 +88,12 @@ func NewCmdExtensions(f *cmdutil.Factory) *cobra.Command { return err } if c != rootCmd { - return fmt.Errorf("could not install %s: %s is already a gh command", args[0], commandName) + return fmt.Errorf("could not install %s: %s is already a command", args[0], commandName) + } + for _, ext := range m.List() { + if ext.Name() == commandName { + return fmt.Errorf("could not install %s: %s is already an installed extension", args[0], commandName) + } } cfg, err := f.Config() diff --git a/pkg/cmd/extensions/command_test.go b/pkg/cmd/extensions/command_test.go index 7bf0b4429..ebc6959fa 100644 --- a/pkg/cmd/extensions/command_test.go +++ b/pkg/cmd/extensions/command_test.go @@ -2,7 +2,6 @@ package extensions import ( "io" - "io/ioutil" "os" "strings" "testing" @@ -25,23 +24,43 @@ func TestNewCmdExtensions(t *testing.T) { args []string managerStubs func(em *extensions.ExtensionManagerMock) func(*testing.T) wantErr bool - wantStdout string - wantStderr string + errMsg string }{ { name: "install an extension", args: []string{"install", "owner/gh-some-ext"}, managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) { + em.ListFunc = func() []extensions.Extension { + return []extensions.Extension{} + } em.InstallFunc = func(s string, out, errOut io.Writer) error { return nil } return func(t *testing.T) { - calls := em.InstallCalls() - assert.Equal(t, 1, len(calls)) - assert.Equal(t, "https://github.com/owner/gh-some-ext.git", calls[0].URL) + installCalls := em.InstallCalls() + assert.Equal(t, 1, len(installCalls)) + assert.Equal(t, "https://github.com/owner/gh-some-ext.git", installCalls[0].URL) + listCalls := em.ListCalls() + assert.Equal(t, 1, len(listCalls)) } }, }, + { + name: "install an extension with same name as existing extension", + args: []string{"install", "owner/gh-existing-ext"}, + managerStubs: func(em *extensions.ExtensionManagerMock) func(*testing.T) { + em.ListFunc = func() []extensions.Extension { + e := &Extension{path: "owner2/gh-existing-ext"} + return []extensions.Extension{e} + } + return func(t *testing.T) { + calls := em.ListCalls() + assert.Equal(t, 1, len(calls)) + } + }, + wantErr: true, + errMsg: "could not install owner/gh-existing-ext: existing-ext is already an installed extension", + }, { name: "install local extension", args: []string{"install", "."}, @@ -60,6 +79,7 @@ func TestNewCmdExtensions(t *testing.T) { name: "upgrade error", args: []string{"upgrade"}, wantErr: true, + errMsg: "must specify an extension to upgrade", }, { name: "upgrade an extension", @@ -107,7 +127,7 @@ func TestNewCmdExtensions(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - ios, _, stdout, stderr := iostreams.Test() + ios, _, _, _ := iostreams.Test() var assertFunc func(*testing.T) em := &extensions.ExtensionManagerMock{} @@ -125,12 +145,12 @@ func TestNewCmdExtensions(t *testing.T) { cmd := NewCmdExtensions(&f) cmd.SetArgs(tt.args) - cmd.SetOut(ioutil.Discard) - cmd.SetErr(ioutil.Discard) + cmd.SetOut(io.Discard) + cmd.SetErr(io.Discard) _, err := cmd.ExecuteC() if tt.wantErr { - assert.Error(t, err) + assert.EqualError(t, err, tt.errMsg) } else { assert.NoError(t, err) } @@ -138,9 +158,6 @@ func TestNewCmdExtensions(t *testing.T) { if assertFunc != nil { assertFunc(t) } - - assert.Equal(t, tt.wantStdout, stdout.String()) - assert.Equal(t, tt.wantStderr, stderr.String()) }) } } From 16d218508d8afb7035fcd0978f8c6b34d3437a68 Mon Sep 17 00:00:00 2001 From: Sam Coe Date: Fri, 2 Jul 2021 13:53:26 -0700 Subject: [PATCH 08/15] Fix tests --- pkg/cmd/extensions/command_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/extensions/command_test.go b/pkg/cmd/extensions/command_test.go index ebc6959fa..9e7243ef1 100644 --- a/pkg/cmd/extensions/command_test.go +++ b/pkg/cmd/extensions/command_test.go @@ -2,6 +2,7 @@ package extensions import ( "io" + "io/ioutil" "os" "strings" "testing" @@ -145,8 +146,8 @@ func TestNewCmdExtensions(t *testing.T) { cmd := NewCmdExtensions(&f) cmd.SetArgs(tt.args) - cmd.SetOut(io.Discard) - cmd.SetErr(io.Discard) + cmd.SetOut(ioutil.Discard) + cmd.SetErr(ioutil.Discard) _, err := cmd.ExecuteC() if tt.wantErr { From 49ff0c65308e8a1911bb4e6df80c16b78e09c226 Mon Sep 17 00:00:00 2001 From: Evan Silberman Date: Fri, 2 Jul 2021 17:12:16 -0700 Subject: [PATCH 09/15] Add a no-browser mode to gh browse For when you just want the destination URL on stdout. --- pkg/cmd/browse/browse.go | 21 ++++++++++++++------- pkg/cmd/browse/browse_test.go | 33 ++++++++++++++++++++++++++++++--- 2 files changed, 44 insertions(+), 10 deletions(-) diff --git a/pkg/cmd/browse/browse.go b/pkg/cmd/browse/browse.go index e7b998e81..6487d1944 100644 --- a/pkg/cmd/browse/browse.go +++ b/pkg/cmd/browse/browse.go @@ -26,10 +26,11 @@ type BrowseOptions struct { SelectorArg string - Branch string - ProjectsFlag bool - SettingsFlag bool - WikiFlag bool + Branch string + ProjectsFlag bool + SettingsFlag bool + WikiFlag bool + NoBrowserFlag bool } func NewCmdBrowse(f *cmdutil.Factory, runF func(*BrowseOptions) error) *cobra.Command { @@ -99,6 +100,7 @@ func NewCmdBrowse(f *cmdutil.Factory, runF func(*BrowseOptions) error) *cobra.Co cmd.Flags().BoolVarP(&opts.ProjectsFlag, "projects", "p", false, "Open repository projects") cmd.Flags().BoolVarP(&opts.WikiFlag, "wiki", "w", false, "Open repository wiki") cmd.Flags().BoolVarP(&opts.SettingsFlag, "settings", "s", false, "Open repository settings") + cmd.Flags().BoolVarP(&opts.NoBrowserFlag, "no-browser", "n", false, "Print destination URL instead of opening the browser") cmd.Flags().StringVarP(&opts.Branch, "branch", "b", "", "Select another branch by passing in the branch name") return cmd @@ -151,10 +153,15 @@ func runBrowse(opts *BrowseOptions) error { } } - if opts.IO.IsStdoutTTY() { - fmt.Fprintf(opts.IO.Out, "now opening %s in browser\n", url) + if opts.NoBrowserFlag { + _, err := fmt.Fprintf(opts.IO.Out, "%s\n", url) + return err + } else { + if opts.IO.IsStdoutTTY() { + fmt.Fprintf(opts.IO.Out, "now opening %s in browser\n", url) + } + return opts.Browser.Browse(url) } - return opts.Browser.Browse(url) } func parseFileArg(fileArg string) (string, error) { diff --git a/pkg/cmd/browse/browse_test.go b/pkg/cmd/browse/browse_test.go index aff46a1ad..d30a95024 100644 --- a/pkg/cmd/browse/browse_test.go +++ b/pkg/cmd/browse/browse_test.go @@ -1,6 +1,7 @@ package browse import ( + "fmt" "net/http" "testing" @@ -49,6 +50,14 @@ func TestNewCmdBrowse(t *testing.T) { }, wantsErr: false, }, + { + name: "no browser flag", + cli: "--no-browser", + wants: BrowseOptions{ + NoBrowserFlag: true, + }, + wantsErr: false, + }, { name: "branch flag", cli: "--branch main", @@ -118,6 +127,7 @@ func TestNewCmdBrowse(t *testing.T) { assert.Equal(t, tt.wants.SelectorArg, opts.SelectorArg) assert.Equal(t, tt.wants.ProjectsFlag, opts.ProjectsFlag) assert.Equal(t, tt.wants.WikiFlag, opts.WikiFlag) + assert.Equal(t, tt.wants.NoBrowserFlag, opts.NoBrowserFlag) assert.Equal(t, tt.wants.SettingsFlag, opts.SettingsFlag) }) } @@ -233,6 +243,17 @@ func Test_runBrowse(t *testing.T) { wantsErr: false, expectedURL: "https://github.com/github/ThankYouGitHub/tree/first-browse-pull/browse.go#L32", }, + { + name: "no browser with branch file and line number", + opts: BrowseOptions{ + Branch: "3-0-stable", + SelectorArg: "init.rb:6", + NoBrowserFlag: true, + }, + baseRepo: ghrepo.New("mislav", "will_paginate"), + wantsErr: false, + expectedURL: "https://github.com/mislav/will_paginate/tree/3-0-stable/init.rb#L6", + }, } for _, tt := range tests { @@ -263,9 +284,15 @@ func Test_runBrowse(t *testing.T) { assert.NoError(t, err) } - assert.Equal(t, "", stdout.String()) - assert.Equal(t, "", stderr.String()) - browser.Verify(t, tt.expectedURL) + if opts.NoBrowserFlag { + assert.Equal(t, fmt.Sprintf("%s\n", tt.expectedURL), stdout.String()) + assert.Equal(t, "", stderr.String()) + browser.Verify(t, "") + } else { + assert.Equal(t, "", stdout.String()) + assert.Equal(t, "", stderr.String()) + browser.Verify(t, tt.expectedURL) + } }) } } From 47314a6bbcaf99439301564f07aeab196ddf22c8 Mon Sep 17 00:00:00 2001 From: bchadwic Date: Sat, 3 Jul 2021 17:09:25 -0700 Subject: [PATCH 10/15] modified HexToRGB to check whether terminal and gh have color enabled, as well as created tests for HexToRGB --- pkg/cmd/issue/shared/display.go | 6 ++-- pkg/cmd/issue/view/view.go | 8 ++--- pkg/cmd/pr/view/view.go | 8 ++--- pkg/iostreams/color.go | 6 +++- pkg/iostreams/color_test.go | 53 +++++++++++++++++++++++++++++++++ 5 files changed, 69 insertions(+), 12 deletions(-) diff --git a/pkg/cmd/issue/shared/display.go b/pkg/cmd/issue/shared/display.go index 983f22e16..f75e445b1 100644 --- a/pkg/cmd/issue/shared/display.go +++ b/pkg/cmd/issue/shared/display.go @@ -22,7 +22,7 @@ func PrintIssues(io *iostreams.IOStreams, prefix string, totalCount int, issues issueNum = "#" + issueNum } issueNum = prefix + issueNum - labels := IssueLabelList(issue) + labels := IssueLabelList(issue, cs) if labels != "" && table.IsTTY() { labels = fmt.Sprintf("(%s)", labels) } @@ -56,14 +56,14 @@ func truncateLabels(w int, t string) string { return fmt.Sprintf("(%s)", truncated) } -func IssueLabelList(issue api.Issue) string { +func IssueLabelList(issue api.Issue, cs *iostreams.ColorScheme) string { if len(issue.Labels.Nodes) == 0 { return "" } labelNames := make([]string, 0, len(issue.Labels.Nodes)) for _, label := range issue.Labels.Nodes { - labelNames = append(labelNames, iostreams.HexToRGB(label.Color, label.Name)) + labelNames = append(labelNames, cs.HexToRGB(label.Color, label.Name)) } return strings.Join(labelNames, ", ") diff --git a/pkg/cmd/issue/view/view.go b/pkg/cmd/issue/view/view.go index d4f96bbd8..a2a75e4b7 100644 --- a/pkg/cmd/issue/view/view.go +++ b/pkg/cmd/issue/view/view.go @@ -125,7 +125,7 @@ func viewRun(opts *ViewOptions) error { return nil } - return printRawIssuePreview(opts.IO.Out, issue) + return printRawIssuePreview(opts.IO.Out, issue, opts.IO.ColorScheme()) } func findIssue(client *http.Client, baseRepoFn func() (ghrepo.Interface, error), selector string, loadComments bool) (*api.Issue, error) { @@ -141,9 +141,9 @@ func findIssue(client *http.Client, baseRepoFn func() (ghrepo.Interface, error), return issue, err } -func printRawIssuePreview(out io.Writer, issue *api.Issue) error { +func printRawIssuePreview(out io.Writer, issue *api.Issue, cs *iostreams.ColorScheme) error { assignees := issueAssigneeList(*issue) - labels := shared.IssueLabelList(*issue) + labels := shared.IssueLabelList(*issue, cs) projects := issueProjectList(*issue) // Print empty strings for empty values so the number of metadata lines is consistent when @@ -193,7 +193,7 @@ func printHumanIssuePreview(opts *ViewOptions, issue *api.Issue) error { fmt.Fprint(out, cs.Bold("Assignees: ")) fmt.Fprintln(out, assignees) } - if labels := shared.IssueLabelList(*issue); labels != "" { + if labels := shared.IssueLabelList(*issue, cs); labels != "" { fmt.Fprint(out, cs.Bold("Labels: ")) fmt.Fprintln(out, labels) } diff --git a/pkg/cmd/pr/view/view.go b/pkg/cmd/pr/view/view.go index 7cc9e0193..2c390d412 100644 --- a/pkg/cmd/pr/view/view.go +++ b/pkg/cmd/pr/view/view.go @@ -139,7 +139,7 @@ func printRawPrPreview(io *iostreams.IOStreams, pr *api.PullRequest) error { reviewers := prReviewerList(*pr, cs) assignees := prAssigneeList(*pr) - labels := prLabelList(*pr) + labels := prLabelList(*pr, cs) projects := prProjectList(*pr) fmt.Fprintf(out, "title:\t%s\n", pr.Title) @@ -197,7 +197,7 @@ func printHumanPrPreview(opts *ViewOptions, pr *api.PullRequest) error { fmt.Fprint(out, cs.Bold("Assignees: ")) fmt.Fprintln(out, assignees) } - if labels := prLabelList(*pr); labels != "" { + if labels := prLabelList(*pr, cs); labels != "" { fmt.Fprint(out, cs.Bold("Labels: ")) fmt.Fprintln(out, labels) } @@ -367,14 +367,14 @@ func prAssigneeList(pr api.PullRequest) string { return list } -func prLabelList(pr api.PullRequest) string { +func prLabelList(pr api.PullRequest, cs *iostreams.ColorScheme) string { if len(pr.Labels.Nodes) == 0 { return "" } labelNames := make([]string, 0, len(pr.Labels.Nodes)) for _, label := range pr.Labels.Nodes { - labelNames = append(labelNames, iostreams.HexToRGB(label.Color, label.Name)) + labelNames = append(labelNames, cs.HexToRGB(label.Color, label.Name)) } list := strings.Join(labelNames, ", ") diff --git a/pkg/iostreams/color.go b/pkg/iostreams/color.go index b8d64fffc..72c4643a5 100644 --- a/pkg/iostreams/color.go +++ b/pkg/iostreams/color.go @@ -204,7 +204,11 @@ func (c *ColorScheme) ColorFromString(s string) func(string) string { return fn } -func HexToRGB(hex string, x string) string { +func (c *ColorScheme) HexToRGB(hex string, x string) string { + if !c.enabled || !c.is256enabled { + return x + } + r, _ := strconv.ParseInt(hex[0:2], 16, 64) g, _ := strconv.ParseInt(hex[2:4], 16, 64) b, _ := strconv.ParseInt(hex[4:6], 16, 64) diff --git a/pkg/iostreams/color_test.go b/pkg/iostreams/color_test.go index 90b3ae024..7923bf912 100644 --- a/pkg/iostreams/color_test.go +++ b/pkg/iostreams/color_test.go @@ -3,6 +3,8 @@ package iostreams import ( "os" "testing" + + "github.com/stretchr/testify/assert" ) func TestEnvColorDisabled(t *testing.T) { @@ -143,3 +145,54 @@ func TestEnvColorForced(t *testing.T) { }) } } + +func Test_HextoRGB(t *testing.T) { + tests := []struct { + name string + hex string + arg string + expectedOutput string + expectedError bool + cs *ColorScheme + }{ + { + name: "Colored red enabled color", + hex: "fc0303", + arg: "red", + expectedOutput: "\033[38;2;252;3;3mred\033[0m", + cs: NewColorScheme(true, true), + }, + { + name: "Failed colored red enabled color", + hex: "fc0303", + arg: "red", + expectedOutput: "\033[38;2;252;2;3mred\033[0m", + expectedError: true, + cs: NewColorScheme(true, true), + }, + { + name: "Colored red disabled color", + hex: "fc0303", + arg: "red", + expectedOutput: "red", + cs: NewColorScheme(false, false), + }, + { + name: "Failed colored red disabled color", + hex: "fc0303", + arg: "red", + expectedOutput: "\033[38;2;252;3;3mred\033[0m", + expectedError: true, + cs: NewColorScheme(false, false), + }, + } + + for _, tt := range tests { + output := tt.cs.HexToRGB(tt.hex, tt.arg) + if tt.expectedError { + assert.NotEqual(t, tt.expectedOutput, output) + } else { + assert.Equal(t, tt.expectedOutput, output) + } + } +} From 1980cc83b9dc93dae11d653fadff73944a0f79e5 Mon Sep 17 00:00:00 2001 From: Des Preston Date: Fri, 9 Jul 2021 11:54:58 -0400 Subject: [PATCH 11/15] return SilentError if completed run failed If `gh run watch ${ID} --exit-status` is run and "ID" is the ID of a completed job that failed, return a SilentError. This ensures that the program returns a non-zero code. Fixes #3962 --- pkg/cmd/run/watch/watch.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/pkg/cmd/run/watch/watch.go b/pkg/cmd/run/watch/watch.go index 70c7d7558..5b51b37ac 100644 --- a/pkg/cmd/run/watch/watch.go +++ b/pkg/cmd/run/watch/watch.go @@ -123,6 +123,9 @@ func watchRun(opts *WatchOptions) error { if run.Status == shared.Completed { fmt.Fprintf(out, "Run %s (%s) has already completed with '%s'\n", cs.Bold(run.Name), cs.Cyanf("%d", run.ID), run.Conclusion) + if opts.ExitStatus && run.Conclusion != shared.Success { + return cmdutil.SilentError + } return nil } From 13037226c232659db87aa44c0bf99ad3a5b769a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Mon, 12 Jul 2021 16:58:45 +0200 Subject: [PATCH 12/15] Add test for `gh run watch --exit-status` with completed runs --- pkg/cmd/run/watch/watch_test.go | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/pkg/cmd/run/watch/watch_test.go b/pkg/cmd/run/watch/watch_test.go index 4152aad52..8b02ea24d 100644 --- a/pkg/cmd/run/watch/watch_test.go +++ b/pkg/cmd/run/watch/watch_test.go @@ -190,6 +190,21 @@ func TestWatchRun(t *testing.T) { }, wantOut: "Run failed (1234) has already completed with 'failure'\n", }, + { + name: "already completed, exit status", + opts: &WatchOptions{ + RunID: "1234", + ExitStatus: true, + }, + httpStubs: func(reg *httpmock.Registry) { + reg.Register( + httpmock.REST("GET", "repos/OWNER/REPO/actions/runs/1234"), + httpmock.JSONResponse(shared.FailedRun)) + }, + wantOut: "Run failed (1234) has already completed with 'failure'\n", + wantErr: true, + errMsg: "SilentError", + }, { name: "prompt, no in progress runs", tty: true, @@ -307,13 +322,8 @@ func TestWatchRun(t *testing.T) { t.Run(tt.name, func(t *testing.T) { err := watchRun(tt.opts) if tt.wantErr { - assert.Error(t, err) - assert.Equal(t, tt.errMsg, err.Error()) - if !tt.opts.ExitStatus { - return - } - } - if !tt.opts.ExitStatus { + assert.EqualError(t, err, tt.errMsg) + } else { assert.NoError(t, err) } assert.Equal(t, tt.wantOut, stdout.String()) From 98d3b7cc795f622832b7305e571d8a4fe6bac65b Mon Sep 17 00:00:00 2001 From: nate smith Date: Mon, 12 Jul 2021 13:05:49 -0500 Subject: [PATCH 13/15] don't check Fprintf error we don't ever check the return of Fprintf anywhere else in the codebase so doing it here suggests that it's a special case. if it's something we should be doing we can circle back and do it more consistently. --- pkg/cmd/browse/browse.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/browse/browse.go b/pkg/cmd/browse/browse.go index 6487d1944..514477efc 100644 --- a/pkg/cmd/browse/browse.go +++ b/pkg/cmd/browse/browse.go @@ -154,8 +154,8 @@ func runBrowse(opts *BrowseOptions) error { } if opts.NoBrowserFlag { - _, err := fmt.Fprintf(opts.IO.Out, "%s\n", url) - return err + fmt.Fprintf(opts.IO.Out, "%s\n", url) + return nil } else { if opts.IO.IsStdoutTTY() { fmt.Fprintf(opts.IO.Out, "now opening %s in browser\n", url) From 17b58bf0b2006d671cdcc5555bd91c731f54322c Mon Sep 17 00:00:00 2001 From: Des Preston Date: Wed, 14 Jul 2021 09:57:59 -0400 Subject: [PATCH 14/15] fix repo create --confirm Respect the --confirm flag when deciding whether to prompt for gitignore and license creation during `repo create` Fixes #3989 --- pkg/cmd/repo/create/create.go | 28 +++++++------- pkg/cmd/repo/create/create_test.go | 61 ++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 13 deletions(-) diff --git a/pkg/cmd/repo/create/create.go b/pkg/cmd/repo/create/create.go index 7b9ec70d2..e4776c4e4 100644 --- a/pkg/cmd/repo/create/create.go +++ b/pkg/cmd/repo/create/create.go @@ -221,21 +221,23 @@ func createRun(opts *CreateOptions) error { return err } - // GitIgnore and License templates not added when a template repository is passed. - if gitIgnoreTemplate == "" && opts.Template == "" && opts.IO.CanPrompt() { - gt, err := interactiveGitIgnore(api.NewClientFromHTTP(httpClient), host) - if err != nil { - return err + // GitIgnore and License templates not added when a template repository + // is passed, or when the confirm flag is set. + if opts.Template == "" && opts.IO.CanPrompt() && !opts.ConfirmSubmit { + if gitIgnoreTemplate == "" { + gt, err := interactiveGitIgnore(api.NewClientFromHTTP(httpClient), host) + if err != nil { + return err + } + gitIgnoreTemplate = gt } - gitIgnoreTemplate = gt - } - - if repoLicenseTemplate == "" && opts.Template == "" && opts.IO.CanPrompt() { - lt, err := interactiveLicense(api.NewClientFromHTTP(httpClient), host) - if err != nil { - return err + if repoLicenseTemplate == "" { + lt, err := interactiveLicense(api.NewClientFromHTTP(httpClient), host) + if err != nil { + return err + } + repoLicenseTemplate = lt } - repoLicenseTemplate = lt } } diff --git a/pkg/cmd/repo/create/create_test.go b/pkg/cmd/repo/create/create_test.go index b74bbe8b2..70361a2ed 100644 --- a/pkg/cmd/repo/create/create_test.go +++ b/pkg/cmd/repo/create/create_test.go @@ -817,3 +817,64 @@ func TestRepoCreate_WithGitIgnore_Org(t *testing.T) { t.Errorf("expected %q, got %q", "OWNERID", ownerId) } } + +func TestRepoCreate_WithConfirmFlag(t *testing.T) { + cs, cmdTeardown := run.Stub() + defer cmdTeardown(t) + + cs.Register(`git remote add -f origin https://github\.com/OWNER/REPO\.git`, 0, "") + cs.Register(`git rev-parse --show-toplevel`, 0, "") + + reg := &httpmock.Registry{} + + reg.Register( + httpmock.GraphQL(`mutation RepositoryCreate\b`), + httpmock.StringResponse(` + { "data": { "createRepository": { + "repository": { + "id": "REPOID", + "url": "https://github.com/OWNER/REPO", + "name": "REPO", + "owner": { + "login": "OWNER" + } + } + } } }`), + ) + + reg.Register( + httpmock.REST("GET", "users/OWNER"), + httpmock.StringResponse(`{ "node_id": "OWNERID" }`), + ) + + httpClient := &http.Client{Transport: reg} + + in := "OWNER/REPO --confirm --private" + output, err := runCommand(httpClient, in, true) + if err != nil { + t.Errorf("error running command `repo create %v`: %v", in, err) + } + + 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()) + + var reqBody struct { + Query string + Variables struct { + Input map[string]interface{} + } + } + + if len(reg.Requests) != 2 { + t.Fatalf("expected 2 HTTP request, got %d", len(reg.Requests)) + } + + bodyBytes, _ := ioutil.ReadAll(reg.Requests[1].Body) + _ = json.Unmarshal(bodyBytes, &reqBody) + if repoName := reqBody.Variables.Input["name"].(string); repoName != "REPO" { + t.Errorf("expected %q, got %q", "REPO", repoName) + } + if repoVisibility := reqBody.Variables.Input["visibility"].(string); repoVisibility != "PRIVATE" { + t.Errorf("expected %q, got %q", "PRIVATE", repoVisibility) + } +} From d6b0749ea2f55ab1e00883ae98275d6316194894 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mislav=20Marohni=C4=87?= Date: Fri, 16 Jul 2021 14:52:05 +0200 Subject: [PATCH 15/15] Tweak error messages and add more tests for extension install name check --- pkg/cmd/extensions/command.go | 40 +++++++++------ pkg/cmd/extensions/command_test.go | 79 +++++++++++++++++++++++++++++- 2 files changed, 102 insertions(+), 17 deletions(-) diff --git a/pkg/cmd/extensions/command.go b/pkg/cmd/extensions/command.go index d06d9dba8..2bdf75b41 100644 --- a/pkg/cmd/extensions/command.go +++ b/pkg/cmd/extensions/command.go @@ -10,6 +10,7 @@ import ( "github.com/cli/cli/git" "github.com/cli/cli/internal/ghrepo" "github.com/cli/cli/pkg/cmdutil" + "github.com/cli/cli/pkg/extensions" "github.com/cli/cli/utils" "github.com/spf13/cobra" ) @@ -73,28 +74,14 @@ func NewCmdExtensions(f *cmdutil.Factory) *cobra.Command { } return m.InstallLocal(wd) } + repo, err := ghrepo.FromFullName(args[0]) if err != nil { return err } - if !strings.HasPrefix(repo.RepoName(), "gh-") { - return errors.New("the repository name must start with `gh-`") - } - - commandName := strings.TrimPrefix(repo.RepoName(), "gh-") - rootCmd := cmd.Root() - c, _, err := rootCmd.Traverse([]string{commandName}) - if err != nil { + if err := checkValidExtension(cmd.Root(), m, repo.RepoName()); err != nil { return err } - if c != rootCmd { - return fmt.Errorf("could not install %s: %s is already a command", args[0], commandName) - } - for _, ext := range m.List() { - if ext.Name() == commandName { - return fmt.Errorf("could not install %s: %s is already an installed extension", args[0], commandName) - } - } cfg, err := f.Config() if err != nil { @@ -145,3 +132,24 @@ func NewCmdExtensions(f *cmdutil.Factory) *cobra.Command { extCmd.Hidden = true return &extCmd } + +func checkValidExtension(rootCmd *cobra.Command, m extensions.ExtensionManager, extName string) error { + if !strings.HasPrefix(extName, "gh-") { + return errors.New("extension repository name must start with `gh-`") + } + + commandName := strings.TrimPrefix(extName, "gh-") + if c, _, err := rootCmd.Traverse([]string{commandName}); err != nil { + return err + } else if c != rootCmd { + return fmt.Errorf("%q matches the name of a built-in command", commandName) + } + + for _, ext := range m.List() { + if ext.Name() == commandName { + return fmt.Errorf("there is already an installed extension that provides the %q command", commandName) + } + } + + return nil +} diff --git a/pkg/cmd/extensions/command_test.go b/pkg/cmd/extensions/command_test.go index 9e7243ef1..41eae92b2 100644 --- a/pkg/cmd/extensions/command_test.go +++ b/pkg/cmd/extensions/command_test.go @@ -11,6 +11,7 @@ import ( "github.com/cli/cli/pkg/cmdutil" "github.com/cli/cli/pkg/extensions" "github.com/cli/cli/pkg/iostreams" + "github.com/spf13/cobra" "github.com/stretchr/testify/assert" ) @@ -60,7 +61,7 @@ func TestNewCmdExtensions(t *testing.T) { } }, wantErr: true, - errMsg: "could not install owner/gh-existing-ext: existing-ext is already an installed extension", + errMsg: "there is already an installed extension that provides the \"existing-ext\" command", }, { name: "install local extension", @@ -166,3 +167,79 @@ func TestNewCmdExtensions(t *testing.T) { func normalizeDir(d string) string { return strings.TrimPrefix(d, "/private") } + +func Test_checkValidExtension(t *testing.T) { + rootCmd := &cobra.Command{} + rootCmd.AddCommand(&cobra.Command{Use: "help"}) + rootCmd.AddCommand(&cobra.Command{Use: "auth"}) + + m := &extensions.ExtensionManagerMock{ + ListFunc: func() []extensions.Extension { + return []extensions.Extension{ + &extensions.ExtensionMock{ + NameFunc: func() string { return "screensaver" }, + }, + &extensions.ExtensionMock{ + NameFunc: func() string { return "triage" }, + }, + } + }, + } + + type args struct { + rootCmd *cobra.Command + manager extensions.ExtensionManager + extName string + } + tests := []struct { + name string + args args + wantError string + }{ + { + name: "valid extension", + args: args{ + rootCmd: rootCmd, + manager: m, + extName: "gh-hello", + }, + }, + { + name: "invalid extension name", + args: args{ + rootCmd: rootCmd, + manager: m, + extName: "gherkins", + }, + wantError: "extension repository name must start with `gh-`", + }, + { + name: "clashes with built-in command", + args: args{ + rootCmd: rootCmd, + manager: m, + extName: "gh-auth", + }, + wantError: "\"auth\" matches the name of a built-in command", + }, + { + name: "clashes with an installed extension", + args: args{ + rootCmd: rootCmd, + manager: m, + extName: "gh-triage", + }, + wantError: "there is already an installed extension that provides the \"triage\" command", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := checkValidExtension(tt.args.rootCmd, tt.args.manager, tt.args.extName) + if tt.wantError == "" { + assert.NoError(t, err) + } else { + assert.EqualError(t, err, tt.wantError) + } + }) + } +}