Update PR when labeled/opened #235
Conversation
There was a problem hiding this comment.
Thanks for adding this. Based on the Go documentation for PullRequestEvent, there is a Label field that is populated for labeled actions. That said, the current structure of the code makes it difficult to use this value for anything, so I think we can try the simple approach of doing this evaluation on every labeled action for now.
The bigger refactor (which you might not want to take on) would be to change Base#ProcessPR and Base#UpdatePR so that we don't duplicate configuration loading. As is, we'll look for and load configuration twice, although the requests the second time should hit the HTTP cache.
server/handler/pull_request.go
Outdated
| @@ -66,6 +67,21 @@ func (h *PullRequest) Handle(ctx context.Context, eventType, deliveryID string, | |||
| logger.Error().Err(errors.WithStack(err)).Msg("Error processing pull request") | |||
| } | |||
|
|
|||
| if event.GetAction() == "labeled" || event.GetAction() == "opened" { | |||
| logger.Debug().Msgf("received pull_request %s event", event.GetAction()) | |||
There was a problem hiding this comment.
This logging is probably more appropriate higher up in the function where it will apply for all possible actions, rather than only labeled and opened actions.
There was a problem hiding this comment.
Great idea, will move this further up
server/handler/pull_request.go
Outdated
| } | ||
|
|
||
| baseRef := ref.GetObject().GetSHA() | ||
| if err := h.UpdatePullRequest(logger.WithContext(ctx), pullCtx, client, pr, baseRef); err != nil { |
There was a problem hiding this comment.
The baseRef passed to UpdatePullRequest is a ref name rather than a SHA. You should be able to use the fmt.Sprintf("refs/heads/%s", base) directly, without resolving it.
There was a problem hiding this comment.
Nice, that will really simplify things
f1sherman
force-pushed
the
update-on-label-change
branch
2 times, most recently
from
012e28e
to
be21959
Compare
|
@bluekeyes I believe I've addressed all of your feedback. I also took a shot at that refactor as a separate commit, let me know if you want me to change that or remove it. Thanks! |
There was a problem hiding this comment.
Thanks for taking on the config refactor too. All of my comments relate to that; the PR handler part of this change looks good. If you'd like to fix up the refactor part, that would be great, otherwise, feel free to remove that commit and I'll clean up some of the duplication in a separate change.
server/handler/base.go
Outdated
| } | ||
|
|
||
| switch { | ||
| case bulldozerConfig.Missing(): | ||
| return nil, errors.Errorf("No configuration found for %s", bulldozerConfig) |
There was a problem hiding this comment.
One consequence of this conversion is that these conditions that were previously only logged are now returned and propagated as errors. I think I'd like to keep the old behavior by logging the same messages here and then returning nil, nil.
This means that either callers will have to check for nil or UpdatePR and ProcessPR should return immediately if passed a nil configuration; I'd probably have the functions return immediately.
This will conditionally update a PR whenever the PR is labeled or opened, which will partially fulfill [this suggestion](palantir#145 (comment)) for palantir#145. It doesn't seem possible to determine which label was added via the Github webhook payload, so this will update the PR regardless of whether the label added is what is configured in `bulldozer.yml`.
Refactor config fetching to happen before calling `ProcessPR` and `UpdatePR` so that it isn't fetched twice during the `pull_request` event when we're both merging and updating a PR.
It probably makes sense to do things in this order rather than how we were doing it before.
f1sherman
force-pushed
the
update-on-label-change
branch
from
d8e3bdc
to
6b18729
Compare
|
@bluekeyes ok I went ahead and made those updates. Thanks so much for patiently working with me through those. Is this something you'd like me to test before merging or would it make more sense to merge first then test? Merging first makes testing a little easier for me but I'm ok with either way. |
|
This seems relatively safe in terms of changes to existing functionality, so I think we can merge first and then test. |
|
@bluekeyes just an update that I've been testing this today and it's working well! |
|
@bluekeyes after some more testing I noticed a bug with this change (or at least I consider it a bug): when a webhook triggers both an update and a merge, Bulldozer continues to use the stale statuses from before the update to evaluate whether a merge can happen. I'll work on putting together a PR to fix this. |
After my changes in palantir#235, I noticed that sometimes a PR is updated and then merged as part of the same webhook request, before the required statuses are fulfilled. This is because bulldozer was still using the stale statuses from before the update was performed.
After my changes in palantir#235, I noticed that sometimes a PR is updated and then merged as part of the same webhook request, before the required statuses are fulfilled. This is because bulldozer was still using the stale statuses from before the update was performed.
|
@bluekeyes I put together a proposed fix for this issue here. Feedback welcome (as always)! |
After my changes in palantir#235, I noticed that sometimes a PR is updated and then merged as part of the same webhook request, before the required statuses are fulfilled. This is because bulldozer was still using the stale statuses from before the update was performed.
After my changes in #235, I noticed that sometimes a PR is updated and then merged as part of the same webhook request, before the required statuses are fulfilled. This is because bulldozer was still using the stale statuses from before the update was performed.
This will conditionally update a PR whenever the PR is labeled or opened, which will partially fulfill this
suggestion for #145.
It doesn't seem possible to determine which label was added via the Github webhook payload, so this will update the PR regardless of whether the label added is what is configured in
bulldozer.yml.I haven't had a chance to test this yet (will update when I have), so I'm just looking for early feedback at this point. Thanks!