I’m proposing in Katello we enable squash and merge button for PRs. Currently, we only enable rebase and merge.
The current typical workflow looks like this:
Contributor pushes up a commit to their branch, makes a PR
PR gets reviewed and changes are asked for
Contributor makes changes locally and force pushes the same commit
PR gets re-reviewed and this cycle continues
rebase and merge on approval
This works fine in small PRs, but when you have a large PR, its hard to see the changes since the last time you have looked at the PR as a reviewer since its still in the same commit.
With squash and merge, you can push as many commits as you like to the PR and use github’s squash and merge button to combine them at the end. It gives you the option to edit the commit message before squashing the commits into one and merging.
The workflow for squash and merge looks like this:
Contributor pushes up a commit to their branch, makes a PR
PR gets reviewed and changes are asked for
Contributor makes changes locally in a new commit and pushes up that commit
PR is re-reviewed, reviewer is able to see the new changes isolated in a separate commit
cycle continues, new commits are added
Contributor squashes and merges commits into one, altering the commit message if needed from Github.
Note that I’m not saying we enforce this method, it’s just nice to have this workflow available. For those who prefer force pushing, no change needs to happen in their workflow as the rebase and merge button is still available.
I also realize you can currently push up multiple commits, rebase -i from the command line, and force push when the PR is ready, but ideally, we enable github’s feature that makes this easier and less error prone.
When enabled, it looks like the squash and merge button is shown as default, so rebase and merge will be more of a deliberate action. This is helpful to prevent someone from accidentally clicking rebase and merge when they have multiple commits. (maybe github thought of this scenario in their design?)
I think that this could improve the PR reviewing process in Katello, curious to hear what you all think, please vote in the poll below.
Keep ‘rebase and merge’ as the only option for Katello
Enable both ‘squash and merge’ and ‘rebase and merge’ for Katello
I’m all for it. One question is that if you’re given the option to edit the commit message when squashing - could we potentially have some commits with messages that the prprocessor would have rejected or can we have it also validate those edited commit messages?
That’s a good question, I’m guessing in its current state, the PR processor won’t check the commit message after editing it in the squash and merge option. I also am not sure it would be possible to add this functionality, since the edit, squash, and merge seems to be one operation (just speculating, I haven’t checked).
Generally, when one squashes and merges, you delete or modify the new commit titles, which are now part of the commit body, and keep the oldest commit’s title, which is the title of the single commit that is being merged. If I have a chance I’ll create an example screenshot to share, but its similar to an interactive rebase locally. I think we could assume that its the responsibility of the merger to keep this commit title the same, which will include the proper info for automation.
Another tangentially related thing-to-note is the pr processor currently runs on all commits pushed up to a PR, so they have to be correctly formatted. Currently, you need a “Fixes #” or “Refs #” on every commit pushed up or it will complain. I’m not sure if we would change that or not, but that can be a follow up discussion if we enable squash and merge find it annoying to format the new commits that will be squashed anyway.
One thing the Squash & merge workflow does by default is append the Github issue number to it as (#xxx). This can create odd interactions with Redmine where we use different numbers. That is the biggest risk I’m concerned about. Other than that I do like the option to change the commit message when merging.
If your workflow involves interactive rebases (git rebase -i --autosquash), then it is quite common to do git commit --fixup=abcdef123 which yields a new commit message starting with Fixup! <old commit message>.
I read that as before you merge, i will squash those commits. So it would be nice, if the pr processor could at least ignore those commits, or just block merging without sending emails about improper commit messages.
It does, but the (#xxx) number is the Github issue/pull request number. Those can overlap with Redmine numbers. You can manually remove it, but you do have to remember to do so and we can’t gate it. An example of what it produces:
@ekohl thanks for clarifying, do we know if the prprocessor or any automation would pick up on the appended number? Would that number be ignored since it doesn’t have the ‘Fixes’ or ‘Refs’ keyword?
I click squash and merge, github gives me the option to edit the commit message and title. Additional commit titles are added in the body of the message
Regarding showing a default button, it sounds like github defaults to the last used button by the user. So if you used squash and merge last, this will show as the default option in your pull request. I haven’t found anything official on this and I’m not sure if that is scoped to repo or globally.
I don’t know what Redmine does if you have a line like Fixes #12345 - do something (#321). That one is very likely to show up in our history. It’s also very confusing and other automation could get confused. tool_belt can check whether an issue was in the release and will warn if there’s a mismatch.
Now you can remove it in the edit message part and there’ll be no problem. I do think that it’s very likely someone will forget it and then it’s in your history. It’s almost guaranteed you will need to deal with it at some point.
I’m fine with squash and merge as long as there’s only one author on the PR in question.
If there are multiple contributors on a PR, then it’d be nice if there was a collective agreement to not squash that PR to avoid losing who contributed what.
The squash and merge workflow is pretty convenient, but what I don’t like about the approach is that we have no way to test or enforce the commit message style. prprocessor checks the commit message style before merging, with the sqash and merge workflow every committer has to pay attention to clean up what github proposes (like adding the PR number to the commit message).
Since the feature is enabled in core, the git commits have become a lot less clean then they used to me.
Generally, as long as the first commit is the properly formatted issue commit I think one should be fine. Squash and merge gives maintainers the power to merge code without asking a developer to squash but puts the onus on the maintainer to properly format the resulting message without breaking automation. TLDR; More human responsibility with the power to not wait on a developer. Therefore, up to the project to decide if htey want that trade off or not.
Judging from the poll looks like overall most are in favor of squash and merge. It’s good to hear that the appending of the PR doesn’t appear to affect automation. I think @ehelms put it best saying we are trading off the power to not wait on a developer (and I’ll add improved PR reviews) for a bit of human responsibility. It’s up the maintainer merging the PR to make judgment calls about editing the commit message and maintaining commit ownership when using squash and merge.
Keep in mind that rebase and merge will still be available (if we got with the current leading option in the poll) and can still be used. I think squash and merge is most helpful in medium-to-large PRs where iterations of changes get lost and plan to use it there.
Also, like most things, we can revert this change if we find any adverse effects from squash and merge.
The poll is ending in 10 days, so there is still time to get your opinions in if you have not yet.
A few things to keep in mind for contributors and maintainers:
Additional commits in a PR still need a “Refs|Fixes #ISSUE_NUM” before it to not cause the PR processor to complain.
When merging and editing the commit message, its your responsibility to make sure the commit message will still work with automation. This means keeping the Fixes|Refs in the commit title.
You do not have to change your current workflow - Nothing has been disabled, just an additional option is added. Force pushing and using rebase and merge is still fine and works as it always has.
Please reach out if there are any questions or anything is unclear
