Revert removed-block approach for archived-repo sub-resources - #170
Merged
Merged
Conversation
removed blocks require the resource's configuration to be deleted from source entirely -- they can't coexist with a resource block that's still declared (even when gated to count=0/empty for_each for the specific instance in question), because the block is shared across every module instance, archived or not. Terraform validates this statically and refuses to plan at all: Error: Removed resource block still exists This statement declares a removal of the resource module.repo_fulfillment_service.github_branch_protection.repo_protection, but this resource block still exists in the configuration. This broke the very first real apply after osac-project#165 merged (run 31053762343), blocking the whole pipeline again -- the exact failure mode this effort exists to fix, caused by the fix itself. Delete removed_archived_repos.tf. No other change is needed: modules/common_repository/main.tf's count/for_each conditions (count = ... : var.archived ? 0 : ..., for_each = var.archived ? {} : {...}) were never modified by the removed-block commit -- they already correctly guard branch_protection and environment for archived repos. That was always the right fix for *not creating new* instances; the removed blocks were only ever needed to cleanly detach the *existing* tracked instances for the 7 already-archived repos without an unverified destroy attempt. That part still needs doing, just via `tofu state rm` instead (see PR description for the exact addresses). Signed-off-by: Elior Erez <eerez@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: osac-project/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
WalkthroughThe pull request deletes Changes
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 11✅ Passed checks (11 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
The
removedblock approach from #165 broke the very first real apply after it merged (run 31053762343), blocking the whole pipeline again -- the exact failure mode this whole effort exists to fix, now caused by the fix itself.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 -- even when that specific module instance is gated tocount = 0/ emptyfor_each-- becausemodules/common_repository/main.tf'sgithub_branch_protection/github_repository_environmentblocks are shared across every repo module, archived or not. Terraform validates this statically, before ever touching real state:(12 instances of this error, one per archived-repo module × resource type that had a
removedblock.)What
Delete
removed_archived_repos.tf. Nothing else changes:modules/common_repository/main.tf'scount/for_eachconditions (count = var.visibility == "private" ? 0 : var.archived ? 0 : ...,for_each = var.archived ? {} : {...}) were never touched by theremoved-block commit -- they already correctly stop creating new branch_protection/environment instances for archived repos. That part of #165 was always correct. What's still needed is cleanly detaching the existing, already-tracked instances for the 7 already-archived repos, without an unverified destroy attempt against their archived-repo API -- viatofu state rmonce #162'sstate_rm_addressesmechanism is available, not viaremovedblocks.State-removal addresses needed before this PR's own apply succeeds cleanly
I don't have direct Terraform backend credentials, so I could not run
tofu state list/tofu showagainst the real state directly. Instead I cross-checked GitHub's live API for each of the 7 archived repos, for the two resource types at issue (a live object existing is a reliable proxy for "Terraform state still tracks this," since these resource types map 1:1 to a real GitHub object with no complex diffing):github_branch_protection.repo_protection[0]-- confirmed live (viagh api repos/OWNER/REPO/branches/main/protection) on all 7:github_repository_environment.env["e2e-test"]-- confirmed live (viagh api repos/OWNER/REPO/environments) on 6 of 7 (all excepthost-management-openstack, which has none):One extra finding, unrelated to the archived-repo guard specifically:
bare-metal-fulfillment-operator's current module block inrepositories.tfhas noenvironmentsfield at all (var.environmentsdefaults empty), yet it has a livee2e-testenvironment right now. That's a pre-existing orphaned state entry from before theenvironmentsline was apparently removed from that module's config -- it would have been blocking applies via this same archived-repo 409 mechanism regardless of theremoved-block saga. Included above since it needs the samestate rmtreatment; flagging it explicitly since it wasn't part of what I set out to check.Caveat: this list is derived from live GitHub API cross-checks, not a direct read of the Terraform state file (no backend credentials available to me). Please confirm with a real
tofu plan(which will show these as pending destroys once this PR's revert is applied) before running the actualtofu state rm-- that's the authoritative source, this is my best derivation without it.(
fulfillment-servicealso shows agithub-pagesenvironment live -- that's GitHub's own auto-created Pages environment, not somethinggithub_repository_environment.envmanages, so it's excluded from the list above.)Test plan
tofu init -backend=false && tofu validate-- passes (same pre-existing deprecation warnings as always)tofu planthat the 13 addresses above (and only those) show as pending destroytofu state rmfor all 13 via NO-ISSUE: Add one-time TF state rm support for unrefreshable resources #162'sstate_rm_addressesmechanism once mergedSummary by CodeRabbit