X Tutup
Skip to content

Commit 7d55ab1

Browse files
vilmibmvilmibm
committed
review feedback
1 parent d204ae7 commit 7d55ab1

File tree

9 files changed

+55
-51
lines changed

9 files changed

+55
-51
lines changed

‎pkg/cmd/job/view/view.go‎

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -58,11 +58,9 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman
5858
// support `-R, --repo` override
5959
opts.BaseRepo = f.BaseRepo
6060

61-
terminal := opts.IO.IsStdoutTTY() && opts.IO.IsStdinTTY()
62-
6361
if len(args) > 0 {
6462
opts.JobID = args[0]
65-
} else if !terminal {
63+
} else if !opts.IO.CanPrompt() {
6664
return &cmdutil.FlagError{Err: errors.New("job ID required when not running interactively")}
6765
} else {
6866
opts.Prompt = true
@@ -76,7 +74,7 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman
7674
}
7775
cmd.Flags().BoolVarP(&opts.Log, "log", "l", false, "Print full logs for job")
7876
// TODO should we try and expose pending via another exit code?
79-
cmd.Flags().BoolVarP(&opts.ExitStatus, "exit-status", "e", false, "Exit with non-zero status if job failed")
77+
cmd.Flags().BoolVar(&opts.ExitStatus, "exit-status", false, "Exit with non-zero status if job failed")
8078

8179
return cmd
8280
}
@@ -107,14 +105,14 @@ func runView(opts *ViewOptions) error {
107105
fmt.Fprintln(out)
108106

109107
opts.IO.StartProgressIndicator()
108+
defer opts.IO.StopProgressIndicator()
110109

111110
run, err := shared.GetRun(client, repo, runID)
112111
if err != nil {
113112
return fmt.Errorf("failed to get run: %w", err)
114113
}
115114

116115
opts.IO.StopProgressIndicator()
117-
118116
jobID, err = promptForJob(*opts, client, repo, *run)
119117
if err != nil {
120118
return err
@@ -124,21 +122,16 @@ func runView(opts *ViewOptions) error {
124122
}
125123

126124
opts.IO.StartProgressIndicator()
127-
128125
job, err := getJob(client, repo, jobID)
129126
if err != nil {
130127
return fmt.Errorf("failed to get job: %w", err)
131128
}
132129

133-
opts.IO.StopProgressIndicator()
134-
135130
if opts.Log {
136-
opts.IO.StartProgressIndicator()
137131
r, err := client.JobLog(repo, jobID)
138132
if err != nil {
139133
return err
140134
}
141-
opts.IO.StopProgressIndicator()
142135

143136
if _, err := io.Copy(out, r); err != nil {
144137
return fmt.Errorf("failed to read log: %w", err)
@@ -152,29 +145,31 @@ func runView(opts *ViewOptions) error {
152145
}
153146

154147
annotations, err := shared.GetAnnotations(client, repo, *job)
148+
opts.IO.StopProgressIndicator()
155149
if err != nil {
156150
return fmt.Errorf("failed to get annotations: %w", err)
157151
}
158152

159-
opts.IO.StopProgressIndicator()
160-
161153
elapsed := job.CompletedAt.Sub(job.StartedAt)
162154
elapsedStr := fmt.Sprintf(" in %s", elapsed)
163155
if elapsed < 0 {
164156
elapsedStr = ""
165157
}
166158

159+
symbol, symColor := shared.Symbol(cs, job.Status, job.Conclusion)
160+
167161
fmt.Fprintf(out, "%s (ID %s)\n", cs.Bold(job.Name), cs.Cyanf("%d", job.ID))
168162
fmt.Fprintf(out, "%s %s ago%s\n",
169-
shared.Symbol(cs, job.Status, job.Conclusion),
163+
symColor(symbol),
170164
utils.FuzzyAgoAbbr(opts.Now(), job.StartedAt),
171165
elapsedStr)
172166

173167
fmt.Fprintln(out)
174168

175169
for _, step := range job.Steps {
170+
stepSym, stepSymColor := shared.Symbol(cs, step.Status, step.Conclusion)
176171
fmt.Fprintf(out, "%s %s\n",
177-
shared.Symbol(cs, step.Status, step.Conclusion),
172+
stepSymColor(stepSym),
178173
step.Name)
179174
}
180175

@@ -227,8 +222,8 @@ func promptForJob(opts ViewOptions, client *api.Client, repo ghrepo.Interface, r
227222
candidates := []string{}
228223

229224
for _, job := range jobs {
230-
symbol := shared.Symbol(cs, job.Status, job.Conclusion)
231-
candidates = append(candidates, fmt.Sprintf("%s %s", symbol, job.Name))
225+
symbol, symColor := shared.Symbol(cs, job.Status, job.Conclusion)
226+
candidates = append(candidates, fmt.Sprintf("%s %s", symColor(symbol), job.Name))
232227
}
233228

234229
// TODO consider custom filter so it's fuzzier. right now matches start anywhere in string but

‎pkg/cmd/job/view/view_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@ func TestNewCmdView(t *testing.T) {
6262
},
6363
{
6464
name: "exit status",
65-
cli: "-e 1234",
65+
cli: "--exit-status 1234",
6666
wants: ViewOptions{
6767
JobID: "1234",
6868
ExitStatus: true,

‎pkg/cmd/run/list/list.go‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,6 @@ func NewCmdList(f *cmdutil.Factory, runF func(*ListOptions) error) *cobra.Comman
6363
}
6464

6565
func listRun(opts *ListOptions) error {
66-
opts.IO.StartProgressIndicator()
6766
baseRepo, err := opts.BaseRepo()
6867
if err != nil {
6968
return fmt.Errorf("failed to determine base repo: %w", err)
@@ -75,31 +74,33 @@ func listRun(opts *ListOptions) error {
7574
}
7675
client := api.NewClientFromHTTP(c)
7776

77+
opts.IO.StartProgressIndicator()
7878
runs, err := shared.GetRuns(client, baseRepo, opts.Limit)
79+
opts.IO.StopProgressIndicator()
7980
if err != nil {
8081
return fmt.Errorf("failed to get runs: %w", err)
8182
}
8283

8384
tp := utils.NewTablePrinter(opts.IO)
8485

8586
cs := opts.IO.ColorScheme()
86-
out := opts.IO.Out
87-
88-
opts.IO.StopProgressIndicator()
8987

9088
if len(runs) == 0 {
9189
if !opts.PlainOutput {
92-
fmt.Fprintln(out, "No runs found")
90+
fmt.Fprintln(opts.IO.ErrOut, "No runs found")
9391
}
9492
return nil
9593
}
9694

95+
out := opts.IO.Out
96+
9797
for _, run := range runs {
9898
if opts.PlainOutput {
9999
tp.AddField(string(run.Status), nil, nil)
100100
tp.AddField(string(run.Conclusion), nil, nil)
101101
} else {
102-
tp.AddField(shared.Symbol(cs, run.Status, run.Conclusion), nil, nil)
102+
symbol, symbolColor := shared.Symbol(cs, run.Status, run.Conclusion)
103+
tp.AddField(symbol, nil, symbolColor)
103104
}
104105

105106
tp.AddField(run.CommitMsg(), nil, cs.Bold)

‎pkg/cmd/run/list/list_test.go‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -80,11 +80,12 @@ func TestNewCmdList(t *testing.T) {
8080

8181
func TestListRun(t *testing.T) {
8282
tests := []struct {
83-
name string
84-
opts *ListOptions
85-
wantOut string
86-
stubs func(*httpmock.Registry)
87-
nontty bool
83+
name string
84+
opts *ListOptions
85+
wantOut string
86+
wantErrOut string
87+
stubs func(*httpmock.Registry)
88+
nontty bool
8889
}{
8990
{
9091
name: "blank tty",
@@ -167,7 +168,8 @@ func TestListRun(t *testing.T) {
167168
httpmock.JSONResponse(shared.RunsPayload{}),
168169
)
169170
},
170-
wantOut: "No runs found\n",
171+
wantOut: "",
172+
wantErrOut: "No runs found\n",
171173
},
172174
}
173175

@@ -180,7 +182,7 @@ func TestListRun(t *testing.T) {
180182
return &http.Client{Transport: reg}, nil
181183
}
182184

183-
io, _, stdout, _ := iostreams.Test()
185+
io, _, stdout, stderr := iostreams.Test()
184186
io.SetStdoutTTY(!tt.nontty)
185187
tt.opts.IO = io
186188
tt.opts.BaseRepo = func() (ghrepo.Interface, error) {
@@ -191,6 +193,7 @@ func TestListRun(t *testing.T) {
191193
assert.NoError(t, err)
192194

193195
assert.Equal(t, tt.wantOut, stdout.String())
196+
assert.Equal(t, tt.wantErrOut, stderr.String())
194197
reg.Verify(t)
195198
})
196199
}

‎pkg/cmd/run/shared/shared.go‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -210,7 +210,7 @@ func PromptForRun(cs *iostreams.ColorScheme, client *api.Client, repo ghrepo.Int
210210
candidates := []string{}
211211

212212
for _, run := range runs {
213-
symbol := Symbol(cs, run.Status, run.Conclusion)
213+
symbol, _ := Symbol(cs, run.Status, run.Conclusion)
214214
candidates = append(candidates,
215215
fmt.Sprintf("%s %s, %s (%s)", symbol, run.CommitMsg(), run.Name, run.HeadBranch))
216216
}
@@ -243,17 +243,20 @@ func GetRun(client *api.Client, repo ghrepo.Interface, runID string) (*Run, erro
243243
return &result, nil
244244
}
245245

246-
func Symbol(cs *iostreams.ColorScheme, status Status, conclusion Conclusion) string {
246+
type colorFunc func(string) string
247+
248+
func Symbol(cs *iostreams.ColorScheme, status Status, conclusion Conclusion) (string, colorFunc) {
249+
noColor := func(s string) string { return s }
247250
if status == Completed {
248251
switch conclusion {
249252
case Success:
250-
return cs.SuccessIcon()
253+
return cs.SuccessIconWithColor(noColor), cs.Green
251254
case Skipped, Cancelled, Neutral:
252-
return cs.SuccessIconWithColor(cs.Gray)
255+
return cs.SuccessIconWithColor(noColor), cs.Gray
253256
default:
254-
return cs.FailureIcon()
257+
return cs.FailureIconWithColor(noColor), cs.Red
255258
}
256259
}
257260

258-
return cs.Yellow("-")
261+
return "-", cs.Yellow
259262
}

‎pkg/cmd/run/view/view.go‎

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -55,11 +55,9 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman
5555
// support `-R, --repo` override
5656
opts.BaseRepo = f.BaseRepo
5757

58-
terminal := opts.IO.IsStdoutTTY() && opts.IO.IsStdinTTY()
59-
6058
if len(args) > 0 {
6159
opts.RunID = args[0]
62-
} else if !terminal {
60+
} else if !opts.IO.CanPrompt() {
6361
return &cmdutil.FlagError{Err: errors.New("run ID required when not running interactively")}
6462
} else {
6563
opts.Prompt = true
@@ -73,7 +71,7 @@ func NewCmdView(f *cmdutil.Factory, runF func(*ViewOptions) error) *cobra.Comman
7371
}
7472
cmd.Flags().BoolVarP(&opts.Verbose, "verbose", "v", false, "Show job steps")
7573
// TODO should we try and expose pending via another exit code?
76-
cmd.Flags().BoolVarP(&opts.ExitStatus, "exit-status", "e", false, "Exit with non-zero status if run failed")
74+
cmd.Flags().BoolVar(&opts.ExitStatus, "exit-status", false, "Exit with non-zero status if run failed")
7775

7876
return cmd
7977
}
@@ -101,6 +99,7 @@ func runView(opts *ViewOptions) error {
10199
}
102100

103101
opts.IO.StartProgressIndicator()
102+
defer opts.IO.StopProgressIndicator()
104103

105104
run, err := shared.GetRun(client, repo, runID)
106105
if err != nil {
@@ -130,22 +129,21 @@ func runView(opts *ViewOptions) error {
130129
annotations = append(annotations, as...)
131130
}
132131

132+
opts.IO.StopProgressIndicator()
133133
if annotationErr != nil {
134134
return fmt.Errorf("failed to get annotations: %w", annotationErr)
135135
}
136136

137-
opts.IO.StopProgressIndicator()
138-
139137
out := opts.IO.Out
140138
cs := opts.IO.ColorScheme()
141139

142140
title := fmt.Sprintf("%s %s%s",
143141
cs.Bold(run.HeadBranch), run.Name, prNumber)
144-
symbol := shared.Symbol(cs, run.Status, run.Conclusion)
142+
symbol, symbolColor := shared.Symbol(cs, run.Status, run.Conclusion)
145143
id := cs.Cyanf("%d", run.ID)
146144

147145
fmt.Fprintln(out)
148-
fmt.Fprintf(out, "%s %s · %s\n", symbol, title, id)
146+
fmt.Fprintf(out, "%s %s · %s\n", symbolColor(symbol), title, id)
149147

150148
ago := opts.Now().Sub(run.CreatedAt)
151149

@@ -169,14 +167,13 @@ func runView(opts *ViewOptions) error {
169167
fmt.Fprintln(out, cs.Bold("JOBS"))
170168

171169
for _, job := range jobs {
172-
symbol := shared.Symbol(cs, job.Status, job.Conclusion)
170+
symbol, symbolColor := shared.Symbol(cs, job.Status, job.Conclusion)
173171
id := cs.Cyanf("%d", job.ID)
174-
fmt.Fprintf(out, "%s %s (ID %s)\n", symbol, job.Name, id)
172+
fmt.Fprintf(out, "%s %s (ID %s)\n", symbolColor(symbol), job.Name, id)
175173
if opts.Verbose || shared.IsFailureState(job.Conclusion) {
176174
for _, step := range job.Steps {
177-
fmt.Fprintf(out, " %s %s\n",
178-
shared.Symbol(cs, step.Status, step.Conclusion),
179-
step.Name)
175+
stepSymbol, stepSymColor := shared.Symbol(cs, step.Status, step.Conclusion)
176+
fmt.Fprintf(out, " %s %s\n", stepSymColor(stepSymbol), step.Name)
180177
}
181178
}
182179
}

‎pkg/cmd/run/view/view_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ func TestNewCmdView(t *testing.T) {
3838
},
3939
{
4040
name: "exit status",
41-
cli: "-e 1234",
41+
cli: "--exit-status 1234",
4242
wants: ViewOptions{
4343
RunID: "1234",
4444
ExitStatus: true,

‎pkg/iostreams/color.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,11 @@ func (c *ColorScheme) WarningIcon() string {
143143
}
144144

145145
func (c *ColorScheme) FailureIcon() string {
146-
return c.Red("X")
146+
return c.FailureIconWithColor(c.Red)
147+
}
148+
149+
func (c *ColorScheme) FailureIconWithColor(colo func(string) string) string {
150+
return colo("X")
147151
}
148152

149153
func (c *ColorScheme) ColorFromString(s string) func(string) string {

‎utils/table_printer.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ func (t ttyTablePrinter) IsTTY() bool {
4444
return true
4545
}
4646

47+
// Never pass pre-colorized text to AddField; always specify colorFunc. Otherwise, the table printer can't correctly compute the width of its columns.
4748
func (t *ttyTablePrinter) AddField(s string, truncateFunc func(int, string) string, colorFunc func(string) string) {
4849
if truncateFunc == nil {
4950
truncateFunc = text.Truncate

0 commit comments

Comments
 (0)
X Tutup