core-plans
core-plans copied to clipboard
habitat-sh
Suggestion: Require branches to be up-to-date with master before merging
Github has an option to only accept merging of PRs that are current with respect to the master branch:
This is advantageous since the project's CI config is now checked into the .expeditor folder; as it ensures that all changes are checked with the very latest version of CI (and not the version of CI at the time the contributor created their branch).
It also somewhat protects against clashing changes that work individually but break in combination (I suspect this is less likely for this repo given it's nature).
There is obviously a little extra work required because every PR needs rebased as soon as another PR gets merged. Github provides an "Update branch" button on the PR so that the reviewer can independently update the PR branch, however I don't know if this works with forks...
This issue is to test out if the branch update button is available for forks and capture feedback - should we enable this option, or not?
I suspected this would not work with forks, but I tested it on another project - a colleague with no permissions to my fork is able to click "Update branch" on my PR and it does refresh the branch on my fork. Which is good (though surprising?)
I love the idea of this, however I was recently reminded that merging from master into a branch doesn't play nice with Builder, which is what this appears to do (rather than a rebase).
The reason for this is, as I understand it, when you do that, all of the commits that were merged become part of the PR. When the PR is merged, Builder is currently unable to determine that it has seen the commits before and will start building all packages that were modified in the merge.
In the case where the branch is many commits behind, which is an easy state to get into if a reviewer is approving multiple PRs at a time, we'd end up with a large number of redundant builds.
@smacfarlane I tested that out and you're right: https://github.com/james-stocks/core-plans/pull/1 The Update Branch button in the Github UI creates a merge commit instead of rebasing.
That's a problem. But having PRs pass against old CI checks is also bad.