Refactor GitHub client usage for merges #105
Conversation
Remove unnecessary context.Context arguments and error returns and add a function for getting a PR's merge state. This starts to remove a direct dependence on github.Client from the MergePR() function.
This removes another direct dependency on a *github.Client
Remove the last direct references to github.Client in MergePR. This theoretically allows testing this function now, although I didn't write any tests.
| if err != nil { | ||
| logger.Error().Err(errors.WithStack(err)).Msgf("Unable to list open prs against ref %s to compare delete request", ref) | ||
| logger.Error().Err(err).Msgf("Unable to determine if ref %s is targeted by other open pull requests before deletion", ref) |
There was a problem hiding this comment.
nit: Why the change away from errors.WithStack(err)?
There was a problem hiding this comment.
I assume that pull.Context implementations already wrap errors as needed, so adding a stack again is redundant. It was useful before because the code was mostly dealing with the GitHub client directly.
| @@ -67,12 +67,24 @@ func (ghc *GithubContext) Locator() string { | |||
| return fmt.Sprintf("%s/%s#%d", ghc.owner, ghc.repo, ghc.number) | |||
| } | |||
|
|
|||
| func (ghc *GithubContext) Title(ctx context.Context) (string, error) { | |||
| return ghc.pr.GetTitle(), nil | |||
There was a problem hiding this comment.
🥇 Huh. How did this pass in CR previously..
| // Delete ref if owner of BASE and HEAD match | ||
| // otherwise, its from a fork that we cannot delete | ||
| if pr.GetBase().GetUser().GetLogin() == pr.GetHead().GetUser().GetLogin() { | ||
| // if head is qualified (contains ":"), PR is from a fork and we don't have delete permission |
There was a problem hiding this comment.
I think we should address this in a later PR, but if the remote fork has bulldozer installed this assumption is no longer correct. I think it was originally here as a workaround from when this operated using oauth tokens.
We might be able to attempt a delete action, and just fail if it doesn't work. Not blocking, though.
There was a problem hiding this comment.
If we want to support this, I consider it a new feature that requires more refactoring: even if the fork has Bulldozer installed, we need to look up the installation ID and construct a new client using that ID.
I'm also not sure how useful this would be. It seems odd to need Bulldozer for merges on a repository that mostly submits PRs to an upstream repo, so would the only reason to install it be so that it could delete branches?
| } | ||
|
|
||
| commitMessage = body | ||
| commitMessage = pullCtx.Body() |
| return | ||
| default: | ||
| logger.Error().Err(errors.WithStack(err)).Msgf("Merge failed unexpectedly %q", gerr.Message) | ||
| logger.Error().Msgf("Merge failed with unexpected status: %d: %q", gerr.Response.StatusCode, gerr.Message) |
There was a problem hiding this comment.
losing the stacktrace on purpose?
There was a problem hiding this comment.
Yeah, we already know that the error in this case is an HTTP failure, so the important part is the status code and the message. It's also unambiguous where the error is generated.
Add additional methods to
pull.Context(and clean up some of the existing ones) so thatbulldozer.MergePRcan operate without any direct references to agithub.Client. This improves testability (although I don't add any new tests here) and enables a fix for #98 that I'll add in a followup PR.Fixes #100.