Actions Support Phase 1 #2923
Actions Support Phase 1 #2923
Conversation
This comment has been hidden.
This comment has been hidden.
This comment has been hidden.
This comment has been hidden.
This commit adds: gh actions gh run list gh run view gh job view as part of our first round of actions support. These commands are unlisted and considered in beta.
|
I left a bunch of minor comments/questions, but overall this is great and I didn't notice anything major. I played around with it a bit and everything worked as expected. Nice work! |
| if len(args) > 0 { | ||
| opts.JobID = args[0] | ||
| } else if !terminal { | ||
| return &cmdutil.FlagError{Err: errors.New("expected a job ID")} |
samcoe
Mar 5, 2021
Member
Maybe change to job ID required when not running interactively since that matches our other commands.
Maybe change to job ID required when not running interactively since that matches our other commands.
| }, | ||
| } | ||
| cmd.Flags().BoolVarP(&opts.Log, "log", "l", false, "Print full logs for job") | ||
| // TODO should we try and expose pending via another exit code? |
samcoe
Mar 5, 2021
Member
I can see this feature being useful for scripts that want to poll for job completion.
I can see this feature being useful for scripts that want to poll for job completion.
| // the cleanest way to do that. | ||
| fmt.Fprintln(out) | ||
|
|
||
| if opts.ShowProgress { |
samcoe
Mar 5, 2021
Member
Due to the fact that opts.ShowProgress is equal to terminal on line 62 and if opts.Prompt is true then terminal must be true due to line 68, these conditionals for opts.ShowProgress are not necessary inside the larger opts.Prompt conditional and will always return true.
Due to the fact that opts.ShowProgress is equal to terminal on line 62 and if opts.Prompt is true then terminal must be true due to line 68, these conditionals for opts.ShowProgress are not necessary inside the larger opts.Prompt conditional and will always return true.
| for { | ||
| buff := make([]byte, 64) | ||
| n, err := r.Read(buff) | ||
| if n <= 0 { | ||
| break | ||
| } | ||
| fmt.Fprint(out, string(buff[0:n])) | ||
| if err != nil { | ||
| break | ||
| } | ||
| } |
samcoe
Mar 5, 2021
Member
I think this could be simplified a bit by using a bufio Reader (https://golang.org/pkg/bufio/#Reader).
I think this could be simplified a bit by using a bufio Reader (https://golang.org/pkg/bufio/#Reader).
| if opts.ExitStatus && shared.IsFailureState(job.Conclusion) { | ||
| return cmdutil.SilentError | ||
| } | ||
|
|
||
| return nil |
samcoe
Mar 5, 2021
Member
In the opts.Log branch it does not seem that opts.IO.StopProgressIndicator() is ever called.
In the opts.Log branch it does not seem that opts.IO.StopProgressIndicator() is ever called.
| case "failure": | ||
| return cs.FailureIcon() | ||
| case "warning": | ||
| return cs.Yellow("!") |
samcoe
Mar 5, 2021
Member
I think we can use cs.WarningIcon() here.
I think we can use cs.WarningIcon() here.
| if len(args) > 0 { | ||
| opts.RunID = args[0] | ||
| } else if !terminal { | ||
| return &cmdutil.FlagError{Err: errors.New("expected a run ID")} |
samcoe
Mar 5, 2021
Member
Same comment as above about matching other commands messaging here.
Same comment as above about matching other commands messaging here.
| }, | ||
| } | ||
| cmd.Flags().BoolVarP(&opts.Verbose, "verbose", "v", false, "Show job steps") | ||
| // TODO should we try and expose pending via another exit code? |
samcoe
Mar 5, 2021
Member
Same comment as above, I think this would be a useful feature.
Same comment as above, I think this would be a useful feature.
| fmt.Fprintln(out) | ||
| fmt.Fprintf(out, "For more information, see: %s\n", cs.Bold(run.URL)) | ||
|
|
||
| if opts.ExitStatus && shared.IsFailureState(run.Conclusion) { |
samcoe
Mar 5, 2021
Member
I don't think we need to check shared.IsFailureState(run.Conclusion) here as it in encapsulated in the conditional above.
I don't think we need to check shared.IsFailureState(run.Conclusion) here as it in encapsulated in the conditional above.
| for _, job := range jobs { | ||
| symbol := shared.Symbol(cs, job.Status, job.Conclusion) | ||
| id := cs.Cyanf("%d", job.ID) | ||
| fmt.Fprintf(out, "%s %s (ID %s)\n", symbol, job.Name, id) | ||
| if opts.Verbose || shared.IsFailureState(job.Conclusion) { | ||
| for _, step := range job.Steps { | ||
| fmt.Fprintf(out, " %s %s\n", | ||
| shared.Symbol(cs, step.Status, step.Conclusion), | ||
| step.Name) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if len(annotations) > 0 { | ||
| fmt.Fprintln(out) | ||
| fmt.Fprintln(out, cs.Bold("ANNOTATIONS")) | ||
|
|
||
| for _, a := range annotations { | ||
| fmt.Fprintf(out, "%s %s\n", a.Symbol(cs), a.Message) | ||
| fmt.Fprintln(out, cs.Grayf("%s: %s#%d\n", | ||
| a.JobName, a.Path, a.StartLine)) | ||
| } | ||
| } |
samcoe
Mar 5, 2021
Member
It seems outputting info of jobs and annotations is also done in job view, I don't know if it is necessary now but might be good to DRY that up if we end up having to output this info in another command.
It seems outputting info of jobs and annotations is also done in job view, I don't know if it is necessary now but might be good to DRY that up if we end up having to output this info in another command.
Part of #2889
This PR introduces four new commands, all unlisted until we make full actions support GA:
gh actionsA purely informational command that acts as an entry point for learning about gh+actionsgh run listLists all runs associated with a repositorygh run viewView a specific run, including jobs+stepsgh job viewView a specific job, including support for getting a full job logThe two view commands both default to an interactive picker for recent runs if no argument is provided;
gh job viewdrills down from recent runs -> jobs -> job.To see them in use, check out this narrated demo video. Note that a few cosmetic changes have happened since the demo was recorded.
I recommend triggering some runs in a test repo to further evaluate these or use them to drill down in existing real runs in
cli/cli.