NO-ISSUE: migrate required status checks to GitHub rulesets#131
Conversation
ae4aff9 to
bb1fdce
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (1)
WalkthroughThe change removes legacy required status checks from branch protection, adds collaborator ordering, and introduces a conditional repository ruleset targeting the default branch with dynamically generated required checks. ChangesRepository status checks
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
should we add |
bb1fdce to
7bb6360
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/common_repository/main.tf`:
- Line 103: Add a GitHub Actions pull-request workflow for the Terraform
configuration containing github_repository_ruleset.status_checks that runs
terraform fmt -check, terraform validate, and terraform plan. Configure the job
to initialize Terraform before validation and planning, and ensure the plan
executes from the correct Terraform working directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: osac-project/coderabbit/.coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3c060cee-e51a-43f0-8338-a91323922289
📒 Files selected for processing (1)
modules/common_repository/main.tf
| depends_on = [github_repository.repo, github_repository_collaborators.repo_collaborators] | ||
| } | ||
|
|
||
| resource "github_repository_ruleset" "status_checks" { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a Terraform plan CI workflow.
Regarding your question about adding a tf plan CI job: Yes, it is strongly recommended. Running terraform fmt -check, terraform validate, and terraform plan on pull requests validates configuration syntax and previews infrastructure changes, preventing unexpected enforcement gaps or errors upon merge.
Would you like me to generate a GitHub Actions workflow snippet or open a new issue to implement this?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@modules/common_repository/main.tf` at line 103, Add a GitHub Actions
pull-request workflow for the Terraform configuration containing
github_repository_ruleset.status_checks that runs terraform fmt -check,
terraform validate, and terraform plan. Configure the job to initialize
Terraform before validation and planning, and ensure the plan executes from the
correct Terraform working directory.
There was a problem hiding this comment.
should be a separate PR
There was a problem hiding this comment.
@rccrdpccl, that makes sense—keeping infra changes and CI tooling changes separate is good practice. Would you like me to open a follow-up issue to track adding the tf plan/fmt/validate CI workflow?
There was a problem hiding this comment.
The reason there is no tf plan pre-commit job is because that requires credentials to access state storage, so it wouldn't work for pull requests from forked repositories.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
The reason there is no
tf planpre-commit job is because that requires credentials to access state storage, so it wouldn't work for pull requests from forked repositories.
There are ways to work around that: see environment protection, or pull_request_target, etc. if this is of interest I could craft a PR.
In any case, I do not think it should be done in this PR.
I don't see any actionable comments from coderabbit.
7bb6360 to
fd23193
Compare
…lesets Move required_status_checks out of github_branch_protection and into a github_repository_ruleset resource. Rulesets do not block PRs when a required check is skipped (e.g. via paths-ignore in the workflow), unlike classic branch protection which leaves skipped checks stuck as "Expected". This enables component repos to add paths-ignore filters to their CI workflows so that docs-only and metadata-only PRs (OWNERS, LICENSE, *.md) can merge without waiting for irrelevant test runs. The ruleset includes OrganizationAdmin bypass so admins can still merge in emergencies without waiting for checks. Affected repos: fulfillment-service, osac-operator, osac-aap, osac-installer, github-config. NOTE: On first apply, there will be a brief window where the old status checks are removed from branch protection and the new ruleset is created. Consider a two-phase apply (create ruleset first, then remove old checks) to avoid a gap in enforcement. Assisted-by: Claude Code <noreply@anthropic.com> Signed-off-by: Riccardo Piccoli <rpiccoli@redhat.com>
fd23193 to
98ca491
Compare
PR #131 migrated required status checks from branch protection to rulesets but only granted bypass to OrganizationAdmin. This broke manual merges for wg-infra members who previously could bypass via enforce_admins=false on branch protection. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
3 participants
Summary
Moves
required_status_checksfromgithub_branch_protectionto a newgithub_repository_rulesetresource. Rulesets don't block PRs when a required check is skipped (e.g. viapaths-ignore), unlike classic branch protection which leaves skipped checks stuck as "Expected."This enables component repos to add
paths-ignorefilters so docs-only PRs (OWNERS, LICENSE,*.md,docs/**) can merge without waiting for irrelevant CI runs.Changes
required_status_checksblock fromgithub_branch_protectiongithub_repository_ruleset.status_checks(only created whenrequired_status_checksis non-empty andbranch_protectionis enabled)Affected repos
ci/prow/unit,e2e-vmaas-full-install / e2eci/prow/temp,e2e-vmaas-full-install / e2eci/prow/temp,e2e-vmaas-full-install / e2ee2e-vmaas-full-install / e2epre-commitApply considerations
On first apply there will be a brief window where the old checks are removed from branch protection before the new ruleset is created. For zero-gap enforcement, consider a two-phase apply:
tofu apply -target='module.repo_fulfillment_service.github_repository_ruleset.status_checks[0]' -target='module.repo_cloudkit_operator.github_repository_ruleset.status_checks[0]' ...(create rulesets first)tofu apply(full apply removes old branch protection checks)Companion PRs
These PRs add
paths-ignoreto the CI workflows in each component repo:Remaining repos (osac-aap, osac-installer, osac-test-infra) will follow.
Assisted-by: Claude Code noreply@anthropic.com
Summary by CodeRabbit