Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 SummaryAdds Zuul-only branch protection for WalkthroughTerraform adds the ChangesZuul-only branch protection
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: 🟡 Moderate · up to Applying this change can temporarily remove the release-branch protections while Terraform recreates them. Preserve the existing resource state with moves before merging. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
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 |
stackhpc/neutron is a first example of a Zuul only CI repo, stackhpc/2025.1 and stackhpc/2026.1 only run Zuul jobs and we can use ``gate`` label to gate the patches for Zuul to merge them. Disable the permission for regular human users to merge patches in these repositories.
8b80997 to
0c7d6eb
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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Advanced
Run ID: e6db8569-db5c-43a0-8bd1-5e40d4ec1075
📒 Files selected for processing (3)
terraform/github/branches.tfterraform/github/terraform.tfvars.jsonterraform/github/variables.tf
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
|
|
||
| resource "github_branch_protection" "openstack_branch_protection_epoxy" { | ||
| for_each = toset(var.repositories["OpenStack"]) | ||
| for_each = toset([for r in var.repositories["OpenStack"] : r if !contains(var.repositories["ZuulOnly"], r)]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Add state moves for the two neutron branch protections.
neutron moves between distinct Terraform resource addresses, and no migration declaration exists. Terraform will therefore plan destruction of the old instances and creation of the new instances. The old resources allow destruction, so the new resources’ prevent_destroy setting does not block this transition.
Proposed state moves
moved {
from = github_branch_protection.openstack_branch_protection_epoxy["neutron"]
to = github_branch_protection.zuulonly_branch_protection_epoxy["neutron"]
}
moved {
from = github_branch_protection.openstack_branch_protection_gazpacho["neutron"]
to = github_branch_protection.zuulonly_branch_protection_gazpacho["neutron"]
}The plan should update the existing protections without destroy actions.
🧰 Tools
🪛 Checkov (3.3.13)
[low] 525-562: Ensure GitHub branch protection rules requires signed commits
(CKV_GIT_6)
[medium] 525-562: GitHub pull requests should require at least 2 approvals
(CKV_GIT_5)
There was a problem hiding this comment.
Bot has a point. We've had issues removing branch protection in the past. You'll either want to flex your admin powers and manually tweak some things, or find some way to do it in TF config
There was a problem hiding this comment.
You could also use setsubtract to make it a bit more readable. I think this would do the same thing but is more clear imo
| for_each = toset([for r in var.repositories["OpenStack"] : r if !contains(var.repositories["ZuulOnly"], r)]) | |
| for_each = setsubtract(toset(var.repositories["OpenStack"]), toset(var.repositories["ZuulOnly"])) |
There was a problem hiding this comment.
@Alex-Welsh, correct. setsubtract produces the same repository set and makes the exclusion operation clearer.
for_each = setsubtract(
toset(var.repositories["OpenStack"]),
toset(var.repositories["ZuulOnly"]),
)The expression also preserves the set type required by for_each.
You are interacting with an AI system.
There was a problem hiding this comment.
Use this command on a human-authored review finding. CodeRabbit findings already use the standard resolution workflow.
Terraform Format and Style 🖌
|
| # merged by Zuul (typically after the "gate" label enqueues the change into its gate | ||
| # pipeline). Neither the Developers team nor the owning team can merge directly. | ||
| # Only the stackhpc/2025.1 and stackhpc/2026.1 branches are gated this way for now; | ||
| # neutron's other branches keep normal OpenStack-group protection (see openstack_branch_protection_*). |
There was a problem hiding this comment.
nit: I'd remove the reference to neutron in this comment. It's a generic branch protection rule that we can (should) apply to more repos in the future.
|
|
||
| resource "github_branch_protection" "openstack_branch_protection_epoxy" { | ||
| for_each = toset(var.repositories["OpenStack"]) | ||
| for_each = toset([for r in var.repositories["OpenStack"] : r if !contains(var.repositories["ZuulOnly"], r)]) |
There was a problem hiding this comment.
Bot has a point. We've had issues removing branch protection in the past. You'll either want to flex your admin powers and manually tweak some things, or find some way to do it in TF config
|
|
||
| resource "github_branch_protection" "openstack_branch_protection_epoxy" { | ||
| for_each = toset(var.repositories["OpenStack"]) | ||
| for_each = toset([for r in var.repositories["OpenStack"] : r if !contains(var.repositories["ZuulOnly"], r)]) |
There was a problem hiding this comment.
You could also use setsubtract to make it a bit more readable. I think this would do the same thing but is more clear imo
| for_each = toset([for r in var.repositories["OpenStack"] : r if !contains(var.repositories["ZuulOnly"], r)]) | |
| for_each = setsubtract(toset(var.repositories["OpenStack"]), toset(var.repositories["ZuulOnly"])) |
3 participants
stackhpc/neutron is a first example of a Zuul only CI repo, stackhpc/2025.1 and stackhpc/2026.1 only run Zuul jobs and we can use
gatelabel to gate the patches for Zuul to merge them.Disable the permission for regular human users to merge patches in these repositories.