palantir/bulldozer

Option to only update when certain required status are passing

wrslatz opened this issue · 9 comments

Similar to how merge can be configured to not take action unless all required_statuses are passing or neutral, it would be helpful if update could be configured to not take action unless all required_statuses are passing or neutral. This would help prevent failing builds from being continuously updated and wasting CI resources.

Considerations for implementation:

  1. required_statuses should be separately configured for update and merge
  2. required_statuses for update should be optional
  3. The update support for required_statuses should reuse the configuration interfaces and code implementations for the equivalent support in merge wherever possible, to help keep things consistent between the two and reduce maintenance over time.

Open questions:

  1. Should trigger conditions override the required_statuses like they do for ignore_drafts (added in #256)? This would allow users to manually override failing statuses to keep updates happening (in case an update is expected to resolve a failing status)

This seems somewhat similar to #181 and #128 (attempting to improve when update actions happen), but I think this is more easily achievable as it does not depend on the state of other PRs, which was the concern for those ideas.

This does sound like a reasonable way to start addressing issues with excessive updates while not tracking PR state in Bulldozer - thanks for suggesting it!

Should trigger conditions override the required_statuses like they do for ignore_drafts?

I think this makes sense. If I explicitly request updates, on a pull requests, I think it would be confusing if they also had to satisfy other conditions. Also, explicit requests are less likely to cause issues with excessive updates in the first place.

While this is different from how required_statuses works in the merge block, I think it's reasonable that update and merge configurations behave differently, and there's already precedent for this.

+1 for this suggestion. Would be really useful to us as we also have a lot of unneeded updates with our PRs.

+1 on this suggestion too. Currently it updates all PRs when the target branch is updated, triggering a ton of validation jobs unnecessarily as they will need to be executed again later when they are first in the queue.

I would like to be able the PR to only be updated once it is the first in the queue and it is going to be merged. On a PR that fails the CI: It should be discarded from the queue, or put back at the end of it.

@bluekeyes would that be possible to implement? How hard would be to implement it, so I can see if I can help?

The original suggestion on this ticket (adding a required_statuses property for updates) should be straightforward to implement, since we already have similar logic for merging. Unfortunately, in our own usage of Bulldozer we use the merge functionality much more (nearly 100:1) than the update functionality, so it is harder to prioritize improvements like this. I'm happy to review a PR for this feature if someone would like to try implementing it.

One clarification: Bulldozer does not maintain a queue of PRs to update. It simply lists the open PRs from GitHub and then updates them in the order they are returned. If you think there is a sort order or other heuristic using data from GitHub that would give better results, I'm also happy to review PRs for that change, but I'd like to avoid adding state (like a persistent queue) to Bulldozer.

If you think there is a sort order or other heuristic using data from GitHub that would give better results, I'm also happy to review PRs for that change

Would it be possible to look at all open PRs and find the PR with the most consecutive update commits from Bulldozer as the top priority for merge? Or perhaps a configurable label matching for priority? I can see how a lack of local state might cause issues and race conditions, though.

If Pull Request Merge Queue ends up landing in full support from GitHub, that seems like a better long-term option for merge queueing (defer to the platform). It would help remove the need for update merged from Bulldozer.

Once that lands, I may investigate further and open issues for potential support in Bulldozer.

Created #297 for supporting GitHub's merge queue (once it is stable).

I'm planning to work on contributing this feature this week 🤞🏻

Deleted my unrelated comment, accidentally posted on the wrong issue 😬 😞

Raised a draft PR for this enhancement: #307

See #307 (comment) for one consideration where I could use feedback. Status or check runs not being fulfilled is working, but completion of status or check runs is not triggering updates as currently implemented. I'm thinking it should, but open to feedback before I dig in on that part of this change.