-
Notifications
You must be signed in to change notification settings - Fork 120
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
RFC: disallow squashing and rebasing on PRs #516
Comments
I like the rationale behind this RFC, that a squashed PR ensures that each PR is runnable, and in a more easily debuggable state, but I've only found this to be truly valuable on Eigen (and Folio), which I would suggest we leave alone. If those repos are removed from this RFC, i'm fine with it 👍 |
I can't say I'm in favor of this. We had a couple of issues in the past with squashing-and-merge when deploying (as far as I know), and this RFC is totally valid. However, I don't like the idea of not having the squash-and-rebase option. I would vote for disabling this option only for Deploy PR's. But that seems like a dream feature for Github. |
I'd think this RFC goes beyond adjusting the project's settings, I guess it's about redefining what's our preferred practice engineering-wide. My personal preference would also be using merge instead of squash for regular (non-deploy) PRs. However, I've always used the squash option when merging non-deploy PRs at Artsy for two reasons:
|
(Didn't know about https:/orgs/community/discussions/10809, but I've now up-voted it, hard.) Like others, I like to have scannable commits in On the other hand, being able to deploy our most critical services is absolutely essential, and squashed releases have interfered with that on a regular basis (~biweekly?) since the squash convention was introduced. We have been able to recover using a semi-standard set of steps, but cumulatively it's required hours of manual intervention. For that reason, I'm in favor of disabling squashes (and updating our workflow docs here and apparently in Potential to match). I'd rather volunteer to time to help contributors fix up PR commits than spend it resolving release conflicts. Alternatively, maybe the proponents of squashing should also be responsible for resolving release conflicts? I think at the moment there is some joy felt around squashing, and also some pain, but by completely separate groups! |
Opposed to this as is, but more specifically for Eigen. Squashing commits makes reverting entire features easy: artsy/eigen@54b4413 If we want to disable for repos besides mobile repos I am more okay with it but still feels like we are adapting a lot of engineering processes due to the lack of a feature in GitHub. Wondering if there are any other options to make this less painful or common, just throwing out ideas don't know how feasible:
|
@lidimayra brings up a good point. @jonallured, as someone who is opposed to the conventional-commit format (which in the end is irrelevant when combined with PR titles and squashing, as Lidi mentions), would you be willing to update your git practices? To be clear, this RFC shouldn't indirectly undermine the other RFC, which has been a win in many regards around standardizing commits, PR titles, and more. And on mobile, it has made that environment vastly more reliable, as @brainbicycle mentions (reverts happen there more than anywhere else just by nature; mobile is sensitive). (But all that said, I think deployability is much more important. I'm still very unsure of what to do when things get in a conflicted state.) |
@brainbicycle regarding your suggestions:
This is easy enough--see artsy/horizon#592. I'm not convinced folks will read that warning, but it can't hurt.
This is possible, but too late. The
I just tried to tune a rule to prevent most of us from merging PRs on |
@joeyAghion I think it should be possible, I did a test in echo: I am still not quite clear on the best way to bypass to perform the actual merge, initial thought was a label could be added that triggers ci which checks out the PR branch, merges locally to release and force pushes. Not sure if the 2nd setting would interfere with that. Without that setting this is what I get: Which is still a big improvement in my mind because you need to explicitly override with the checkbox and combined with a message not to do that seems like it would prevent most cases as opposed to the big green button. |
I'm 👎🏼 generally as I would prefer to retain the flexibility to rebase-merge or squash-merge other PRs. I like the idea of utilizing commit statuses / branch protections and I think there might even be a simpler option available. For the So the new flow would be for an engineer to approve the PR once ready to merge. Horizon then takes over and merges on behalf of the engineer. It looks like GitHub has added a new option recently to disallow bypassing the configured protections for a specific branch, even for administrators. |
👎 on making branch rules more restrictive - surely there's another signal we can use to execute that idea. |
I think squashing PRs is very practical when merging to So I would vote to keep the ability to use squash-merging as part of our dev workflow. In many cases it keeps units of work nicely grouped in the |
@damassi what is the issue with making the |
How would automated deployment work under your proposal @dblandin? Would it be able to override the approval requirement or approve the PR itself before merging? Auto-deploy via Horizon is the norm in most of the repos I work with and I wouldn't want to lose that capability. |
@cavvia Ahhh — good reminder about auto-deploy. I suppose Horizon could approve the PR before merging if we wanted to leverage that extra "do not allow bypassing" protection. Otherwise, Horizon-backed auto-merge/deploy could work normally without it enabled. |
@brainbicycle - Ah, i didn't catch that that was just meant to be applied to |
Hey pals, I'm going to close this one but thank you so much to everyone for their input! I think we got to some really cool ideas about how to improve our deployment process so that these broken deploy PRs aren't possible and that makes me so happy. I have opened a ticket here to follow up on that part: https://artsyproduct.atlassian.net/browse/PHIRE-176. |
Proposal
I'd like to disable squash and rebase as options for Pull Requests.
Reasoning
Deploy PRs must be merged using the
Merge pull request
version of the merge button but we routinely make the mistake of using the wrong version of this button. I have personally fixed this situation enough times to get upset and open this RFC - how about you?Exceptions
None.
Additional Context
The button to merge a PR can look one of 3 ways:
However, we can remove those last two options by unchecking them here:
How is this RFC resolved?
Unless somebody knows a better way, resolving this RFC would mean going to each repo and updating the settings. I'd settle for updating the Big Five:
The text was updated successfully, but these errors were encountered: