[TODO] Update branch protection to require the check job for main and 8.3.x branches

[webknjaz]

In #10315 + #12488 + #12517, I added a CI gating job. During the sprints, I suggested that we don't add it as a required check for some while to allow more older PRs to produce this status.
I think the time has come to complete the integration. I'm assigning Ronny because he has enough repo access and we talked through the context in person.

• Add a single requirement for the status called check in the branch protection
• Delete all the GHA-required checks in the branch protection. DO NOT delete any non-GHA requirements (like those from GitHub Apps and other integrations)
• Make sure this is done for both main and 8.3.x
• Bonus: attempt migrating from branch protection to rulesets (but that's likely out of the scope and should be separate). It is possible to make a ruleset but not enable it — keep it in the "monitoring" mode so that it'd collect stats and show them in the repo settings without actually enforcing the restrictions for a while.

[webknjaz]

Whoever has enough privileges — it's time to do this. The grace period has passed, and any important PRs should have a check status reported by now.

[webknjaz]

@nicoddemus @RonnyPfannschmidt @Pierre-Sassoulas can anybody with access complete this task? At least, the first 3 points, excluding the bonus one. This should take under 10 minutes.

[RonnyPfannschmidt]

I'll add it to my morning schedule, thanks for the reminder

[RonnyPfannschmidt]

branch and tag rules where deprecated and the new ruleset stuff doesn't support regex

i created the ruleset require-commit-checks which matches the default branch and all future release branches

i configured it so it allows the creation of branches even without check

[RonnyPfannschmidt]

i update the tag ruleset and refined the commit check ruleset, - im going to drop the old branch rules now

[RonnyPfannschmidt]

@nicoddemus @webknjaz i completed creating the new rule sets rules and dropped the old branch rules + fixed the tag rule sets - i'd love another set of eyes on those settings to ensure i didn't miss anything

[Pierre-Sassoulas]

I recently had to drop py38 job and added py313 job in #12874 too.

[RonnyPfannschmidt]

@Pierre-Sassoulas the new rules use only the check job (which depends on all the others) - so the branch rules are more simple

[nicoddemus]

Thanks @RonnyPfannschmidt for looking into it!

I have a few questions about protect release tags:

1. Shouldn't we enable Restrict deletions and Block force pushes?

2. Also Restrict creations is checked, won't that prevent our deploy workflow from creating tags?

Other than that everything looks reasonable to me!

[webknjaz]

I was recently playing with this a lot and settled on having several smaller rules that cover different aspects...

@RonnyPfannschmidt I think the wildcards are supported in the base inclusions. But there was also a way separate section in the bottom parts of the rules that literally suports regexes and can be used to refine allowed naming further. Although, that might only be available in the enterprise orgs. But let me double-check...

[webknjaz]

Also Restrict creations is checked, won't that prevent our deploy workflow from creating tags?

It would, if they match the rule. This is one of the reasons I've gone for more granularity elsewhere..

[webknjaz]

I don't have repo settings access but wanted to share that rulesets are accessible publicly, which is better than the legacy protections that weren't transparent: https://github.com/pytest-dev/pytest/rules.

This also means that I can link to what I've set up elsewhere and you all can see what I've been playing with:
https://github.com/ansible/awx_plugins.interfaces/rules.

Oh, and one of the cool new features is that you can actually export each ruleset as a JSON file and seed the new repos with your battle-tested rules. It's one file per rule, though so some repetition is still needed. You can import them and safe as disabled for further refinement too. Or share those files with others, adding them to multiple repos.
I've tested this and it works but I haven't yet checked if there's an API for automating such provisioning.

[webknjaz]

Bear in mind that those rule names are publicly visible (on the web UI, but probably not in the git push output). So I'd recommend using actionable phrases that would instruct the readers how to avoid the restrictions.

[webknjaz]

@RonnyPfannschmidt check if we have that last section in the org:

If not, it might be possible to combine inclusions and exclusions.

Also, it is very important to exclude the bot-managed branches from the rule. Otherwise, apps like patchback, pre-commit.ci and dependabot won't be able to create PRs since they operate within the upstream repo. We also need to handle the merge queue branches similarly in case we enable that in the future.

[webknjaz]

But there was also a way separate section in the bottom parts of the rules that literally suports regexes and can be used to refine allowed naming further.

I checked this in the pytest-forked repo where I have access to the repo settings, and it has an Enterprise label next to the Restrictions section, so I suppose this is a no-go.

[webknjaz]

@RonnyPfannschmidt I crossed a few TODO list items in the initial post per what you've completed already.

I noticed that there's no GH App-provided checks in the rules anymore. Should we keep RTD/pre-commit.ci there?

Also, in the top section called Bypass list, could you add Dependabot, Patchback and pre-commit.ci? This will make sure the rule isn't breaking these automations.

I'd still enumerate the known branch exclusions just in case. Here's mine:

• maintenance/pip-tools-constraint-lockfiles
• maintenance/pip-tools-constraint-lockfiles-*
• patchback/backports/**
• dependabot/**
• release/v*
• pre-commit-ci-update-config
• gh-readonly-queue/**/*
  Of course, some are not applicable here, but perhaps you could exclude those that are?

[webknjaz]

@RonnyPfannschmidt could you look into what's left once more?

[RonnyPfannschmidt]

@webknjaz i added the suggested bypasses and the enumeration

i wonder if we should make that ruleset an export and apply it to all repos where sensible

[webknjaz]

Yes, that would make sense. I was thinking that the export could be made public in some shared files so people could pick it up on their own. I've been experimenting with this more and the import experience is mostly fine but there's a quirk that it pre-fills the form and doesn't let you save it if it references GitHub Apps or checks that aren't present in the target repo. One has to then figure out which parts to remove to be able to actually save new ruleset.

[webknjaz]

@RonnyPfannschmidt could you post a screenshot of the added checks like chronographer/rtd/codecov? I don't see these marked in PRs...

[RonnyPfannschmidt]

I see. I added the bots as bypass not as pr check requirements

I'll fix that

[webknjaz]

Great, thanks! Those are just examples as I don't know what else might've been required in the old branch protection...

[RonnyPfannschmidt]

i added the checks for codecov, chronographer and rtd

we may need to remove codecov again as it seems slightly problematic

[webknjaz]

@RonnyPfannschmidt Codecov is pretty stable these days. We currently only have the patch variant of it in. It probably needs more configuration, but that's a separate topic.

P.S. I'd also add the pre-commit.ci - pr one as required.

[webknjaz]

@RonnyPfannschmidt would you mind marking pre-commit.ci - pr one as required? I think this is the last bit in this issue. Once done (or decided against), the issue can be closed.