Skip to content

Commit 741ced9

Browse files
author
“yashdesai30”
committed
fix(pullrequests): return warning instead of error on label addition failure in create_pull_request
- Add optional `Warning` field to `MinimalResponse` to allow propagating non-blocking warnings on successful operations. - Update `create_pull_request` to return a successful tool response with a warning if PR creation succeeds but label addition fails, resolving the retry hazard. - Correct unit tests to mock `422` (Unprocessable Entity) instead of `404` for label addition failure, and assert the warning field. - Update description of the `labels` parameter in schema to clarify that labels must already exist in the repository.
1 parent e3a96a4 commit 741ced9

4 files changed

Lines changed: 27 additions & 20 deletions

File tree

pkg/github/__toolsnaps__/create_pull_request.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,7 @@
3131
"type": "string"
3232
},
3333
"labels": {
34-
"description": "Labels to apply to this pull request",
34+
"description": "Labels to apply to this pull request (must already exist in the repository)",
3535
"items": {
3636
"type": "string"
3737
},

pkg/github/minimal_types.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,8 +134,9 @@ type MinimalTag struct {
134134
// Success is implicit in the HTTP response status, and all other information
135135
// can be derived from the URL or fetched separately if needed.
136136
type MinimalResponse struct {
137-
ID string `json:"id"`
138-
URL string `json:"url"`
137+
ID string `json:"id"`
138+
URL string `json:"url"`
139+
Warning string `json:"warning,omitempty"`
139140
}
140141

141142
type MinimalProject struct {

pkg/github/pullrequests.go

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -589,7 +589,7 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
589589
"labels": {
590590
Type: "array",
591591
Items: &jsonschema.Schema{Type: "string"},
592-
Description: "Labels to apply to this pull request",
592+
Description: "Labels to apply to this pull request (must already exist in the repository)",
593593
},
594594
},
595595
Required: []string{"owner", "repo", "title", "head", "base"},
@@ -693,15 +693,12 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
693693
return ghErrors.NewGitHubAPIStatusErrorResponse(ctx, "failed to create pull request", resp, bodyBytes), nil, nil
694694
}
695695

696+
var warningMsg string
696697
// Add labels if provided
697698
if len(labels) > 0 {
698699
_, labelsResp, labelsErr := client.Issues.AddLabelsToIssue(ctx, owner, repo, pr.GetNumber(), labels)
699700
if labelsErr != nil {
700-
return ghErrors.NewGitHubAPIErrorResponse(ctx,
701-
fmt.Sprintf("pull request created (#%d) but failed to add labels", pr.GetNumber()),
702-
labelsResp,
703-
labelsErr,
704-
), nil, nil
701+
warningMsg = fmt.Sprintf("pull request created (#%d) but failed to add labels: %s", pr.GetNumber(), labelsErr.Error())
705702
}
706703
if labelsResp != nil {
707704
_ = labelsResp.Body.Close()
@@ -710,8 +707,9 @@ func CreatePullRequest(t translations.TranslationHelperFunc) inventory.ServerToo
710707

711708
// Return minimal response with just essential information
712709
minimalResponse := MinimalResponse{
713-
ID: fmt.Sprintf("%d", pr.GetID()),
714-
URL: pr.GetHTMLURL(),
710+
ID: fmt.Sprintf("%d", pr.GetID()),
711+
URL: pr.GetHTMLURL(),
712+
Warning: warningMsg,
715713
}
716714

717715
r, err := json.Marshal(minimalResponse)

pkg/github/pullrequests_test.go

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2207,12 +2207,13 @@ func Test_CreatePullRequest(t *testing.T) {
22072207
}
22082208

22092209
tests := []struct {
2210-
name string
2211-
mockedClient *http.Client
2212-
requestArgs map[string]any
2213-
expectError bool
2214-
expectedPR *github.PullRequest
2215-
expectedErrMsg string
2210+
name string
2211+
mockedClient *http.Client
2212+
requestArgs map[string]any
2213+
expectError bool
2214+
expectedPR *github.PullRequest
2215+
expectedErrMsg string
2216+
expectedWarning string
22162217
}{
22172218
{
22182219
name: "successful PR creation",
@@ -2295,7 +2296,7 @@ func Test_CreatePullRequest(t *testing.T) {
22952296
mockedClient: MockHTTPClientWithHandlers(map[string]http.HandlerFunc{
22962297
PostReposPullsByOwnerByRepo: mockResponse(t, http.StatusCreated, mockPR),
22972298
PostReposIssuesLabelsByOwnerByRepoByIssueNumber: http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
2298-
w.WriteHeader(http.StatusNotFound)
2299+
w.WriteHeader(http.StatusUnprocessableEntity)
22992300
_, _ = w.Write([]byte(`{"message": "Label does not exist"}`))
23002301
}),
23012302
}),
@@ -2307,8 +2308,9 @@ func Test_CreatePullRequest(t *testing.T) {
23072308
"base": "main",
23082309
"labels": []any{"nonexistent-label"},
23092310
},
2310-
expectError: true,
2311-
expectedErrMsg: "failed to add labels",
2311+
expectError: false,
2312+
expectedPR: mockPR,
2313+
expectedWarning: "pull request created (#42) but failed to add labels",
23122314
},
23132315
}
23142316

@@ -2351,6 +2353,12 @@ func Test_CreatePullRequest(t *testing.T) {
23512353
err = json.Unmarshal([]byte(textContent.Text), &returnedPR)
23522354
require.NoError(t, err)
23532355
assert.Equal(t, tc.expectedPR.GetHTMLURL(), returnedPR.URL)
2356+
2357+
if tc.expectedWarning != "" {
2358+
assert.Contains(t, returnedPR.Warning, tc.expectedWarning)
2359+
} else {
2360+
assert.Empty(t, returnedPR.Warning)
2361+
}
23542362
})
23552363
}
23562364
}

0 commit comments

Comments
 (0)