Comment when Bulldozer fails merge PR #214
Conversation
There was a problem hiding this comment.
I think it would be useful to check how often the existing errors are logged to get a sense of how often the comment path will trigger and see if multiple errors will happen on the same PR. I think I've seen Bulldozer instances race with each other to merge a PR and then both get errors. If the timing is right, that could lead to double comments but I'm not sure how to avoid it without sharing state or coordinating.
| @@ -46,6 +47,12 @@ func (h *IssueComment) Handle(ctx context.Context, eventType, deliveryID string, | |||
| installationID := githubapp.GetInstallationIDFromEvent(&event) | |||
| ctx, logger := githubapp.PreparePRContext(ctx, installationID, repo, number) | |||
|
|
|||
| // Debouncing debug comments from bot to prevent an infinite loop | |||
| if event.GetComment().GetUser().GetLogin() == fmt.Sprintf("%s[bot]", h.AppName) { | |||
There was a problem hiding this comment.
I've been curious about this for Policy Bot - does the event contain the integration ID if the actor is an integration? If so, maybe we could skip the app name and check the IDs, which we already have from configuration.
There was a problem hiding this comment.
Hm, it appears that the event contains the installation ID (which we already use on L46) and doesn't include the app's integration ID. I agree if we had that it would be easier to use instead of using the app client on server startup.
server/handler/base.go
Outdated
| @@ -64,7 +66,31 @@ func (b *Base) ProcessPullRequest(ctx context.Context, pullCtx pull.Context, cli | |||
| return errors.Wrap(err, "unable to determine merge status") | |||
| } | |||
| if shouldMerge { | |||
| bulldozer.MergePR(ctx, pullCtx, merger, config.Merge) | |||
| failureReason := bulldozer.MergePR(ctx, pullCtx, merger, config.Merge) | |||
| if failureReason != "" { | |||
There was a problem hiding this comment.
Might be nice to extract the commenting logic in this condition to a helper function
server/handler/base.go
Outdated
| } | ||
|
|
||
| if shouldComment { | ||
| _, _, err := client.Issues.CreateComment(ctx, pullCtx.Owner(), pullCtx.Repo(), pullCtx.Number(), &github.IssueComment{ |
There was a problem hiding this comment.
Does the app already have commenting permissions? I guess it might if we needed write on issues to close them when merging linked PRs. If not, we'll need to update the README.
There was a problem hiding this comment.
Yeah, the app already asks for write on issues & PRs in order to merge PRs and close linked issues.
bulldozer/merge.go
Outdated
| @@ -152,7 +152,7 @@ func (m *PushRestrictionMerger) DeleteHead(ctx context.Context, pullCtx pull.Con | |||
|
|
|||
| // MergePR merges a pull request if all conditions are met. It logs any errors | |||
| // that it encounters. | |||
| func MergePR(ctx context.Context, pullCtx pull.Context, merger Merger, mergeConfig MergeConfig) { | |||
| func MergePR(ctx context.Context, pullCtx pull.Context, merger Merger, mergeConfig MergeConfig) string { | |||
There was a problem hiding this comment.
While string is functional here, perhaps it might be better to return an error now that something checks the return. That could centralize some logging and a custom error type could be used to signal errors that should be reported as comments.
bulldozer/merge.go
Outdated
| case http.StatusConflict: | ||
| logger.Info().Msgf("Merge rejected due to being invalid: %q", gerr.Message) | ||
| return false, false | ||
| reason = fmt.Sprintf("Merge rejected due to being invalid: %q", gerr.Message) |
There was a problem hiding this comment.
We should check if this is the status reported when two Bulldozer instances race each other and the loser tries to merge an already-merged PR. I think it would just be confusing if Bulldozer posted a comment in that case.
Only leave a comment when bulldozer fails to merge a PR via the API, rather than speculatively reasoning about state before we attempt to merge. An alternative approach to #213
Resolves #195