From 4c412bc88ccd2067e1f6ae50fcc28fc6e4e63791 Mon Sep 17 00:00:00 2001 From: bchadwic Date: Tue, 29 Jun 2021 22:26:41 -0700 Subject: [PATCH 1/6] 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 2/6] 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 47314a6bbcaf99439301564f07aeab196ddf22c8 Mon Sep 17 00:00:00 2001 From: bchadwic Date: Sat, 3 Jul 2021 17:09:25 -0700 Subject: [PATCH 3/6] 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 4/6] 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 5/6] 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 6/6] 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)