Show cancelled checks

[rajhawaldar]

Fixes #7551

[andyfeller]

@rajhawaldar : These changes look good, thank you for contributing to the GitHub CLI! ✨

Just because @williammartin weighed in on original issue, I want to check with him to see if any thing he'd call out 🙇

[williammartin]

So, over in #7551 (comment), we agreed that there would be no change to the non-TTY output, however this introduces a new bucket string of cancelled which results in an output change from:

➜  gh pr checks https://github.com/cli/cli/pull/7974 | cat
build (macos-latest)    fail    5s      https://github.com/cli/cli/actions/runs/6146985649/job/16705004740
build (ubuntu-latest)   fail    10s     https://github.com/cli/cli/actions/runs/6146985649/job/16705004372
build (windows-latest)  fail    5s      https://github.com/cli/cli/actions/runs/6146985649/job/16705004563
build (macos-latest)    pass    6m1s    https://github.com/cli/cli/actions/runs/6146967695/job/16677397574
build (ubuntu-latest)   pass    5m14s   https://github.com/cli/cli/actions/runs/6146967695/job/16677396916
build (windows-latest)  pass    14m36s  https://github.com/cli/cli/actions/runs/6146967695/job/16677397318
pr-auto pass    2s      https://github.com/cli/cli/actions/runs/6146985592/job/16704994604

to:

➜  ./bin/gh pr checks https://github.com/cli/cli/pull/7974 | cat
build (macos-latest)    cancel  5s      https://github.com/cli/cli/actions/runs/6146985649/job/16705004740
build (ubuntu-latest)   cancel  10s     https://github.com/cli/cli/actions/runs/6146985649/job/16705004372
build (windows-latest)  cancel  5s      https://github.com/cli/cli/actions/runs/6146985649/job/16705004563
build (macos-latest)    pass    6m1s    https://github.com/cli/cli/actions/runs/6146967695/job/16677397574
build (ubuntu-latest)   pass    5m14s   https://github.com/cli/cli/actions/runs/6146967695/job/16677396916
build (windows-latest)  pass    14m36s  https://github.com/cli/cli/actions/runs/6146967695/job/16677397318
pr-auto pass    2s      https://github.com/cli/cli/actions/runs/6146985592/job/16704994604

This has the potential to break scripts that depended on parsing out fail e.g. to simple re-run them. I think this is probably not ok, and we should ensure we make this backwards compatible on the non-TTY output and very clearly document in the code why we are doing this so that we can fix it in a later major version bump. I could be persuaded otherwise though, and I'd like @samcoe to weigh in.

Separately, I think this bucket should be cancelled rather than cancel here: https://github.com/cli/cli/pull/7960/files#diff-efc661aec6e8e9f77edef814b32d91c0ad2534e7270787f185e55ca7dd6b250bR78

[andyfeller]

Separately, I think this bucket should be cancelled rather than cancel here: https://github.com/cli/cli/pull/7960/files#diff-efc661aec6e8e9f77edef814b32d91c0ad2534e7270787f185e55ca7dd6b250bR78

The existing language tense is inconsistent:

• pass is present infinitive
• skipping is present participle
• fail is present infinitive
• cancel is present infinitive
• pending is present participle

So, I agree with canceled but also wonder why skipping wasn't skipped, fail wasn't failed, and pass wasn't passed 🤔

[samcoe]

@andyfeller We definitely should have done a better job of standardizing the verbiage used here, would like to see that changed in the future. I think this would be a breaking change and something we should not change without a major version release.

As for changing fail to cancel for the existing cases I am on the fence on if that is a breaking change or not. One one hand @williammartin is correct that it would break scripts, on the other hand it might be considered a bug that we were not properly representing the checks status in the first place. As pr checks command is meant to be an aggregation of checks based on our own custom logic I think that change should probably be consider breaking as it changes the aggregation behavior. For this PR I would suggest we keep fail in the non-TTY output to avoid this.

[andyfeller]

(@williammartin) I think this is probably not ok, and we should ensure we make this backwards compatible on the non-TTY output and very clearly document in the code why we are doing this so that we can fix it in a later major version bump.

(@samcoe) For this PR I would suggest we keep fail in the non-TTY output to avoid this.

@samcoe @williammartin: Putting this into more concrete terms, are we saying aggregateChecks logic should conditionalize on whether TTY or not?

cli/pkg/cmd/pr/checks/aggregate.go

Lines 31 to 88 in 3c9e0b0

	func aggregateChecks(checkContexts []api.CheckContext, requiredChecks bool) (checks []check, counts checkCounts) {
	for _, c := range eliminateDuplicates(checkContexts) {
	if requiredChecks && !c.IsRequired {
	continue
	}
	state := string(c.State)
	if state == "" {
	if c.Status == "COMPLETED" {
	state = string(c.Conclusion)
	} else {
	state = c.Status
	}
	}
	link := c.DetailsURL
	if link == "" {
	link = c.TargetURL
	}
	name := c.Name
	if name == "" {
	name = c.Context
	}
	item := check{
	Name: name,
	State: state,
	StartedAt: c.StartedAt,
	CompletedAt: c.CompletedAt,
	Link: link,
	Event: c.CheckSuite.WorkflowRun.Event,
	Workflow: c.CheckSuite.WorkflowRun.Workflow.Name,
	Description: c.Description,
	}
	switch state {
	case "SUCCESS":
	item.Bucket = "pass"
	counts.Passed++
	case "SKIPPED", "NEUTRAL":
	item.Bucket = "skipping"
	counts.Skipping++
	case "ERROR", "FAILURE", "TIMED_OUT", "ACTION_REQUIRED":
	item.Bucket = "fail"
	counts.Failed++
	case "CANCELLED":
	item.Bucket = "cancel"
	counts.Canceled++
	default: // "EXPECTED", "REQUESTED", "WAITING", "QUEUED", "PENDING", "IN_PROGRESS", "STALE"
	item.Bucket = "pending"
	counts.Pending++
	}
	checks = append(checks, item)
	}
	return
	}

There is already some conditional logic within printSummary based on whether TTY.

cli/pkg/cmd/pr/checks/output.go

Lines 67 to 87 in 3c9e0b0

	if counts.Failed+counts.Passed+counts.Skipping+counts.Pending > 0 {
	if counts.Failed > 0 {
	summary = "Some checks were not successful"
	} else if counts.Pending > 0 {
	summary = "Some checks are still pending"
	} else if counts.Canceled > 0 {
	summary = "Some checks were cancelled"
	} else {
	summary = "All checks were successful"
	}
	tallies := fmt.Sprintf("%d cancelled, %d failing, %d successful, %d skipped, and %d pending checks",
	counts.Canceled, counts.Failed, counts.Passed, counts.Skipping, counts.Pending)
	summary = fmt.Sprintf("%s\n%s", io.ColorScheme().Bold(summary), tallies)
	}
	if io.IsStdoutTTY() {
	fmt.Fprintln(io.Out, summary)
	fmt.Fprintln(io.Out)
	}

[samcoe]

@andyfeller I would suggest that aggregation stays the same and we just modify the output in non-TTY mode to maintain the backwards compatibility. Specifically line 52 in pkg/cmd/pr/checks/output.go can be changed from

tp.AddField(o.Bucket)

to something like this

if o.Bucket == "cancelled" {
    tp.AddField("failed")
} else {
    tp.AddField(o.Bucket)
}

[andyfeller]

@rajhawaldar : I think these changes look good and will work with the other core maintainers to doubly 🚢 given the back and forth we've experienced