Closed jonallured closed 1 year ago
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://github.com/orgs/community/discussions/10809, but I've now up-voted it, hard.)
Like others, I like to have scannable commits in main
, but I can't say I've relied on that for anything critical. I also appreciate the analysis (e.g.) that conventional commits enable, but--again--that's a nice-to-have.
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: https://github.com/artsy/eigen/commit/54b4413e6a37ee0964755be49d75c544ac72ace4 And I appreciate the clean history.
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:
update deploy PRs to remind us not to squash?
This is easy enough--see https://github.com/artsy/horizon/pull/592. I'm not convinced folks will read that warning, but it can't hurt.
Can we update the deploy process itself to fail fast when a PR is squashed vs merged? and maybe send out an alert
This is possible, but too late. The release
branch is already corrupt in relation to main
, so all the same tedium is required to recover.
Can we protect release branches (that is disable the button completely) and trigger deploys another way that guarantees we don't squash?
I just tried to tune a rule to prevent most of us from merging PRs on release
, unsuccessfully. Understandably, admins can always merge and engineers have admin access to most repos. (Branch protection rules are powerful and complex though, so maybe someone else will know how.) This would leave us dependent on automatic merges by Horizon, which isn't always enough, but it's an interesting prospect.
@joeyAghion I think it should be possible, I did a test in echo:
https://github.com/artsy/echo/pull/512
And the merge button is greyed out for me despite being an admin.
there are a couple settings:
Require status checks to pass before merging
which is currently set to a status check that always fails.
and
Do not allow bypassing the above settings
Here is what it looks like:
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 release
branch protections, if we were to enable the Require approvals
option — engineers would be unable to merge without first supplying a PR approval. At the same time, we could teach Horizon to listen to any approval for a PR with a target branch of release
and immediately merge with the appropriate strategy.
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 main
since git commit message hygiene is far from ideal when people are working on a dev branch. It's much quicker to squash the PR in the UI than to do interactive rebasing locally to squash commits, which is what I used to do before this feature. That convenience far outweighs any issues I've had with deploys. Those issues have caught me out a couple of times but it's easily recoverable as per the notion doc. I have not spent more than 5 minutes recovering the state of the release branch when it has occurred.
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 main
branch and makes git log
more easily scannable by the human eye. Relying on automated deployment as much as possible also helps, obviously.
@damassi what is the issue with making the release
branch rules more restrictive? in my mind if we want the thing to happen the same way every time we should have a machine do it, do we need manual merges to release
for any reason? And I think you can always force push. the rest of the branches would stay as is of course but maybe I am missing something because I don't work in these repos as often.
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 release
! Makes sense.
Just learned a few more things about branch protections and API requests:
405 - At least 1 approving review is required by reviewers with write access. // See: https://docs.github.com/articles/about-protected-branches (Octokit::MethodNotAllowed)
422 - Unprocessable Entity (Octokit::UnprocessableEntity)
Error summary:
Can not approve your own pull request // See: https://docs.github.com/rest/pulls/reviews#create-a-review-for-a-pull-request
But, GitHub has another option to leverage:
"Allow specified actors to bypass required pull requests"
The wording is a bit strange but it essentially means that only the users we specify can bypass the configured branch protections.
How the PR looks to those users:
How the PR looks to everyone else:
This means that we can add the following branch protection rules on the release branch
to:
Require a pull request before merging
Require approvals (at least 1)
Allow specified actors to bypass required pull requests
Do not allow bypassing the above settings
Horizon would still need to listen for approval events and respond accordingly but that should be pretty simply to wire up with a new API endpoint + a webhook configured on GitHub.
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: