fix(OSAC-3445): guard sub-resources for archived repos in common_repository module - #165
fix(OSAC-3445): guard sub-resources for archived repos in common_repository module#165amej wants to merge 1 commit into
Conversation
…sitory module GitHub rejects writes (409) to pages, labels, branch protection, rulesets, and environments on archived repos. archived=true was already threaded through repositories.tf but the module never skipped these sub-resources, which broke tofu apply once any repo was archived. Assisted-by: Claude Code (claude-sonnet-5) Signed-off-by: Ameya Sathe <asathe@redhat.com> rh-pre-commit.version: 2.4.0 rh-pre-commit.check-secrets: ENABLED
WalkthroughArchived repositories now skip GitHub Pages, issue labels, branch protection, status-check rulesets, and environments. Non-archived repository behavior remains unchanged. ChangesArchived repository handling
Estimated code review effort: 2 (Simple) | ~10 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 |
![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`:
- Around line 55-56: Update modules/common_repository/main.tf at lines 55-56,
70-72, 108-108, and 155-156 so archived repositories do not change resource
instance presence via empty for_each or count = 0. Detach these resources from
Terraform or manage them outside Terraform while preserving existing labels,
environments, branch protection, and rulesets; do not rename resources or create
a destroyed/kept state.
🪄 Autofix
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: CHILL
Plan: Pro Plus
Run ID: db5e4e54-869d-47c9-861e-9e110d43385a
📒 Files selected for processing (1)
modules/common_repository/main.tf
| # Skip labels for archived repos (GitHub rejects writes with 409) | ||
| for_each = var.archived ? {} : { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file stats =="
wc -l modules/common_repository/main.tf
echo "== relevant sections =="
sed -n '1,220p' modules/common_repository/main.tf | cat -n
echo "== lifecycle/search relevant resources =="
rg -n "lifecycle|detach|destroy|for_each|count = var|github_repository|github_branch|github_ruleset|github_repository_environment|github" modules/common_repository/main.tf modules -g '*.tf'Repository: osac-project/github-config
Length of output: 13536
🌐 Web query:
Terraform empty for_each removes instances destroys state resource multiple labels dynamic for_each empty branch protection count zero destructive
💡 Result:
In Terraform, both count and for_each serve as meta-arguments to manage multiple resource instances, and both handle empty inputs by removing all previously managed instances of that resource [1][2]. When you provide an empty map or set to for_each, or a count of zero, Terraform recognizes that no instances should exist [1][2]. If those instances were previously managed in the state, Terraform will plan to destroy them [3][4]. Key technical behaviors: - For_each: Terraform tracks resources by their map key or set member [5]. When a key is removed from the input collection, Terraform marks the associated resource instance for destruction [4][6]. If the input becomes an empty map {}, all associated instances in the state are destroyed [1][4]. - Count: Terraform tracks resources by their integer index [4]. Setting count = 0 instructs Terraform to destroy all instances of that resource [2][7]. - Dynamic Blocks: When using dynamic blocks, an empty collection in the for_each argument results in the block not being rendered at all [1][8]. This is a common pattern for conditionally creating or omitting configuration blocks [8]. - Destructive Actions: Both methods perform standard destruction of infrastructure objects in the state [1][2]. Neither inherently ignores existing state unless specifically handled; they proactively reconcile the state to match the configuration (which has zero instances) [3]. - Prevent Destroy: The lifecycle argument prevent_destroy can be used to block the destruction of resources, even if you remove them from your configuration (e.g., by setting for_each to an empty map) [9]. If Terraform attempts to destroy a resource with this flag, it will return an error and halt the plan/apply process [9]. If you are experiencing unexpected destruction, verify that your for_each or count expression is resolving to an empty value (e.g., {} or 0) as intended, and check for any lifecycle rules or external dependencies that might be affected [7][8].
Citations:
- 1: https://developer.hashicorp.com/terraform/language/meta-arguments/for%5Feach
- 2: https://developer.hashicorp.com/terraform/language/meta-arguments/count
- 3: https://github.com/hashicorp/terraform/blob/main/docs/planning-behaviors.md
- 4: https://support.hashicorp.com/hc/en-us/articles/31348158569363-Terraform-count-versus-for-each-meta-argument
- 5: https://developer.hashicorp.com/terraform/language/meta-arguments/for_each
- 6: https://support.hashicorp.com/hc/en-us/articles/27320180205843-Unexpected-re-creation-of-resources-when-modifying-variable-with-for-each
- 7: https://discuss.hashicorp.com/t/handling-linear-dependencies-when-count-of-resource-is-zero/54921
- 8: https://discuss.hashicorp.com/t/skipping-resource-creation-when-resource-consist-for-each-in-code-block/18534
- 9: https://developer.hashicorp.com/terraform/language/block/resource
Do not remove archived resource instances from configuration.
The archive guards set github_issue_label.repo_labels and github_repository_environment.env to an empty for_each, and github_branch_protection.repo_protection and github_repository_ruleset.status_checks to count = 0. Terraform treats these as destroyed instances, so applying can issue writes to delete labels, branch protection, rulesets, or environments on archived repositories instead of preserving them. Detach these resources or manage them outside Terraform for archived repos; do not create a separate destroyed/kept state with a name change.
📍 Affects 1 file
modules/common_repository/main.tf#L55-L56(this comment)modules/common_repository/main.tf#L70-L72modules/common_repository/main.tf#L108-L108modules/common_repository/main.tf#L155-L156
🤖 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` around lines 55 - 56, Update
modules/common_repository/main.tf at lines 55-56, 70-72, 108-108, and 155-156 so
archived repositories do not change resource instance presence via empty
for_each or count = 0. Detach these resources from Terraform or manage them
outside Terraform while preserving existing labels, environments, branch
protection, and rulesets; do not rename resources or create a destroyed/kept
state.
There was a problem hiding this comment.
@eliorerz : I am unsure about the right approach here.
Quoting chai-bot here;
PR #165 Review — Looks Good ✅
PR #165 implements all 5 changes correctly in modules/common_repository/main.tf:
- ✅ pages dynamic block — guarded with var.archived ? [] : var.pages[*]
- ✅ github_issue_label — guarded with var.archived ? {} : { ... }
- ✅ github_branch_protection — guarded with var.archived ? 0 in the count chain
- ✅ github_repository_ruleset — same pattern in the count chain
- ✅ github_repository_environment — guarded with var.archived ? {} : { ... }
Only modules/common_repository/main.tf was modified (10 additions, 6 deletions). CI is green — pre-commit passed.
About CodeRabbit's "Changes Requested"
CodeRabbit flagged a theoretical concern: setting count = 0 / for_each = {} causes Terraform to plan destruction of existing sub-resources (labels, branch protection, rulesets, environments) on archived repos, potentially issuing DELETE API calls to GitHub.
This concern is technically valid but practically a non-issue for two reasons:
-
The sub-resources are meaningless on archived repos. Labels, branch protection, rulesets, and environments serve no purpose on a read-only archived repository. Removing them from Terraform state is the correct behavior — you don't want Terraform managing resources that GitHub won't let it update.
-
GitHub typically allows DELETEs on archived repos. The 409 errors you're seeing are on PUT/POST (create/update) operations. DELETE operations generally succeed on archived repos. In the unlikely case a delete fails, the worst outcome is a harmless error on the first apply, resolvable with a one-time tofu state rm for the affected resource.
CodeRabbit's suggested alternative — "detach these resources from Terraform or manage them outside Terraform" — would require running tofu state rm for every sub-resource of every archived repo before applying. That's a complex manual operation that's more error-prone than the clean approach in your PR.
Recommendation: The PR is ready to merge. You can safely dismiss CodeRabbit's comment. If the first apply after merge encounters any delete failures on already-archived repos (unlikely), run:tofu state rm 'module.repo_host_management_openstack.github_branch_protection.repo_protection[0]'
for any such resource to clean up the orphaned state entry.
There was a problem hiding this comment.
🧩 Analysis chain
🤖 web_search executed:
GitHub REST API archived repository delete issue label branch protection repository ruleset environment collaborator 409 documentation
Length of output: 11125
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Terraform/OpenTofu provider lock or required provider definitions =="
rg -n -C 3 'required_providers|integrations/github|github.*=' -g '*.tf' -g '*.lock.hcl' .
echo "== Archived-resource declarations and all unguarded GitHub resources =="
rg -n -C 5 '^resource "github_|var\.archived|for_each =|count =' modules/common_repository/main.tf
echo "== Provider source references, if vendored or pinned locally =="
fd -HI '.*github.*' .terraform . 2>/dev/null | head -200 || trueLength of output: 7871
🤖 web_search executed:
site:github.com/integrations/terraform-provider-github v6.12.1 resourceGithubIssueLabel Delete github_branch_protection github_repository_ruleset github_repository_environment
Length of output: 14015
🤖 web_search executed:
site:docs.github.com archived repositories read-only REST API delete repository labels branch protection rulesets environments collaborators
Length of output: 14237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="integrations/terraform-provider-github"
ref="v6.12.1"
for file in \
github/resource_github_issue_label.go \
github/resource_github_branch_protection.go \
github/resource_github_repository_ruleset.go \
github/resource_github_repository_environment.go \
github/resource_github_repository_collaborators.go
do
echo "== ${file} =="
gh api "repos/${repo}/contents/${file}?ref=${ref}" \
--jq '.content' | base64 --decode \
| rg -n -C 8 'func .*Delete|Delete\(|archived|Archived|Remove|destroy' \
|| true
doneLength of output: 7835
🏁 Script executed:
#!/bin/bash
set -euo pipefail
repo="integrations/terraform-provider-github"
ref="v6.12.1"
for file in \
github/resource_github_branch_protection.go \
github/resource_github_repository_environment.go \
github/resource_github_issue_label.go \
github/resource_github_repository_ruleset.go \
github/utils.go \
github/provider.go
do
echo "== ${file} =="
gh api "repos/${repo}/contents/${file}?ref=${ref}" \
--jq '.content' | base64 --decode \
| rg -n -C 18 'func resourceGithubBranchProtectionDelete|func resourceGithubRepositoryEnvironmentDelete|func handleArchivedRepoDelete' \
|| true
doneLength of output: 3827
@amej, I would not dismiss the comment as written. I would narrow it.
The quoted review is correct for github_issue_label. Provider version 6.12.1 explicitly skips label deletion for archived repositories and removes the label from state. The provider documentation states this behavior.
The quoted review is not correct for all guarded resources:
github_branch_protectionruns the GraphQLdeleteBranchProtectionRulemutation directly. It has no archived-repository error handling.github_repository_environmentrunsDeleteEnvironmentdirectly. It only handles 404 and 304 responses. It has no archived-repository error handling.- Setting
count = 0orfor_each = {}schedules these delete writes for already-managed instances.
Therefore, the current change can still fail when it archives a repository that has managed branch protection or environments. It can also delete those settings if GitHub accepts the requests.
github_repository_ruleset uses the provider's archive-delete handler. It is closer to the label case. However, it still removes the resource from state. That is acceptable only if this PR intentionally stops managing rulesets after archival.
The main decision is lifecycle policy:
- If archival must preserve settings for possible unarchival, do not remove the instances from configuration or state as a side effect of
var.archived. - If archival must intentionally discard managed settings, document that behavior and verify a plan for each archived repository before apply.
- Do not rely on undocumented DELETE behavior for branch protection and environments.
I recommend retaining the finding for github_branch_protection and github_repository_environment, and withdrawing the label portion. The current PR should not merge solely on the assumption that DELETE requests are harmless.
You are interacting with an AI system.
1 participant
Summary
var.archivedguards tomodules/common_repository/main.tfforpages,github_issue_label,github_branch_protection,github_repository_ruleset.status_checks, andgithub_repository_environment— GitHub returns 409 on writes to these sub-resources for archived repos.github_repository.repoalready setarchived = var.archived; the module never skipped the dependent sub-resources, which is the root cause of thetofu applyGitHub Action failures below.Related PRs whose
tofu applyfailed because of this gaparchived=truefor repos merged into osac mono-repo (original introduction of the gap)archived=truewithout the module fixhost-management-openstack— hit the same 409 failures(PR #159, the interim revert of #158, is not affected since it undid the archival rather than triggering the bug.)
What gets unblocked
Once merged, the pending
tofu applydrift for the repos archived in #158/#161/#163 (fulfillment-service,osac-operator,osac-aap,osac-installer,bare-metal-fulfillment-operator,osac-csi-driver,host-management-openstack) will apply cleanly instead of failing with 409 errors on branch protection, rulesets, labels, pages, and environments.Test plan
tofu init -backend=false && tofu validate— passes (only pre-existing deprecation warnings)--agent, committed vsupstream/main) — 0 findingstofu applysucceeds in CI for the previously-failing archived reposAssisted-by: Claude Code (claude-sonnet-5)
Summary by CodeRabbit