[gh run watch] Support `--compact` flag by iamazeem · Pull Request #10629 · cli/cli

iamazeem · 2025-03-18T11:44:39Z

babakks

Thanks for the work on this PR, @iamazeem! 🎉

While I was reviewing the PR, I noticed there is no A/C on the issue, and also the expected experience described in the issue needs to be improved. I have commented on the issue and I'll wait for you and others in the team to have your thoughts on it. Then we can come back to this PR and give it a review.

babakks · 2025-05-21T12:10:39Z

pkg/cmd/run/shared/presentation.go

 			for _, step := range job.Steps {
 				stepSymbol, stepSymColor := Symbol(cs, step.Status, step.Conclusion)
-				lines = append(lines, fmt.Sprintf("  %s %s", stepSymColor(stepSymbol), step.Name))
+
+				if compact {
+					if step.Status == InProgress || step.Status == Completed {
+						stepLogs = append(stepLogs, fmt.Sprintf("  %s %s", stepSymColor(stepSymbol), step.Name))
+
+						if len(stepLogs) > 1 {
+							stepLogs = stepLogs[len(stepLogs)-1:]
+						}
+
+						if IsFailureState(step.Conclusion) {
+							break
+						}
+					}
+				} else {
+					stepLogs = append(stepLogs, fmt.Sprintf("  %s %s", stepSymColor(stepSymbol), step.Name))
+				}


As @BagToad and I were checking the PR, we decided that it's best to have two separate functions for rendering in normal and compact modes. It'd be easier to read the code, and less conflated.

…cli into 4535-gh-run-watch-compact-flag

iamazeem · 2025-05-22T09:50:29Z

@babakks: Changes added according to the updated AC. Please review. Thanks!

babakks

Thanks for the changes, @iamazeem! 🙏

It looks good and addresses the A/C. Thanks again!

However, it still needs a couple of small improvements, and also one test case in TestWatchRun to ensure the behaviour is as expected.

babakks · 2025-05-22T13:00:34Z

pkg/cmd/run/shared/presentation.go

 	return strings.Join(lines, "\n")
 }

+func RenderJobsInCompactMode(cs *iostreams.ColorScheme, jobs []Job) string {


To make it less verbose, please:
s/RenderJobsInCompactMode/RenderJobsCompact

babakks · 2025-05-22T13:13:16Z

pkg/cmd/run/shared/presentation.go

+		if job.Status == InProgress {
+			lines = append(lines, inProgressStepLine)
+		}


I think this if-statement is a bit confusing, since we already now the step is still in progress. Instead we should check if inProgressStepLine is not empty:

Suggested change

if job.Status == InProgress {

lines = append(lines, inProgressStepLine)

}

if inProgressStepLine != "" {

lines = append(lines, inProgressStepLine)

}

babakks · 2025-05-22T13:51:48Z

pkg/cmd/run/watch/watch.go

+			By default, all the steps are shown with each refresh.
+			With the %[1]s--compact%[1]s flag:
+			- For a successfully completed job, not steps are shown.
+			- For a failed job, all the failed steps are shown.
+			- For an ongoing job with no failed steps, only the currently running step is shown.
+			- For an ongoing job with failed steps, the failed steps along with the currently running step are shown.


IMO the user doesn't need to see the details of the --compact mode here. Just saying it removes irrelevant steps from the output is enough.

Suggested change

By default, all the steps are shown with each refresh.

With the %[1]s--compact%[1]s flag:

- For a successfully completed job, not steps are shown.

- For a failed job, all the failed steps are shown.

- For an ongoing job with no failed steps, only the currently running step is shown.

- For an ongoing job with failed steps, the failed steps along with the currently running step are shown.

By default, all steps are displayed. The %[1]s--compact%[1]s option can be used

to only show the relevant/failed steps.

babakks · 2025-05-22T13:52:43Z

pkg/cmd/run/watch/watch.go

+			# Watch a run in compact mode
+			$ gh run watch --compact


nitpick: This can be pulled up to appear after the simple gh run watch example.

babakks · 2025-05-22T13:54:14Z

pkg/cmd/run/watch/watch.go

 		},
 	}
 	cmd.Flags().BoolVar(&opts.ExitStatus, "exit-status", false, "Exit with non-zero status if run fails")
+	cmd.Flags().BoolVar(&opts.Compact, "compact", false, "Enable compact mode")


The description should be a bit more:

Suggested change

cmd.Flags().BoolVar(&opts.Compact, "compact", false, "Enable compact mode")

cmd.Flags().BoolVar(&opts.Compact, "compact", false, "Show only relevant/failed steps")

babakks · 2025-05-22T13:55:41Z

pkg/cmd/run/watch/watch.go

+		verbose := true
+		fmt.Fprintln(out, shared.RenderJobs(cs, jobs, verbose))


Temp var verbose is redundant here.

Suggested change

verbose := true

fmt.Fprintln(out, shared.RenderJobs(cs, jobs, verbose))

fmt.Fprintln(out, shared.RenderJobs(cs, jobs, true))

babakks · 2025-05-22T14:08:39Z

pkg/cmd/run/watch/watch_test.go

+		{
+			name: "compact status",
+			cli:  "1234 --compact",
+			wants: WatchOptions{
+				Interval: defaultInterval,
+				RunID:    "1234",
+				Compact:  true,
+			},
+		},


We also need a test case in TestWatchRun to cover the changes we made.

I think it's okay to just have a single test case that covers all cases. I know adding such tests is a bit annoying. 😄 So, let me know if I can help with that. 🙏

@babakks: Regarding tests, I looked into it.
Apparently, there needs to be separate tests for in-progress and completed scenarios. 🤔
It'd be great if you added tests. Thanks!

babakks

LGTM! 🎉

I'm going to add the test we talked about and will then merge this.

Signed-off-by: Babak K. Shandiz <babakks@github.com>

babakks · 2025-05-23T09:26:37Z

Since this might take a while and I'm going to add a couple of tests for the functions in presentation.go (and it'll need review from other folks), I'm merging this PR now. This PR is fine since it confirms with the A/C, and also the existing render function does not have proper tests, so this PR is not inconsistent with our current state.

UPDATE: Just pushed a quick docs improvement to this.

This MR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://github.com/cli/cli) | minor | `v2.73.0` -> `v2.74.1` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>cli/cli (cli/cli)</summary> ### [`v2.74.1`](https://github.com/cli/cli/releases/tag/v2.74.1): GitHub CLI 2.74.1 [Compare Source](cli/cli@v2.74.0...v2.74.1) #### What's Changed - Document support for `@copilot` in `gh [pr|issue] edit --add-assignee` and `--remove-assignee` by [@timrogers](https://github.com/timrogers) in cli/cli#11056 - Fix pr edit when URL is provided by [@williammartin](https://github.com/williammartin) in cli/cli#11057 - Fix const in MR finder tests by [@babakks](https://github.com/babakks) in cli/cli#11091 **Full Changelog**: cli/cli@v2.74.0...v2.74.1 ### [`v2.74.0`](https://github.com/cli/cli/releases/tag/v2.74.0): GitHub CLI 2.74.0 [Compare Source](cli/cli@v2.73.0...v2.74.0) #### Security A security vulnerability has been identified in a core `gh` dependency, `go-gh`, where an attacker-controlled GitHub Enterprise Server could result in executing arbitrary commands on a user's machine by replacing HTTP URLs provided by GitHub with local file paths for browsing. This issue is addressed in this `gh` release by updating `go-gh` to a fixed version. For more information, see GHSA-g9f5-x53j-h563 #### What's changed ##### ✨ Features - Add `preview prompter` command by [@BagToad](https://github.com/BagToad) in cli/cli#10745 - \[gh run watch] Support `--compact` flag by [@iamazeem](https://github.com/iamazeem) in cli/cli#10629 - Fix brew update notifications by [@BagToad](https://github.com/BagToad) in cli/cli#11024 ##### 🐛 Fixes - Revert "\[gh config] Escape pipe symbol in Long desc for website manual" by [@BagToad](https://github.com/BagToad) in cli/cli#11004 - Fix formatting in allowed values for `gh config --help` by [@BagToad](https://github.com/BagToad) in cli/cli#11003 - fix: `gh gist edit` panic when no file in a gist by [@phanen](https://github.com/phanen) in cli/cli#10627 - Add retry logic when fetching TUF content in `gh attestation` commands by [@malancas](https://github.com/malancas) in cli/cli#10943 ##### 📚 Docs & Chores - Update README.md by [@irhdab](https://github.com/irhdab) in cli/cli#11022 - Add tests for `RenderJobs` and `RenderJobsCompact` by [@babakks](https://github.com/babakks) in cli/cli#11013 - Add example usage of `--head` option to `pr list` docs by [@babakks](https://github.com/babakks) in cli/cli#10979 - Mention `pr create` will print the created MR's URL by [@babakks](https://github.com/babakks) in cli/cli#10980 - Add Digest to ReleaseAsset struct by [@bdehamer](https://github.com/bdehamer) in cli/cli#11030 #####

Dependencies - Bump `go-gh` to v2.12.1 by [@BagToad](https://github.com/BagToad) in cli/cli#11043 - chore(deps): bump github.com/gabriel-vasile/mimetype from 1.4.8 to 1.4.9 by [@dependabot](https://github.com/dependabot) in cli/cli#10825 - Update sigstore-go dependency to v1.0.0 by [@malancas](https://github.com/malancas) in cli/cli#11028 - chore(deps): bump github.com/sigstore/protobuf-specs from 0.4.1 to 0.4.2 by [@dependabot](https://github.com/dependabot) in cli/cli#10999 - chore(deps): bump github.com/yuin/goldmark from 1.7.8 to 1.7.12 by [@dependabot](https://github.com/dependabot) in cli/cli#11032 #### New Contributors - [@irhdab](https://github.com/irhdab) made their first contribution in cli/cli#11022 - [@phanen](https://github.com/phanen) made their first contribution in cli/cli#10627 **Full Changelog**: cli/cli@v2.73.0...v2.74.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).

[gh run watch] Support --compact flag

22e092a

iamazeem requested a review from a team as a code owner March 18, 2025 11:44

iamazeem requested a review from BagToad March 18, 2025 11:44

iamazeem temporarily deployed to cli-automation March 18, 2025 11:44 — with GitHub Actions Inactive

cliAutomation added the external pull request originating outside of the CLI core team label Mar 18, 2025

[gh run watch] Support --compact flag

37690a7

BagToad force-pushed the 4535-gh-run-watch-compact-flag branch from 22e092a to 37690a7 Compare April 14, 2025 17:17

babakks self-requested a review May 19, 2025 09:31

babakks reviewed May 19, 2025

View reviewed changes

babakks reviewed May 21, 2025

View reviewed changes

iamazeem added 3 commits May 22, 2025 11:42

Merge branch '4535-gh-run-watch-compact-flag' of github.com:iamazeem/…

4276a7f

…cli into 4535-gh-run-watch-compact-flag

Add changes for updated AC

b81b172

Merge branch 'trunk' into 4535-gh-run-watch-compact-flag

1754ead

iamazeem requested a review from babakks May 22, 2025 09:47

babakks requested changes May 22, 2025

View reviewed changes

Incorporate review changes

e3cabe2

iamazeem requested a review from babakks May 23, 2025 05:23

babakks approved these changes May 23, 2025

View reviewed changes

docs(run watch): fill up the line to 80 chars

221d537

Signed-off-by: Babak K. Shandiz <babakks@github.com>

babakks mentioned this pull request May 23, 2025

Add tests to presentation functions used in run watch and run view #11012

Closed

babakks merged commit 1e6a2b1 into cli:trunk May 23, 2025
9 checks passed

iamazeem deleted the 4535-gh-run-watch-compact-flag branch May 23, 2025 10:03

	cmd.Flags().BoolVar(&opts.Compact, "compact", false, "Enable compact mode")
	cmd.Flags().BoolVar(&opts.Compact, "compact", false, "Show only relevant/failed steps")

		verbose := true
		fmt.Fprintln(out, shared.RenderJobs(cs, jobs, verbose))

Conversation

iamazeem commented Mar 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

babakks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamazeem commented May 22, 2025

Uh oh!

babakks left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

babakks left a comment

Choose a reason for hiding this comment

Uh oh!

babakks commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

iamazeem commented Mar 18, 2025 •

edited

Loading

babakks commented May 23, 2025 •

edited

Loading