Handle HTTP 404 when deleting remote branch in pr merge
#11234
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| @@ -460,10 +460,22 @@ func (m *mergeContext) deleteRemoteBranch() error { | |||
| if !m.merged { | |||
There was a problem hiding this comment.
I tried to improve the code around this if-statement, because it's confusing, but at the end gave up due to tests breaking. The coupling among the methods on mergeContext, and their behaviour against the state fields make it very hard to understand/improve the code.
Signed-off-by: Babak K. Shandiz <babakks@github.com>
…s error Signed-off-by: Babak K. Shandiz <babakks@github.com>
2abe161 to
b1e1c8d
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR updates the remote-branch deletion logic to treat GitHub’s 404 “Reference does not exist” as non-fatal (in addition to 422) and adds tests for these cases.
- Treat HTTP 404 errors from the delete-ref endpoint as “already deleted”
- Retain existing behavior for HTTP 422 and surface other HTTP errors
- Add
TestPrMerge_deleteBranch_apiErrorcovering 422, 404, and 500 scenarios
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| pkg/cmd/pr/merge/merge.go | Extend deleteRemoteBranch to ignore both 422 and 404 responses |
| pkg/cmd/pr/merge/merge_test.go | Add tests for handling 422, 404, and unexpected API errors |
Comments suppressed due to low confidence (1)
pkg/cmd/pr/merge/merge_test.go:700
- In the unknown-API-error test you define
wantStderrbut never assert onoutput.Stderr()whenwantErris set. Add an assertion to verify that no "Deleted remote branch" message is printed on failure.
wantStderr: heredoc.Doc(`
Signed-off-by: Babak K. Shandiz <babakks@github.com>
There was a problem hiding this comment.
LGTM. We reviewed this in sync and discussed the problem and confidence around tests. We are both confident with these changes because the tests back up the behavior around which API call (the delete ref API) returns a 404 and what the expected error is in that case. In other words, there are no changes to behavior here if other API calls were to return a 404.
Fixes #11187
This PR improves the error handling when deleting a remote branch. Prior to this, only an
HTTP 422response was considered as an indication of an already deleted branch. However, as explained in the issue, the API can also returnHTTP 404, which is now handled in this PR.Since we don't have a simple way to reproduce this, there's no A/C check result. However, necessary test cases are added and verify the expected behaviour.