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

Can bulldozer wait until non-required checks are finished before merging? #311

Open
markcellus opened this issue May 13, 2022 · 8 comments

Comments

@markcellus
Copy link

markcellus commented May 13, 2022

Hello! Love bulldozer but having some trouble understanding something: Bulldozer seems to not wait for non-required checks to finish before merging. But the docs says that bulldozer will:

Wait for additional status checks that are not required by GitHub

Here's an example of bulldozer merging a PR without waiting until the "UI Tests" check completes.

Selection_117

How do we get bulldozer to wait until all non-required checks to complete (regardless of a failure or success) before merging?

@bluekeyes
Copy link
Member

Bulldozer will only wait for statuses that it knows about up front. This is because in general, there is no way to know when looking at a PR if all the expected status are already present or if more statuses would appear if we waited a bit longer.

There are two ways to tell Bulldozer about statuses:

  1. Make the status required using GitHub branch protection. This also makes it required for human users, which may be undesirable.
  2. Add the status to the merge.required_statuses list in the bulldozer.yml file. This makes the status required only for Bulldozer merges and is the feature the quoted line from the docs are talking about.

There's currently no way to make Bulldozer wait for failed statuses (both of the methods above require success) or make Bulldozer wait for a status without explicitly listing it.

@markcellus
Copy link
Author

markcellus commented May 16, 2022

Thanks for the information.

Hard-coding statuses by their name in merge.required_statuses forces us to manage our statuses in two places: in github settings and in merge.required_statuses list, which can grow out of date if status names are changed in GitHub. Additionally, based on documentation, it looks like the merge.required_statuses expects the checks to actually pass. But it's fine, if in my case, the check fails. I'd just like for bulldozer to wait until it's completed, regardless of success or failure.

there is no way to know when looking at a PR if all the expected status are already present or if more statuses would appear if we waited a bit longer

I'm quite sure that bulldozer only needs to wait a few milliseconds before all statuses are available to be evaluated. Can a status_wait_time or similar option be added so that bulldozer's merging can be delayed a bit for this purpose?


EDIT: added another bit of rationale for why merge.required_statuses may not work for this case.

@wrslatz
Copy link
Contributor

wrslatz commented May 20, 2022

I wonder if Bulldozer could have an additional option to wait for all statuses or checks that are present to complete before merging?

@bluekeyes
Copy link
Member

For any particular status, it should only need to appear in one place: either the GitHub protected branch configuration (if it applies to both humans and to Bulldozer) or the merge.required_status (if it only applies to Bulldozer), but I acknowledge that keeping these in sync if statuses change can be difficult and has occasionally caused problems with our use internally.

Regarding waiting for either success or failure, I don't fully understand the use case (I'm having trouble thinking of checks where I'd want it to finish but then not care about the result), but would be fine adding an option for this if you or someone else would like to contribute it.


On the bigger topic of automatically waiting for statuses, I'm still pretty hesitant. An important design goal for Bulldozer is that (once configured) it should not merge a pull request unexpectedly. When something goes wrong, in Bulldozer or in GitHub, I'd rather leave a pull request open and have someone manually merge or trigger Bulldozer than have to revert a PR that wasn't ready.

I haven't yet thought of or seen a suggestion for how to implement some kind of automatic waiting that satisfies this goal. Part of this is just how statuses work in GitHub: they don't exist until something with write permission makes the API call to create one. That means there's no universal pattern that status creators have to follow and leaves a bunch of places for failure (I've seen all of these actually happen):

  • GitHub webhook delivery is delayed
  • Performance issues with the GitHub API or the app cause the status to take longer than usual to appear
  • A bug in the app or configuration causes a failure before it ever creates a status
  • The app posts more than one status and these reflect some process that can take a long time or have dependencies

As a specific example of the last point, CircleCI allows users to create a graph of jobs that execute on each new commit. Circle only posts the pending status for a job when all dependencies complete. Unless I know something about the workflow upfront, when Bulldozer is evaluating a new success status, it doesn't really know if that's the last job or if another one is going to start.

In any particular case, you could probably pick delays or a heuristic that works most of the time but I'm not sure it would generalize and I'm still reluctant to have any guessing involved in the merge decision. When something does go wrong with CI or the approval app or whatever, I'd like to not make it worse by adding a bunch of incorrectly merged PRs that need reverts.

@markcellus
Copy link
Author

markcellus commented May 24, 2022

Regarding waiting for either success or failure, I don't fully understand the use case (I'm having trouble thinking of checks where I'd want it to finish but then not care about the result)

These are non-required checks that are added but don't require success (from a human's perspective) to be merged. Plenty of bots and even humans add checks that are just informational, see Snyk, Chromatic, etc. Off hand, I know Chromatic automatically adds a check that builds a Storybook at a URL accessible after the PR has been merged. It can fail. But we don't need that check to be required for a human or bot to merge. Just to be clear, I'm not saying this feature shouldn't wait until all checks are successful. I'd actually be okay with bulldozer not automatically merging unless all checks successful. But I'd like for it to at least wait for the checks to complete before merging, which I think is a common expectation.

Regarding automatically waiting for statuses, I understand the hesitancy around the delay option.

@markcellus
Copy link
Author

markcellus commented May 24, 2022

For context, here is a github action that I use on an open-source repo I own. It waits for all checks to pass (even non-required ones) before it merges. Maybe it's the --auto argument passed to gh pr merge command? Can that functionality be replicated in bulldozer? Redacted, the action doesn't actually wait until all non-required checks are passed as @bluekeyes pointed out below.

@bluekeyes
Copy link
Member

If this is doable by an automated tool like Dependabot, why isn't the same thing possible for bulldozer?

The unsatisfying answer is that I don't know how to do this while maintaining what I believe are the requirements for Bulldozer (specifically, don't merge unexpectedly, even when other systems fail.) If you or anyone else has suggestions or knows of specific open source projects I could look at for reference, I'm interested - I've thought about this for a bit and haven't had any good ideas yet.

For instance, if you're talking about the @dependabot merge command, I found several references to this command waiting for CI checks to pass, but I could not find documentation or source code that explained how it does this. I also don't have any personal experience using this tool that I could use to guess at how it is implemented.

In your second example, I believe that gh pr merge --auto enables GitHub's automatic merge functionality and defers to that for the actual merge decision. My tests suggest that GitHub's auto-merge feature behaves the same as Bulldozer: it will only wait for statuses required by branch protection and will happily merge a pull request if non-required statuses are still pending. I sampled a few of the Dependabot PRs in your scroll-js project and only saw one status check (Travis CI), which appears to be required by branch protection, but I could have easily missed something.

@wrslatz
Copy link
Contributor

wrslatz commented May 29, 2022

Could the requirements here be simplified to "if configured, do not merge a PR while it has any statuses or checks that are not complete?"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants