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

Add "Update with rebase" button #3407

Open
kolyshkin opened this issue Mar 8, 2022 · 6 comments
Open

Add "Update with rebase" button #3407

kolyshkin opened this issue Mar 8, 2022 · 6 comments

Comments

@kolyshkin
Copy link
Contributor

We were seeing 2 or 3 cases when merging two pull requests results in a non-compilable code in main branch. This is because PRs are usually not rebased.

I have now enabled a button to update a PR to the tip of the branch. Previously this button added a merge commit, which I don't like. Now there's an option to do a rebase with force push. Unfortunately this is a second option so you have to explicitly select it.

⚠️ @opencontainers/runc-maintainers, please use the "Update with rebase" option, NOT the default "Update with merge commit".

Here's a short blog post by github about the new feature: https://github.blog/changelog/2022-02-03-more-ways-to-keep-your-pull-request-branch-up-to-date/

I suggest all admins to use this before merging a PR. I have tried it myself on #3404 and it works as expected (no merge commit is created, no LGTMs are lost).

Pros:

  • 🟢 ability to test a PR against tip of the branch
  • 🟢 no LGTMs are lost
  • 🟢 no merge commit is created

Cons:

  • 🔴 if the author wants to work on the branch, they have to do a fetch and a hard reset (or a local git pull --rebase)

Because of 🔴, I think the best way to use this feature is when the PR is ready to be merged, i.e. no more changes are expected.

@thaJeztah
Copy link
Member

I prefer not using this feature, as doing so modifies the contributor's commit, and removes their GPG signing.

We should check our CI configuration, as a rebase should not be needed to test against 'latest' of the target branch; if I'm not mistaken GHA (as well as other CI systems) should usually do a merge with target branch before running (if that's not the case, we should fix that).

IOW, if the above is true, then "restarting" CI should trigger a merge, and test against the latest changes.

What we can do in that case (but this requires all tests to be marked as "required", is to use the auto-merge option;

  • trigger CI to run again
  • enable auto-merge (which merges the PR once CI completes)

@kolyshkin
Copy link
Contributor Author

We should check our CI configuration, as a rebase should not be needed to test against 'latest' of the target branch; if I'm not mistaken GHA (as well as other CI systems) should usually do a merge with target branch before running (if that's not the case, we should fix that).

Alas, Looks like it's not the case. The GHA action used (https://github.com/actions/checkout) fetches the code from the last commit in a PR, and does not do a merge. I took a quick look and do not see a way to implement it.

I see a number of open issues about it, e.g. this one actions/checkout#481 from @saschagrunert.

I am not saying this can't be implemented, I am saying it is not implemented now.

What we can do in that case (but this requires all tests to be marked as "required", is to use the auto-merge option;

The required checks are already marked as required for all branches, and I am maintaining it. Yet, auto-merge won't solve the issue, as we're not testing against the HEAD of the branch -- it's just a way to not wait till the tests are finished.

I see three ways of solving this:

  1. using the "Update with rebase" / "Rebase branch" button.
  2. asking PR author to do git pull --rebase and git push -f.
  3. implementing a GH action which tests a PR against the HEAD of the branch.

I guess that 3 would be the ideal solution, but don't have resources to work on that myself. If anyone else can tackle this I'd be happy to adopt their solution, of course.

For now, I guess, we can either use the button (1) or see if it appears and ask the author to rebase (2).

The alternative is to ignore the issue and fix the repo post-merge. We did it a few times already; personally I don't like a non-working HEAD.

@tianon
Copy link
Member

tianon commented Mar 10, 2022

Related to actions/checkout#481, restarting a job will always re-run the same YAML as the original run (and there's nothing something like actions/checkout can do about that), so if you've made any YAML changes in the target branch then a rebase or merge is required to pick them up. 😞

@thaJeztah
Copy link
Member

Ugh.. 😞

I'm confused by all of these; so I recently was discussion this (similar conversation) with @crazy-max (he had some ideas I think to fix the checkout action to do a merge), but @ndeloof mentioned that it already does that (or something along those lines).

To some extend, I guess it's "fine"(ish) if it didn't pick up the changes from the YAML itself (not ideal, but with Jenkins that also doesn't always work 100%, and I would consider that more of a "calculated risk"), as long as it does a proper merge of main into the PR when restarting. CI

Yet, auto-merge won't solve the issue, as we're not testing against the HEAD of the branch -- it's just a way to not wait till the tests are finished.

Yeah, so I meant: if it does do a proper merge when restarting, we could "trigger a rebuild" (so implicitly, "merge, and run CI"), and have GitHub deal with the merging once that completes.

@thaJeztah
Copy link
Member

Oh! Actually wanted to comment about that, but forgot;

restarting a job will always re-run the same YAML as the original run (and there's nothing something like actions/checkout can do about that)

This is indeed a known issue with Github Actions; if changes were made in the .github/workflows YAML files, they don't get picked up automatically. I've also ran into that in repositories that were switching to GitHub actions from other CI providers; the newly added actions either don't show (if they're not marked as "required"), or do show (if they're marked as "required"), but show as "pending".

A dirty workaround for that is to close and reopen the PR; doing so triggers GitHub to scan for the actions in the repository, and will then run the latest version of the target (main/master) branch.

Would be great if GitHub had a button to "re-scan" (to pick up new or updated actions) and "re-trigger" CI

@AkihiroSuda
Copy link
Member

Can we use merge queue ?

https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/configuring-pull-request-merges/managing-a-merge-queue#about-merge-queues

The merge queue provides the same benefits as the Require branches to be up to date before merging branch protection, but does not require a pull request author to update their pull request branch and wait for status checks to finish before trying to merge.

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

4 participants