Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Option to only update when certain required status are passing #259

Closed
wrslatz opened this issue Aug 18, 2021 · 9 comments · Fixed by #307
Closed

Option to only update when certain required status are passing #259

wrslatz opened this issue Aug 18, 2021 · 9 comments · Fixed by #307
Labels
enhancement Feature requests or new functionality

Comments

@wrslatz
Copy link
Contributor

wrslatz commented Aug 18, 2021

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 Allow ignoring Draft PRs from updates #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)
@wrslatz
Copy link
Contributor Author

wrslatz commented Aug 18, 2021

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.

@bluekeyes
Copy link
Member

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.

@bluekeyes bluekeyes added the enhancement Feature requests or new functionality label Aug 19, 2021
@CritsAndCode
Copy link

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

@skakim
Copy link

skakim commented Jan 25, 2022

+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?

@bluekeyes
Copy link
Member

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.

@wrslatz
Copy link
Contributor Author

wrslatz commented Jan 25, 2022

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.

@wrslatz
Copy link
Contributor Author

wrslatz commented Apr 25, 2022

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

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

@wrslatz
Copy link
Contributor Author

wrslatz commented Apr 26, 2022

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

@wrslatz
Copy link
Contributor Author

wrslatz commented May 3, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests or new functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants