Skip to content

Comments

Gracefully handle "422: Reference does not exist" when deleting remote branch#1279

Merged
mislav merged 4 commits intocli:trunkfrom
pborzenkov:delete-branch-422
Jul 2, 2020
Merged

Gracefully handle "422: Reference does not exist" when deleting remote branch#1279
mislav merged 4 commits intocli:trunkfrom
pborzenkov:delete-branch-422

Conversation

@pborzenkov
Copy link
Contributor

Summary

This PR modifies BrachDeleteRemote to gracefully handle the case when remote branch has already been automatically
deleted by GitHub.

Closes #1010

So that it's possible to access it in mocked HTTP tests.

Signed-off-by: Pavel Borzenkov <pavel.borzenkov@gmail.com>
`fmt.Errorf` hides information and makes it hard to test for specific
conditions in returned error. Return a structured error instead.

Signed-off-by: Pavel Borzenkov <pavel.borzenkov@gmail.com>
If a GitHub repo is configured to automatically delete branches after PR
is merged, `gh pr merge` fails with error like:

failed to delete remote branch: ... (422): 'Reference does not exist'

Gracefully handle such case and don't report the error.

Signed-off-by: Pavel Borzenkov <pavel.borzenkov@gmail.com>
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! I've taken the liberty to add some tweaks.

One output change that I snuck in here is that "HTTP" and "GraphQL" are now correctly capitalized in error messages ✨

Message string `json:"message"`
}
if err := json.Unmarshal(body, &parsedBody); err == nil {
httpError.Message = parsedBody.Message
Copy link
Contributor Author

Choose a reason for hiding this comment

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

You no longer pass body as error Message in case json.Unmarshal is unsuccessful. Is this intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this is intentional. I remembered that the body of some 5xx responses might be a HTML document (caused by e.g. an outage) and we wouldn't want to show that as error message. For non-JSON responses, I'm fine if we just report HTTP {StatusCode} on stderr.

@pborzenkov
Copy link
Contributor Author

@mislav Thanks for the patch. Looks great aside from a very minor comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Gracefully handle when branches automatically deleted when PR is merged

3 participants