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:
required_statuses
should be separately configured forupdate
andmerge
required_statuses
forupdate
should be optional- The
update
support forrequired_statuses
should reuse the configuration interfaces and code implementations for the equivalent support inmerge
wherever possible, to help keep things consistent between the two and reduce maintenance over time.
Open questions:
- Should
trigger
conditions override therequired_statuses
like they do forignore_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 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 forignore_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.