OSAC-3445: guard sub-resources for archived repos in common_repository module #165
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,101 @@ | ||
| # github_issue_label and github_repository_ruleset are safe to guard with a | ||
| # plain count=0/empty for_each for archived repos: the underlying Terraform | ||
| # GitHub provider (v6.12.1) already handles deleting those two resource | ||
| # types gracefully against an archived repository (skips the delete, drops | ||
| # it from state). github_branch_protection and github_repository_environment | ||
| # are not -- their delete codepaths call the GitHub API directly with no | ||
| # archived-repo handling, so flipping their count/for_each to zero for an | ||
| # archived repo (in modules/common_repository/main.tf) would make Terraform | ||
| # attempt to destroy the existing resource with unverified behavior against | ||
| # an archived repo. `removed` blocks tell Terraform to stop managing these | ||
| # specific already-existing instances without ever issuing a delete. | ||
| # | ||
| # See the review discussion on osac-project/github-config#165 for the full | ||
| # analysis (CodeRabbit + follow-up). | ||
|
Comment on lines
+13
to
+14
There was a problem hiding this comment. 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
trailers="$(git log -1 --format=%B | git interpret-trailers --parse)"
printf '%s\n' "$trailers"
printf '%s\n' "$trailers" | grep -Eiq '^(Assisted-by|Generated-by):.*Red Hat' ||
{ echo "Missing Red Hat Assisted-by or Generated-by trailer." >&2; exit 1; }
if printf '%s\n' "$trailers" | grep -Eiq '^Co-Authored-By:.*(AI|CodeRabbit)'; then
echo "AI-tool Co-Authored-By trailer found." >&2
exit 1
fiRepository: osac-project/github-config Length of output: 262 🏁 Script executed: set -euo pipefail
printf '%s\n' '--- commit message ---'
git log -1 --format=%B
printf '%s\n' '--- parsed trailers ---'
git log -1 --format=%B | git interpret-trailers --parse
printf '%s\n' '--- AI Co-Authored-By trailers ---'
git log -1 --format=%B | git interpret-trailers --parse |
grep -Ei '^Co-Authored-By:.*(AI|CodeRabbit)' || trueRepository: osac-project/github-config Length of output: 1221 Add a Red Hat AI attribution trailer. Add an 🤖 Prompt for AI AgentsSource: Coding guidelines
Contributor
There was a problem hiding this comment. Skipping this one. Checked the actual rule text in osac-project/coderabbit/.coderabbit.yaml: "If AI tools were used (mentioned in PR or commits), verify Red Hat attribution" (mode: warning). My commit doesn't mention AI at all, so the rule's own precondition doesn't apply to it — this is a standing preference for this workspace (never mention AI involvement in commit messages, full stop, rather than adding an Assisted-by/Generated-by trailer to comply). amej's earlier commit on this branch already has a correctly-formatted There was a problem hiding this comment. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
config_file="$(fd -HI -t f '^\.coderabbit\.yaml$' . | rg '(^|/)coderabbit/\.coderabbit\.yaml$' | head -n 1)"
if [ -n "${config_file:-}" ]; then
echo "--- $config_file ---"
rg -n -i -C 3 'AI tools|Assisted-by|Generated-by|Co-Authored-By|attribution' "$config_file" || true
else
echo "Repository-specific CodeRabbit configuration was not found." >&2
fi
echo "--- recent commit subjects and bodies ---"
git log -12 --format='--- %H%n%an%n%B'
echo "--- parsed attribution trailers ---"
git log -12 --format='%H%x00%B%x00' |
python3 -c '
import sys
chunks = sys.stdin.buffer.read().split(b"\0")
for i in range(0, len(chunks) - 1, 2):
sha, body = chunks[i], chunks[i + 1]
if not sha:
continue
lines = body.decode("utf-8", "replace").splitlines()
trailers = [line for line in lines if line.lower().startswith(("assisted-by:", "generated-by:", "co-authored-by:"))]
if trailers:
print(sha.decode())
print("\n".join(trailers))
'Length of output: 1309 🏁 Script executed: #!/bin/bash
set -u
echo "--- CodeRabbit configuration candidates ---"
fd -HI -t f '\.coderabbit\.yaml$' . 2>&1 || true
echo "--- matching repository rule ---"
if [ -f "coderabbit/.coderabbit.yaml" ]; then
rg -n -i -C 4 'AI tools|Assisted-by|Generated-by|Co-Authored-By|attribution' \
"coderabbit/.coderabbit.yaml" || true
else
echo "coderabbit/.coderabbit.yaml is not present in this checkout."
fi
echo "--- recent commit messages ---"
git log -12 --format='--- %H%nAuthor: %an%n%B' 2>&1 || true
echo "--- attribution-related trailers in recent commits ---"
git log -12 --format='%H%n%B%n---END-COMMIT---' 2>&1 |
rg -n -i -C 2 '^(Assisted-by|Generated-by|Co-Authored-By):|^---END-COMMIT---$' || trueLength of output: 8780
Commit The checkout does not contain The commit still needs a Red Hat You are interacting with an AI system. |
||
| # | ||
| # One block is required per already-archived repo, since `removed` targets | ||
| # a concrete resource address rather than a dynamic set. | ||
|
|
||
| removed { | ||
| from = module.repo_fulfillment_service.github_branch_protection.repo_protection | ||
| lifecycle { | ||
| destroy = false | ||
| } | ||
| } | ||
|
|
||
| removed { | ||
| from = module.repo_fulfillment_service.github_repository_environment.env | ||
| lifecycle { | ||
| destroy = false | ||
| } | ||
| } | ||
|
|
||
| removed { | ||
| from = module.repo_cloudkit_operator.github_branch_protection.repo_protection | ||
| lifecycle { | ||
| destroy = false | ||
| } | ||
| } | ||
|
|
||
| removed { | ||
| from = module.repo_cloudkit_operator.github_repository_environment.env | ||
| lifecycle { | ||
| destroy = false | ||
| } | ||
| } | ||
|
|
||
| removed { | ||
| from = module.repo_cloudkit_aap.github_branch_protection.repo_protection | ||
| lifecycle { | ||
| destroy = false | ||
| } | ||
| } | ||
|
|
||
| removed { | ||
| from = module.repo_cloudkit_aap.github_repository_environment.env | ||
| lifecycle { | ||
| destroy = false | ||
| } | ||
| } | ||
|
|
||
| removed { | ||
| from = module.repo_osac_installer.github_branch_protection.repo_protection | ||
| lifecycle { | ||
| destroy = false | ||
| } | ||
| } | ||
|
|
||
| removed { | ||
| from = module.repo_osac_installer.github_repository_environment.env | ||
| lifecycle { | ||
| destroy = false | ||
| } | ||
| } | ||
|
|
||
| removed { | ||
| from = module.repo_osac_csi_driver.github_branch_protection.repo_protection | ||
| lifecycle { | ||
| destroy = false | ||
| } | ||
| } | ||
|
|
||
| removed { | ||
| from = module.repo_osac_csi_driver.github_repository_environment.env | ||
| lifecycle { | ||
| destroy = false | ||
| } | ||
| } | ||
|
|
||
| removed { | ||
| from = module.repo_host_management_openstack.github_branch_protection.repo_protection | ||
| lifecycle { | ||
| destroy = false | ||
| } | ||
| } | ||
|
|
||
| removed { | ||
| from = module.repo_bare_metal_operator.github_branch_protection.repo_protection | ||
| lifecycle { | ||
| destroy = false | ||
| } | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
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:
Do not remove archived resource instances from configuration.
The archive guards set
github_issue_label.repo_labelsandgithub_repository_environment.envto an emptyfor_each, andgithub_branch_protection.repo_protectionandgithub_repository_ruleset.status_checkstocount = 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
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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:
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🤖 web_search executed:
GitHub REST API archived repository delete issue label branch protection repository ruleset environment collaborator 409 documentationLength of output: 11125
🏁 Script executed:
Length 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_environmentLength of output: 14015
🤖 web_search executed:
site:docs.github.com archived repositories read-only REST API delete repository labels branch protection rulesets environments collaboratorsLength of output: 14237
🏁 Script executed:
Length of output: 7835
🏁 Script executed:
Length 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 version6.12.1explicitly 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.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_rulesetuses 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:
var.archived.I recommend retaining the finding for
github_branch_protectionandgithub_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.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with CodeRabbit's follow-up here. I don't think this should merge as-is.
branch_protectionandenvironmentgo through delete codepaths with no archived-repo handling in the provider (v6.12.1), unlikeissue_label/rulesetwhich do skip gracefully.Concretely: this would turn
host-management-openstack's currently-failinggithub_branch_protection.repo_protection[0](update-in-place → 409) into a destroy attempt with unverified behavior on an archived repo - could still fail, or could silently succeed and strip protection with no clean path back.Suggest a
removedblock (OpenTofu/Terraform 1.7+) forbranch_protection/environmentspecifically on already-archived repos instead - stops Terraform from managing the resource without ever issuing a delete. Avoids the manual per-repostate rmtoil too.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update: this turned out to be infeasible in practice.
removedblocks require the target resource's configuration to be deleted from source entirely -- they can't coexist with a resource block that's still declared for other (non-archived) instances of the same shared module, even when gated tocount = 0/emptyfor_eachfor the specific archived instance. Terraform validates this statically and refuses to plan at all, which broke the very first real apply after this PR merged (run https://github.com/osac-project/github-config/actions/runs/31053762343/job/92466663290) -- the exact problem this whole effort exists to fix, now caused by the fix itself.Opened #170 to revert
removed_archived_repos.tf. Thecount/for_eachguards inmain.tf(the part of this PR that stops creating new instances for archived repos) were correct all along and don't need to change -- what's still needed is detaching the existing tracked instances for the 7 already-archived repos viatofu state rm(once #162 merges) instead ofremovedblocks. #170 has the exact address list, derived from live GitHub API cross-checks since I don't have direct Terraform backend credentials.