RFC: disallow squashing and rebasing on PRs about readme

[brainbicycle]

Opposed to this as is, but more specifically for Eigen. Squashing commits makes reverting entire features easy: artsy/eigen@54b4413
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:

• update deploy PRs to remind us not to squash?
• Can we update the deploy process itself to fail fast when a PR is squashed vs merged? and maybe send out an alert?
• Can we protect release branches (that is disable the button completely) and trigger deploys another way that guarantees we don't squash?

[dblandin]

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.

[egdbear]

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.

[dblandin]

@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.

[damassi]

@brainbicycle - Ah, i didn't catch that that was just meant to be applied to release! Makes sense.

[dblandin]

Just learned a few more things about branch protections and API requests:

• An API request (direct or via Octokit) to merge a PR will automatically use any admin/owner privileges to bypass any pending protections
• If you enable the "do not allow bypassing" rule, an API request will error with a message like the following:

  405 - At least 1 approving review is required by reviewers with write access. // See: https://docs.github.com/articles/about-protected-branches (Octokit::MethodNotAllowed)
  

• GitHub enforces that a PR author cannot approve their own PR and this is also enforced on API requests. Such an API request will error out with the following error message:

  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

  • Horizon GitHub user added here

• 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.

[damassi]

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 👍

[lidimayra]

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:

1. It's the recommendation in our workflow docs
2. The conventional commits format on the main branch is needed for our logs and work tracking. I'm not a fan of the format for my individual commits (a bit more about this here). So squashing the PRs is the ideal way for me to be compliant with our standards, since I apply the conventional commits only on the PR title. Once squashed, the main branch will contain the conventional commits, but on the PR level, I'll still have access to the history in the way that works best for me.

[joeyAghion]

(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!

[damassi]

@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.)

[joeyAghion]

@brainbicycle regarding your suggestions:

update deploy PRs to remind us not to squash?

This is easy enough--see artsy/horizon#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.

[brainbicycle]

@joeyAghion I think it should be possible, I did a test in echo:
artsy/echo#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.

[damassi]

👎 on making branch rules more restrictive - surely there's another signal we can use to execute that idea.

[cavvia]

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.

[brainbicycle]

@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.

[cavvia]

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.

[jonallured]

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.