feat(project-repo): standardize github settings and commit types #45
Conversation
There was a problem hiding this comment.
Thanks for formalising this!
My comments are around discussing on the "finale roundtrip" on which I’m not fully for and against it, as there are advantages and drawbacks in both approaches.
On the settings (for branch protection): we generally have short circuit fallback to allow some people to force push if needed to main (zomg). This is generally to unblock situations and be a little bit like a disaster recovery, I don’t know if you want to include this here?
As an aside, we should also standardize around the "Code Security" scanning for both active scanning and dependabot settings.
|
|
||
| - [x] Require a pull request before merging | ||
| - [x] Require approvals | ||
| - [x] Dismiss stale pull request approvals when new commits are pushed |
There was a problem hiding this comment.
That’s the part incompatible with the workflow itself. If we allow both, maybe we should set that to optional and add a note for it? But I think this depends on the discussion below :)
| 1. The submitter will squash the optional additional commits and force push to the pull request. | ||
| 1. The submitter examines one last time the diff, the test results, and the coverage. | ||
| 1. The submitter is in charge of merging the changes by clicking the merge button. | ||
| Finally, the submitter should merge the changes by clicking the merge button. |
There was a problem hiding this comment.
I am with you on this, but some team member rely on squashing to rewrite history before merging so much, and we discussed that back and forth. Maybe we can advise a default workflow, but then, if the reviewer wants to resquash, still trust them? So far, from memory, we only had one booboo in all our PRs during rebasing… (and I am not even sure if a last finale look would have caught it)
|
|
||
| - [x] Allow merge commits (default commit message: "Pull request title and description") | ||
| - [ ] Allow squash merging | ||
| - [ ] Allow rebase merging |
There was a problem hiding this comment.
Mh, personally in some projects I'd prefer to allow using a linear history (we do it already in various ubuntu/* projects that has various benefits (mostly for bisecting and testability of each commit). Thus this option can be useful in those contexts.
|
This has lost a bit of momentum due to other priorities, but if we want to continue thinking about establishing a standard process, we'd probably need to agree on a few constraints first (judging by the comments so far):
|
|
In an ideal world, that would be a process shared company-wise. As it’s not realistic to expect that from the get go, I think the only possible option is the other way around: onboarding squads on them, one after another. We are quite strict at imposing that in the enterprise desktop team and WSL and following it (with the last rebase trusting the submitter though, which only created one regression due to the rebase itself in the past 3 years, which I think is more than a good score with the flexibility this provides). |
We've recently discussed review process standards and corresponding github settings that help support this process in one of our weekly meetings with @didrocks. This PR contains a few proposed changes and simplifications that came out of this meeting.
I'd be happy to iterate on this a bit further in case someone has concerns or more ideas.
UDENG-5073