Skip to content

Commit 4f2304d

Browse files
BagToadCopilot
andcommitted
Remove StateReason feature detection for issue close
The stateReason field was added in GHES ~3.4, which is far older than the earliest supported GHES version (3.14). The feature detection and conditional inclusion of stateReason is therefore unnecessary. This removes: - StateReason field from IssueFeatures struct - GHES introspection query in IssueFeatures() (only ActorIsAssignable remains, which is always false on GHES) - Conditional stateReason field inclusion in issue list - Feature detection guard in issue close - Feature detection guard in FindIssueOrPR Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 6b56a23 commit 4f2304d

6 files changed

Lines changed: 6 additions & 128 deletions

File tree

internal/featuredetection/feature_detection.go

Lines changed: 2 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,10 @@ type Detector interface {
2323
}
2424

2525
type IssueFeatures struct {
26-
StateReason bool
2726
ActorIsAssignable bool
2827
}
2928

3029
var allIssueFeatures = IssueFeatures{
31-
StateReason: true,
3230
ActorIsAssignable: true,
3331
}
3432

@@ -137,32 +135,9 @@ func (d *detector) IssueFeatures() (IssueFeatures, error) {
137135
return allIssueFeatures, nil
138136
}
139137

140-
features := IssueFeatures{
141-
StateReason: false,
138+
return IssueFeatures{
142139
ActorIsAssignable: false, // replaceActorsForAssignable GraphQL mutation unavailable on GHES
143-
}
144-
145-
var featureDetection struct {
146-
Issue struct {
147-
Fields []struct {
148-
Name string
149-
} `graphql:"fields(includeDeprecated: true)"`
150-
} `graphql:"Issue: __type(name: \"Issue\")"`
151-
}
152-
153-
gql := api.NewClientFromHTTP(d.httpClient)
154-
err := gql.Query(d.host, "Issue_fields", &featureDetection, nil)
155-
if err != nil {
156-
return features, err
157-
}
158-
159-
for _, field := range featureDetection.Issue.Fields {
160-
if field.Name == "stateReason" {
161-
features.StateReason = true
162-
}
163-
}
164-
165-
return features, nil
140+
}, nil
166141
}
167142

168143
func (d *detector) PullRequestFeatures() (PullRequestFeatures, error) {

internal/featuredetection/feature_detection_test.go

Lines changed: 1 addition & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,6 @@ func TestIssueFeatures(t *testing.T) {
2323
name: "github.com",
2424
hostname: "github.com",
2525
wantFeatures: IssueFeatures{
26-
StateReason: true,
2726
ActorIsAssignable: true,
2827
},
2928
wantErr: false,
@@ -32,38 +31,18 @@ func TestIssueFeatures(t *testing.T) {
3231
name: "ghec data residency (ghe.com)",
3332
hostname: "stampname.ghe.com",
3433
wantFeatures: IssueFeatures{
35-
StateReason: true,
3634
ActorIsAssignable: true,
3735
},
3836
wantErr: false,
3937
},
4038
{
41-
name: "GHE empty response",
39+
name: "GHE",
4240
hostname: "git.my.org",
43-
queryResponse: map[string]string{
44-
`query Issue_fields\b`: `{"data": {}}`,
45-
},
4641
wantFeatures: IssueFeatures{
47-
StateReason: false,
4842
ActorIsAssignable: false,
4943
},
5044
wantErr: false,
5145
},
52-
{
53-
name: "GHE has state reason field",
54-
hostname: "git.my.org",
55-
queryResponse: map[string]string{
56-
`query Issue_fields\b`: heredoc.Doc(`
57-
{ "data": { "Issue": { "fields": [
58-
{"name": "stateReason"}
59-
] } } }
60-
`),
61-
},
62-
wantFeatures: IssueFeatures{
63-
StateReason: true,
64-
},
65-
wantErr: false,
66-
},
6746
}
6847

6948
for _, tt := range tests {

pkg/cmd/issue/close/close.go

Lines changed: 2 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,9 @@ package close
33
import (
44
"fmt"
55
"net/http"
6-
"time"
76

87
"github.com/MakeNowJust/heredoc"
98
"github.com/cli/cli/v2/api"
10-
fd "github.com/cli/cli/v2/internal/featuredetection"
119
"github.com/cli/cli/v2/internal/ghrepo"
1210
"github.com/cli/cli/v2/pkg/cmd/issue/shared"
1311
prShared "github.com/cli/cli/v2/pkg/cmd/pr/shared"
@@ -26,8 +24,6 @@ type CloseOptions struct {
2624
Comment string
2725
Reason string
2826
DuplicateOf string
29-
30-
Detector fd.Detector
3127
}
3228

3329
func NewCmdClose(f *cmdutil.Factory, runF func(*CloseOptions) error) *cobra.Command {
@@ -165,7 +161,7 @@ func closeRun(opts *CloseOptions) error {
165161
}
166162
}
167163

168-
err = apiClose(httpClient, baseRepo, issue, opts.Detector, closeReason, duplicateIssueID)
164+
err = apiClose(httpClient, baseRepo, issue, closeReason, duplicateIssueID)
169165
if err != nil {
170166
return err
171167
}
@@ -175,30 +171,11 @@ func closeRun(opts *CloseOptions) error {
175171
return nil
176172
}
177173

178-
func apiClose(httpClient *http.Client, repo ghrepo.Interface, issue *api.Issue, detector fd.Detector, reason string, duplicateIssueID string) error {
174+
func apiClose(httpClient *http.Client, repo ghrepo.Interface, issue *api.Issue, reason string, duplicateIssueID string) error {
179175
if issue.IsPullRequest() {
180176
return api.PullRequestClose(httpClient, repo, issue.ID)
181177
}
182178

183-
if reason != "" || duplicateIssueID != "" {
184-
if detector == nil {
185-
cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24)
186-
detector = fd.NewDetector(cachedClient, repo.RepoHost())
187-
}
188-
features, err := detector.IssueFeatures()
189-
if err != nil {
190-
return err
191-
}
192-
// TODO stateReasonCleanup
193-
if !features.StateReason {
194-
// If StateReason is not supported silently close issue without setting StateReason.
195-
if duplicateIssueID != "" {
196-
return fmt.Errorf("closing as duplicate is not supported on %s", repo.RepoHost())
197-
}
198-
reason = ""
199-
}
200-
}
201-
202179
switch reason {
203180
case "":
204181
// If no reason is specified do not set it.

pkg/cmd/issue/close/close_test.go

Lines changed: 0 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@ import (
55
"net/http"
66
"testing"
77

8-
fd "github.com/cli/cli/v2/internal/featuredetection"
98
"github.com/cli/cli/v2/internal/ghrepo"
109
"github.com/cli/cli/v2/pkg/cmd/issue/argparsetest"
1110
"github.com/cli/cli/v2/pkg/cmdutil"
@@ -185,7 +184,6 @@ func TestCloseRun(t *testing.T) {
185184
opts: &CloseOptions{
186185
IssueNumber: 13,
187186
Reason: "not planned",
188-
Detector: &fd.EnabledDetectorMock{},
189187
},
190188
httpStubs: func(reg *httpmock.Registry) {
191189
reg.Register(
@@ -213,7 +211,6 @@ func TestCloseRun(t *testing.T) {
213211
opts: &CloseOptions{
214212
IssueNumber: 13,
215213
Reason: "duplicate",
216-
Detector: &fd.EnabledDetectorMock{},
217214
},
218215
httpStubs: func(reg *httpmock.Registry) {
219216
reg.Register(
@@ -241,7 +238,6 @@ func TestCloseRun(t *testing.T) {
241238
opts: &CloseOptions{
242239
IssueNumber: 13,
243240
DuplicateOf: "99",
244-
Detector: &fd.EnabledDetectorMock{},
245241
},
246242
httpStubs: func(reg *httpmock.Registry) {
247243
reg.Register(
@@ -338,33 +334,6 @@ func TestCloseRun(t *testing.T) {
338334
wantErr: true,
339335
errMsg: "invalid value for `--duplicate-of`: invalid issue format: \"not-an-issue\"",
340336
},
341-
{
342-
name: "close issue with reason when reason is not supported",
343-
opts: &CloseOptions{
344-
IssueNumber: 13,
345-
Reason: "not planned",
346-
Detector: &fd.DisabledDetectorMock{},
347-
},
348-
httpStubs: func(reg *httpmock.Registry) {
349-
reg.Register(
350-
httpmock.GraphQL(`query IssueByNumber\b`),
351-
httpmock.StringResponse(`
352-
{ "data": { "repository": {
353-
"hasIssuesEnabled": true,
354-
"issue": { "id": "THE-ID", "number": 13, "title": "The title of the issue"}
355-
} } }`),
356-
)
357-
reg.Register(
358-
httpmock.GraphQL(`mutation IssueClose\b`),
359-
httpmock.GraphQLMutation(`{"id": "THE-ID"}`,
360-
func(inputs map[string]interface{}) {
361-
assert.Equal(t, 1, len(inputs))
362-
assert.Equal(t, "THE-ID", inputs["issueId"])
363-
}),
364-
)
365-
},
366-
wantStderr: "✓ Closed issue OWNER/REPO#13 (The title of the issue)\n",
367-
},
368337
{
369338
name: "issue already closed",
370339
opts: &CloseOptions{

pkg/cmd/issue/list/list.go

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -147,15 +147,7 @@ func listRun(opts *ListOptions) error {
147147
cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24)
148148
opts.Detector = fd.NewDetector(cachedClient, baseRepo.RepoHost())
149149
}
150-
features, err := opts.Detector.IssueFeatures()
151-
if err != nil {
152-
return err
153-
}
154-
fields := defaultFields
155-
// TODO stateReasonCleanup
156-
if features.StateReason {
157-
fields = append(defaultFields, "stateReason")
158-
}
150+
fields := append(defaultFields, "stateReason")
159151

160152
filterOptions := prShared.FilterOptions{
161153
Entity: "issue",

pkg/cmd/issue/shared/lookup.go

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,8 @@ import (
88
"regexp"
99
"strconv"
1010
"strings"
11-
"time"
1211

1312
"github.com/cli/cli/v2/api"
14-
fd "github.com/cli/cli/v2/internal/featuredetection"
1513
"github.com/cli/cli/v2/internal/ghrepo"
1614
o "github.com/cli/cli/v2/pkg/option"
1715
"github.com/cli/cli/v2/pkg/set"
@@ -138,18 +136,6 @@ func FindIssuesOrPRs(httpClient *http.Client, repo ghrepo.Interface, issueNumber
138136
func FindIssueOrPR(httpClient *http.Client, repo ghrepo.Interface, number int, fields []string) (*api.Issue, error) {
139137
fieldSet := set.NewStringSet()
140138
fieldSet.AddValues(fields)
141-
if fieldSet.Contains("stateReason") {
142-
cachedClient := api.NewCachedHTTPClient(httpClient, time.Hour*24)
143-
detector := fd.NewDetector(cachedClient, repo.RepoHost())
144-
features, err := detector.IssueFeatures()
145-
if err != nil {
146-
return nil, err
147-
}
148-
// TODO stateReasonCleanup
149-
if !features.StateReason {
150-
fieldSet.Remove("stateReason")
151-
}
152-
}
153139

154140
var getProjectItems bool
155141
if fieldSet.Contains("projectItems") {

0 commit comments

Comments
 (0)