fix: Phase 4 follow-ups — destroy ordering/protection from state, backup fidelity, state migration, shared fetchActual (#17) - #40
Merged
Conversation
…erarchy (#17 item 9) - Extract resolveWithEnv(explicit, env, fallback) for the shared explicit||env||default idiom (config/state/backup-dir path resolvers). - context.ts: parentKey narrows to `parent || undefined` after the type guard. - hierarchy.ts applyHierarchy: one pass over opted-in desired groups instead of a derived optedIn Set + separate state loop.
…17 item 6) buildPlan folds `parents`/`dynamic` into its actual map in place and apply reuses that map for the backup, so backups carried non-restorable logical-key sets. writeBackup now drops synthetic fields (isSyntheticField) without mutating the shared map, matching destroy's clean backup.
…item 7) Both buildPlan and destroy's pre-delete backup fetched managed actuals with divergent semantics. Extract fetchActual(client, resources): concurrent, 404 → omit, non-404 → record (never throw), warn on missing spec. buildPlan is now a thin wrapper; the fetch-failed threading from #38 is preserved exactly. destroy adopts it next so backup and plan read actuals identically.
items 2,4) - ManagedResource gains `preventDestroy?`: state, not the ephemeral config, is the source of truth for destroy protection (apply mirrors it — next commit). - migrateState renames the vestigial campus `shortName` key to `shorty` on load (only when `shorty` is absent), clearing the permanent phantom drift left by the Phase-4 rename that shipped with no state-version bump.
…tem 2) computePlan carries the desired preventDestroy on each item; executePlan writes it to state on create/update, and reconciles it even on a note-less no-op (a flag toggle is never a diffed field, so it would otherwise never reach state). Config→state stays the source of truth: dropping the flag clears it on re-apply. (apply.ts also adopts resolveWithEnv for the backup-dir resolver.)
#17 items 1,2,3) - orderDestroy now reuses orderKeys (reversed) with parent edges discovered live from /groups/hierarchies (state carries none), so a child group is deleted before its parent — no more parent-first 4xx. Fetch failure degrades to tier-only order rather than aborting. - preventDestroy is read from STATE, so a target dropped from config stays protected. destroy loads no config file at all, so a config eval error (a sibling still referencing the dropped target) can no longer block a teardown. - Backup reuses the shared fetchActual (item 7). Drops the dead --config option.
…ils non-404 The fetchActual extraction made destroy's backup best-effort: a transient 500 on the backup GET warned and proceeded to delete the target with no backup of its pre-delete state — the exact loss the pre-destroy backup exists to prevent. Restore the old abort semantics (404 still means already-gone and is skipped). Flagged by the PR #40 review.
Member
Author
|
Review verdict was APPROVE with one design decision to confirm: the fetchActual extraction had made destroy's backup best-effort (non-404 fetch failure → warn + proceed to DELETE unbacked). Confirmed as NOT acceptable on the destructive path — restored abort-before-any-DELETE semantics in ab08c95 with a command-level test (500 on a target's backup GET → exit 1, zero DELETEs, state intact). 404 still means already-gone and is skipped. |
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.
Implements the still-open items of #17 (items 5 and 8 were verified already resolved by the #28 fix and the Phase-5 synthetic-field registry respectively).
Destroy ordering + protection (items 1–3)
/groups/hierarchiesfetch at destroy time feeds the sameorderKeystopological sort, reversed — children delete before parents; degrades to tier-only order with a warning if the fetch fails.preventDestroyis persisted into the state snapshot at apply time (created/updated and reconciled on no-ops), and destroy reads protection from state — deleting a resource from config no longer drops its protection, and destroy no longer loads the config at all (a config eval error can't block teardown). Missing flag on old state = false.State migration (item 4)
shortName→shortyshipped in Phase 4 with no state-version bump, so pre-rename state carries phantom drift forever.migrateStateon load renames the campus field only whenshortyis absent.Backup fidelity (item 6)
Backups no longer carry the synthetic
parents/dynamicpseudo-fields (stripped viaisSyntheticField, without mutating the shared actual map).Shared fetchActual (item 7)
buildPlan's fetch-actual loop extracted tofetchActual(concurrent, 404-tolerant), reused by destroy's backup pass — preserving #38's fetch-failed threading exactly. Destroy's backup is now consistently best-effort: a non-404 fetch error records + skips instead of aborting the run.Cheap cleanups (item 9)
resolveWithEnvfactored out;parentKeynarrowing simplified;applyHierarchysingle-pass.Verification: 262 passed / 4 skipped, typecheck + lint clean.
Closes #17.