Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
3 of 4 tasks
webknjaz opened this issue Jul 1, 2024 · 16 comments
Open
3 of 4 tasks
Assignees
Labels
type: infrastructure improvement to development/releases/CI structure type: selftests a problem in the tests of pytest

Comments

@webknjaz
Copy link
Member

webknjaz commented Jul 1, 2024

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 webknjaz added type: infrastructure improvement to development/releases/CI structure type: selftests a problem in the tests of pytest labels Jul 1, 2024
@webknjaz
Copy link
Member Author

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.

@The-Compiler The-Compiler removed their assignment Sep 16, 2024
@webknjaz webknjaz changed the title [TODO] Update branch protection to require the check job for main and 8.2.x branches [TODO] Update branch protection to require the check job for main and 8.3.x branches Oct 21, 2024
@webknjaz
Copy link
Member Author

@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
Copy link
Member

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

@RonnyPfannschmidt
Copy link
Member

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
Copy link
Member

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

@RonnyPfannschmidt
Copy link
Member

@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
Copy link
Contributor

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

@RonnyPfannschmidt
Copy link
Member

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

@nicoddemus
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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
Copy link
Member Author

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

Screenshot_2024-10-22-23-10-09-01_40deb401b9ffe8e1df2f1cc5ba480b12.jpg

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
Copy link
Member Author

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
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: infrastructure improvement to development/releases/CI structure type: selftests a problem in the tests of pytest
Projects
None yet
Development

No branches or pull requests

6 participants