Skip to content

Add support for issue state reason #6245

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
samcoe merged 6 commits into from
Sep 14, 2022
Merged

Add support for issue state reason #6245

samcoe merged 6 commits into from
Sep 14, 2022

Conversation

@samcoe
Copy link
Contributor

@samcoe samcoe commented Sep 13, 2022 •
edited
Loading

This PR adds support for issue state reason to various commands:

  • issue list set color for closed issues according to state reason
  • issue view set color for closed issues according to state reason
  • search issues set color for closed issues according to state reason
  • issue close add --reason flag to allow closing of issues with a reason.
    • If running on GHES that does not support issue state reason, silently remove reason but still close the issue.

This PR does not add support of searching by issue state reason to issue list or search issues. Both of those commands support custom search queries that can be used to search by issue state reason according to the docs. We can revisit adding in dedicated flags for this later, but I felt as support for issue state reason was only introduced in GHES 3.6 that adding in flags that did not work on all supported GHES versions would be confusing for users of GHES older than 3.6.

cc/ @azenMatt
cc/ https://github.com/github/cli/issues/114

@samcoe samcoe self-assigned this Sep 13, 2022
samcoe
samcoe commented Sep 13, 2022
View reviewed changes
pkg/cmd/issue/close/close_test.go
@@ -1,189 +1,281 @@
package close

Copy link
Contributor Author

@samcoe samcoe Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Migrated these tests from old style to new style hence the large number of line changes.

samcoe
samcoe commented Sep 13, 2022
View reviewed changes
@samcoe samcoe marked this pull request as ready for review September 13, 2022 07:04
@samcoe samcoe requested a review from a team as a code owner September 13, 2022 07:04
@samcoe samcoe requested review from vilmibm and removed request for a team September 13, 2022 07:04
mislav
mislav approved these changes Sep 13, 2022
View reviewed changes
Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!

pkg/cmd/issue/view/view.go Outdated
Comment on lines 120 to 121
if features.StateReason {
lookupFields.Add("stateReason")
Copy link
Contributor

@mislav mislav Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would it be simpler to push this logic into shared.findIssueOrPR?

Example: if findIssueOrPR was invoked with stateReason among fields, use featuredetector to determine whether it's supported, and scrub it out if not. Prior art:

cli/pkg/cmd/pr/shared/finder.go

Lines 142 to 151 in e2e8d69

if fields.Contains("isInMergeQueue") || fields.Contains("isMergeQueueEnabled") {
cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24)
detector := fd.NewDetector(cachedClient, f.repo.RepoHost())
prFeatures, err := detector.PullRequestFeatures()
if err != nil {
return nil, nil, err
}
if !prFeatures.MergeQueue {
fields.Remove("isInMergeQueue")
fields.Remove("isMergeQueueEnabled")

This way, all callers of findIssueOrPR across all commands would not have to worry about GHES not supporting certain fields.

The only gotcha would be that a fd.Detector would have to be passed down as an injectable, though 🤔 Feel free to disregard if that's too much hassle.

Copy link
Contributor Author

@samcoe samcoe Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea. I went ahead and implemented it. Luckily (or unluckily) none of our issue tests target GHES so the feature detection here is a no-op and doesn't need to be injected for testing purposes.

Copy link
Contributor Author

@samcoe samcoe Sep 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Future tests that target GHES could also just mock out the request instead of having to inject the detector.

@samcoe samcoe merged commit e14d14c into trunk Sep 14, 2022
@samcoe samcoe deleted the issue-state-reason branch September 14, 2022 08:39
@andyatmiami andyatmiami mentioned this pull request Mar 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vilmibm vilmibm Awaiting requested review from vilmibm vilmibm is a code owner automatically assigned from cli/code-reviewers

1 more reviewer

@mislav mislav mislav approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@samcoe samcoe

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@samcoe @mislav