fix: route CmdDelete through the engine and unstick finalizer teardown - #103
Open
ntlaletsi70 wants to merge 1 commit into
Open
fix: route CmdDelete through the engine and unstick finalizer teardown#103ntlaletsi70 wants to merge 1 commit into
ntlaletsi70 wants to merge 1 commit into
Conversation
GitHubEvent and Deployment reconcilers computed the correct cmdType (Update vs Delete based on finalizer/deletionTimestamp) but then hardcoded Type: command.CmdUpdate when building the engine command. CmdDelete was never reached, so CleanupPrerequisites never ran on deletion — the finalizer only cleared if create-ish logic happened to succeed against a dying object, and stuck forever otherwise. Also fixes a related class of stuck finalizer: Build, Deployment, and GitRepository's CleanupPrerequisites required an Environment lookup that fails permanently once the Environment is deleted, wedging the finalizer if the Environment goes before its children. Added EnvironmentGone as a pre-check mediators can use to skip store-bound cleanup once the Environment is confirmed gone. GitHubEvent's teardown drops the Environment dependency entirely instead: GitHubEvent CRs are written by the Argo Events Sensor into the fixed argo-events namespace, never the Environment's own namespace, so the lookup was structurally unsatisfiable in production regardless of deletion order. GitHubWebhookSecretReconciler.Delete only needs the secret's name/namespace, not a store binding. Also fixes a nil-pointer panic in the build observer: applyTriggers dereferenced resolved.Spec.Policy.Triggers without checking Policy for nil, crashing on every Build CR with no policy block. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
1 participant
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.
Summary
cmdType(Update vs Delete) correctly but then hardcodedType: command.CmdUpdatewhen building the engine command —CmdDelete(and thereforeCleanupPrerequisites) was never reached on deletion, so the finalizer either leaked resources (if create-ish logic happened to succeed against a dying object) or got stuck forever (if it didn't). Fixed both to use the computedcmdType.EnvironmentGoneas a pre-check so teardown skips store-bound cleanup once the Environment is confirmed gone, instead of erroring forever.argo-eventsnamespace, never the Environment's own namespace, so the lookup was structurally unsatisfiable in production.GitHubWebhookSecretReconciler.Deleteonly needs the secret's name/namespace, not a store binding.applyTriggersdereferencedresolved.Spec.Policy.Triggerswithout a nil check onPolicy), crashing on every Build CR with nopolicyblock.All four fixes were reproduced and verified live against a kind cluster before and after the change, in addition to unit test coverage.
Test plan
go build ./...,go vet ./...,go test ./...all passCmdDeletenow reaches the engine for both GitHubEvent and Deployment (previously always routed asCmdUpdate)policyblockEnvironmentGone(mediators),CleanupPrerequisites_NoEnvironment(githubevent mediator), updatedTestBuildDomain_Handle_Delete_MissingEnvironmentto assert the corrected behavior🤖 Generated with Claude Code