squalrus / merge-bot

A GitHub action that manages pull request integrations
MIT License
70 stars 12 forks source link

Update from base branch before merging. #13

Open Evanion opened 4 years ago

Evanion commented 4 years ago

Is your feature request related to a problem? Please describe. merge queues are most useful in workflows that take time. For instance... running the CI pipeline for unit and e2e tests might take 20 minutes or more. In this case, the risk that some other team member manages to merge their branch before your PRs CI run is done, is high, forcing you to start over by updating the PR with the latest HEAD of master. This is the primary case for a merge bot. The bot should then watch a repo for changes, see if it becomes ready to merge, and check if it's behind master, if it's behind master, it should trigger a branch update, and when the CI system reports done, it should try again.

Describe the solution you'd like check if current branch is behind target branch, and then update current branch with current head. notifying the user incase of merge conflict for manual resolution. https://octokit.github.io/rest.js/#octokit-routes-pulls-update-branch

Describe alternatives you've considered well manual handling .. since one of the checks to see if you are allowed to merge, is that the branch is behind master, it would still be blocked... But then the user still needs to check in to see if the branch have been merged or if they need to update the branch again. This would only be a semi automatic system... I would prefer a fire and forget solution.. where if you mark a branch as ready to merge, you can trust that the bot will merge it as long as there aren't any merge conflicts, even if another merge gets in between.

AArnott commented 4 years ago

I never understood why some folks want the source branch to be current with the target branch before merging. The PR merge commit will merge the two versions together anyway, so why artificially create another merge commit?

squalrus commented 4 years ago

Thank you for the feedback! In general I agree with @AArnott but am interested in hearing more @Evanion.

check if current branch is behind target branch, and then update current branch with current head. notifying the user in case of merge conflict for manual resolution.

Regardless of the age of the branch, if merge conflicts exist into the base, they will be identified in the pull request.

If the goal is to keep your master branch clean, modifying the merge strategy may help.

RevolutionTech commented 3 years ago

I don't know about @Evanion's use case, but one use case for requiring features branches to be up-to-date with the main branch before merging is to avoid a situation where CI passes on the main branch and CI passes on the feature branch but the (automatically-generated) merge commit fails CI.

This might be possible if changes on the main branch are incompatible with changes in your feature branch, but not in a way that generates git conflicts.

AArnott commented 3 years ago

Sure, but why not run the tests on the merge commit prior to pushing? This is standard practice: send a pull request from the feature branch to the main branch, run validations on it as part of the PR, then merge. No surprises in CI if the PR validations are the same as those in CI.

RevolutionTech commented 3 years ago

...run the tests on the merge commit prior to pushing...

This is how I've parsed the above: Create a feature branch based off of the main branch and open a pull request. Then once the PR is approved, merge your feature branch into the main branch using your local git client. Then perform any necessary checks locally and only if everything goes well then push the main branch to the remote.

I'm guessing that I've misunderstood what you're saying because the above workflow would seem to defeat the purpose of using merge-bot since every merge would require manual steps in the local git client.

Would you mind rephrasing or maybe using an example?

In case I didn't express what I intended before, here is a contrived example of the scenario I'm talking about:

  1. Developer A creates a feature branch off of the main branch
  2. Developer B updates the main branch with a commit that adds some logic where the existence of foo.txt in the repo causes a failure. Assume that this logic is executed in a test and so therefore would cause CI to fail if foo.txt existed.
  3. Developer A adds foo.txt in their feature branch.

Without requiring feature branches to be up-to-date with the main branch before merging, both the main branch and the feature branch above could pass CI since the main branch doesn't contain foo.txt and the feature branch does not have logic checking for foo.txt. There are also no git conflicts in this case. However, when the feature branch is merged into the main branch, CI will now fail.

If feature branches must be up-to-date with the main branch, then this problem is avoided because once the main branch is merged into the feature branch, CI will fail in the feature branch instead.

AArnott commented 3 years ago

Thanks for being patient with me, @RevolutionTech. I wonder if this is from my ignorance of how "merge-bot" works. I don't use it myself, and I can't remember how I even found this issue.

In my (non-merge-bot) world, your 1, 2, 3 scenario above ends with 4:

  1. Developer A creates a PR from the feature branch to the main branch.
  2. Validation runs on the provisionary merge commit created by the (still active) PR, and fails because in the merge commit brings together the new validation from main with the new file in the feature branch.
  3. Developer A observes the failure and fixes it and pushes an update to their branch. This update may include a merge from main.

An alternate (more common) ending is that nothing should fail in the merge and the PR is completed. The git history has no extraneous merges, and it's clear from history that the two branches were developed in parallel prior to their merging.

RevolutionTech commented 3 years ago

Thank you @AArnott for elaborating to help me understand.

  1. Validation runs on the provisionary merge commit created by the (still active) PR...

This is the key part I was missing -- I've never realized that the pull_request build option in CI tools like Travis CI and Github Actions create a provisionary merge commit. That completely blows my mind to be honest.

I think that feature does solve the use case I mentioned above, although practically speaking I think a similar issue could occur if the CI passes on a provisionary merge commit and then the incompatible changes reach the main branch. That would make the CI for the feature branch effectively stale, but because it would have passed previously the developer could still merge the PR.

Perhaps there are mechanisms for forcing CI to be run again on the new provisionary merge commit before the PR can be merged, but to me that intuitively feels equivalent to requiring feature branches to be up-to-date with the main branch. Or have I missed something?

squalrus commented 3 years ago

The reference in CI of the "merge" from a pull request is typically something like refs/pull/15/merge, which is the "merge of pull request 15 into the main branch".

Some CIs may offer the ability to re-run when the base is updated, but depending on the team size, this may not be practical. My team is ~30 people that are typically trying to integrate work during the same time (with a CI + tests that can take up to 50 minutes) and to have to wait in line for sequential integrations would be very frustrating and near impossible to get work integrated. Granted, we are often working in different areas of the repository and this is a team-specific challenge (yours may be different).

Honestly, this works for us 99% of the time, and with the validation and tests running for pull requests, in the main branch, and at time of release we are able to catch the 1% before it is released.

I enjoy hearing about others' work flows -- thanks for sharing and the discussion!

AArnott commented 3 years ago

I think a similar issue could occur if the CI passes on a provisionary merge commit and then the incompatible changes reach the main branch. That would make the CI for the feature branch effectively stale, but because it would have passed previously the developer could still merge the PR.

True. There is that chance. A PR that passed tests could me merged a month or even a year later, long after we might consider the validation long since stale.

but to me that intuitively feels equivalent to requiring feature branches to be up-to-date with the main branch.

The difference is in git history after the PR is merged. With a policy that requires the topic to have already merged main into itself first, every merged topic branch now has two merges in git history instead of just 1, which you can safely achieve by relying on the provisionary merge commit validation.

And as @squalrus says, some cloud offerings offer mitigations for the risk of a stale validation. Azure Repos for example lets you reset all validation immediately when the target branch is updated, or after x hours: image

But also as @squalrus said, for most teams this is overkill and just slows down merges. PR builds will catch nearly everything, and if two PRs are co-active, validate, and then merge yet are incompatible with one another, the CI build will pick up on it right away and that can be fixed. I'm on a team where hundreds check into the default branch, regularly, and we have hit the very rare case a few times, but setting a policy to require the validation to be current with the target branch at the time the PR is completed would bring work nearly to a halt.