Comments (17)
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?
from readme.
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.
from readme.
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.
from readme.
@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.
from readme.
@brainbicycle - Ah, i didn't catch that that was just meant to be applied to release
! Makes sense.
from readme.
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.
from readme.
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 👍
from readme.
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:
- It's the recommendation in our workflow docs
- 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.
from readme.
(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!
from readme.
@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.)
from readme.
@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.
from readme.
@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
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.
from readme.
👎 on making branch rules more restrictive - surely there's another signal we can use to execute that idea.
from readme.
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.
from readme.
@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.
from readme.
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.
from readme.
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.
from readme.
Related Issues (20)
- [RFC] Feedback Friday time reschedule HOT 2
- RFC: Catch more WTFs during onboarding HOT 2
- RFC: Protect main/master branches HOT 5
- RFC: We are all solely responsible for ensuring that we are not disturbed outside of working hours HOT 16
- RFC: Incrementally adopt I18n library in Rails projects HOT 11
- RFC: Adopt Codecov at Artsy, starting with Gravity HOT 8
- RFC: Adopt inclusive language for repository naming as well as allow/deny lists HOT 12
- RFC: Rename product slack channels to `prd-*` HOT 17
- RFC: Host one Hackathon per quarter in 2022 HOT 8
- RFC: Host one Codebase Refinement per quarter in 2022 HOT 11
- RFC: Officially recommend against using GraphQL Stitching in Gravity HOT 19
- RFC: Reusable components HOT 21
- RFC: Updating Best Practices Documentation HOT 10
- RFC: Retiring Torque HOT 1
- RFC: Feature Flags Naming Conventions / Maintenance HOT 14
- Want access of Web & Mobile best practices documentation
- RFC: More Relaxed CodePush Usage for Folio HOT 4
- RFC: Consolidate Eigen feature flags HOT 22
- RFC: Decommission Volt 2 and roll code back into Volt 1 HOT 1
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from readme.