Protecting main branch from wild push

[Keenuts]

The main branch is currently not protected, meaning anybody with write permission can push to it. As far as we know, this was only useful for the release process: one could create release commits to update changelog without having to wait for a review cycle. Maybe we could do without.

Proposition:

• protect the master branch
• prevent merge without a green CI

The second point is debatable, but would force us to maintain a green CI.

[alan-baker]

Definitely in favour of the first point. For the second I would omit a green requirement on the shaderc smoketest at least.

[Keenuts]

Definitely in favour of the first point. For the second I would omit a green requirement on the shaderc smoketest at least.

Aren't you afraid of the "red test fatigue"? The log is long, and we could easily miss a real failure in-between the multiple version failures to take today's example.

[alan-baker]

That test is an integration test though. Sometimes failures require multiple repos to be updated. It seems unnecessarily onerous that SPIRV-Tools PRs can't be merged because a bug exists in multiple places.

[Keenuts]

Ok, thanks for the input, so what about those rules:

• Require a pull request before merging: true
• Require approvals: 1
• Dismiss stale pull request approvals when new commits are pushed: true to prevent accident commit of a new file when rebasing)
• Require review from Code Owners: true, even if we don't use those right now.
• Allow specified actors to bypass required pull requests: false
• Require status checks to pass before merging: false For the CI, it looks like we cannot say "requires green, except for Kokoro's job X", so we'll have to allow failures.
• Require branches to be up to date before merging: false
• Require conversation resolution before merging: false
• Require signed commits: false
• Do not allow bypassing the above settings: true (or review who has those rights)

(https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/defining-the-mergeability-of-pull-requests/managing-a-branch-protection-rule)

[alan-baker]

That sounds reasonable to me.

@dneto0 @dnovillo any thoughts?

[dnovillo]

That sounds reasonable to me.

@dneto0 @dnovillo any thoughts?

+1 - This looks reasonable to me.

[alan-baker]

Given #4981 maybe we should change:

Require branches to be up to date before merging: false

To true for a month or so to avoid merging based on the old headers.

[Keenuts]

Given #4981 maybe we should change:

Require branches to be up to date before merging: false

To true for a month or so to avoid merging based on the old headers.

I was hesitant on that. Because enforcing that & approval drop on push means:

• A sends PR
• B sends PR
• both are approved
• A merges

• B now has to rebase & get approval again.

  If it's only for this PR, could alan just make sure it's using the latest version before pressing merge? (Prone to human error, but just afraid the drawbacks of enabling this options on the whole repo could lead to frustration)

[alan-baker]

I can certainly ensure that PR is up-to-date, but if others have PRs that are based before it, the bots won't flag them as needing rebased.

[Keenuts]

Ok, see your point.

@vettoreldaniele since you are admin: could you apply the following rules (https://github.com/KhronosGroup/SPIRV-Tools/issues/4977#issuecomment-1300306177) except the "Require branches to be up to date before merging" which should be set to true ? (Adding reminder to disable this in a month)

[vettoreldaniele]

I applied those settings + required branches to be up to date before merging set to true.

[Keenuts]

@alan-baker : is it OK for you if we drop the requirement for up-to-date branch now?

[Keenuts]

@vettoreldaniele do you have the rights to change this setting back to "don't require"?

[vettoreldaniele]

@vettoreldaniele do you have the rights to change this setting back to "don't require"?

Done.