Skip to content

Add support for issue state reason - #6245

Merged
samcoe merged 6 commits into
trunkfrom
issue-state-reason
Sep 14, 2022
Merged

Add support for issue state reason#6245
samcoe merged 6 commits into
trunkfrom
issue-state-reason

Conversation

@samcoe

@samcoe samcoe commented Sep 13, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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
Comment thread pkg/cmd/issue/close/close_test.go
@@ -2,188 +2,280 @@ package close

@samcoe samcoe Sep 13, 2022

Copy link
Copy Markdown
Contributor Author

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
Comment thread pkg/cmd/issue/close/close.go Outdated
@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

@mislav mislav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great!

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

@mislav mislav Sep 13, 2022

Copy link
Copy Markdown
Contributor

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.

@samcoe samcoe Sep 14, 2022

Copy link
Copy Markdown
Contributor Author

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.

@samcoe samcoe Sep 14, 2022

Copy link
Copy Markdown
Contributor Author

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 @cliAutomation