From 5a103d7c1f843f6c612722462a0c8e182e422efa Mon Sep 17 00:00:00 2001 From: Chris Reed Date: Tue, 12 May 2026 09:16:12 -0500 Subject: [PATCH 1/3] feat(cli): Add prune confirmation. --- .../cmd/migrate/prune/namespaced_policy.go | 33 ++- .../prune_commit_confirmation.go | 106 +++++++++ .../prune_commit_confirmation_test.go | 224 ++++++++++++++++++ .../namespacedpolicy/prune_execute_test.go | 13 + .../migrations/namespacedpolicy/prune_plan.go | 2 + .../namespacedpolicy/prune_summary.go | 64 ++--- .../namespacedpolicy/prune_summary_test.go | 42 +++- 7 files changed, 437 insertions(+), 47 deletions(-) create mode 100644 otdfctl/migrations/namespacedpolicy/prune_commit_confirmation.go create mode 100644 otdfctl/migrations/namespacedpolicy/prune_commit_confirmation_test.go diff --git a/otdfctl/cmd/migrate/prune/namespaced_policy.go b/otdfctl/cmd/migrate/prune/namespaced_policy.go index 0ec88822af..7ba0110c84 100644 --- a/otdfctl/cmd/migrate/prune/namespaced_policy.go +++ b/otdfctl/cmd/migrate/prune/namespaced_policy.go @@ -54,7 +54,7 @@ func pruneNamespacedPolicy(cmd *cobra.Command, args []string) { if interactive { if err := namespacedpolicy.ReviewPrunePlan(cmd.Context(), plan, prompter); err != nil { if errors.Is(err, namespacedpolicy.ErrInteractiveReviewAborted) { - writeNamespacedPolicyPruneSummary(cmd, plan, false, "aborted") + writeNamespacedPolicyPruneSummary(cmd, plan, false, namespacedpolicy.PruneSummaryResultAborted) } cli.ExitWithError("could not review namespaced-policy prune plan", err) } @@ -64,19 +64,14 @@ func pruneNamespacedPolicy(cmd *cobra.Command, args []string) { executeNamespacedPolicyPruneCommit(cmd, h, plan, interactive, prompter) } - if _, err := cmd.OutOrStdout().Write([]byte(namespacedpolicy.RenderNamespacedPolicyPruneSummary(plan, commit) + "\n")); err != nil { + if _, err := cmd.OutOrStdout().Write([]byte(namespacedpolicy.RenderNamespacedPolicyPruneSummary(plan, commit, namespacedpolicy.PruneSummaryResultSuccess) + "\n")); err != nil { cli.ExitWithError("could not write namespaced-policy prune summary", err) } } func executeNamespacedPolicyPruneCommit(cmd *cobra.Command, h namespacedpolicy.ExecutorHandler, plan *namespacedpolicy.PrunePlan, interactive bool, prompter namespacedpolicy.InteractivePrompter) { if interactive { - if err := namespacedpolicy.ConfirmNamespacedPolicyPruneBackup(cmd.Context(), prompter); err != nil { - if errors.Is(err, namespacedpolicy.ErrNamespacedPolicyBackupNotConfirmed) { - writeNamespacedPolicyPruneSummary(cmd, plan, false, "aborted") - } - cli.ExitWithError("could not confirm namespaced-policy prune backup", err) - } + reviewNamespacedPolicyPruneInteractiveCommit(cmd, plan, prompter) } executor, err := namespacedpolicy.NewExecutor(h) @@ -85,13 +80,29 @@ func executeNamespacedPolicyPruneCommit(cmd *cobra.Command, h namespacedpolicy.E } if err := executor.ExecutePrune(cmd.Context(), plan); err != nil { - writeNamespacedPolicyPruneSummary(cmd, plan, true, "failure") + writeNamespacedPolicyPruneSummary(cmd, plan, true, namespacedpolicy.PruneSummaryResultFailure) cli.ExitWithError("could not execute namespaced-policy prune commit", err) } } -func writeNamespacedPolicyPruneSummary(cmd *cobra.Command, plan *namespacedpolicy.PrunePlan, commit bool, result string) { - if _, err := cmd.OutOrStdout().Write([]byte(namespacedpolicy.RenderNamespacedPolicyPruneSummaryWithResult(plan, commit, result) + "\n")); err != nil { +func reviewNamespacedPolicyPruneInteractiveCommit(cmd *cobra.Command, plan *namespacedpolicy.PrunePlan, prompter namespacedpolicy.InteractivePrompter) { + if err := namespacedpolicy.ConfirmNamespacedPolicyPruneBackup(cmd.Context(), prompter); err != nil { + if errors.Is(err, namespacedpolicy.ErrNamespacedPolicyBackupNotConfirmed) { + writeNamespacedPolicyPruneSummary(cmd, plan, false, namespacedpolicy.PruneSummaryResultAborted) + } + cli.ExitWithError("could not confirm namespaced-policy prune backup", err) + } + + if err := namespacedpolicy.ConfirmPrunePlanDeletes(cmd.Context(), plan, prompter); err != nil { + if errors.Is(err, namespacedpolicy.ErrInteractiveReviewAborted) { + writeNamespacedPolicyPruneSummary(cmd, plan, false, namespacedpolicy.PruneSummaryResultAborted) + } + cli.ExitWithError("could not review namespaced-policy prune commit", err) + } +} + +func writeNamespacedPolicyPruneSummary(cmd *cobra.Command, plan *namespacedpolicy.PrunePlan, executed bool, result namespacedpolicy.PruneSummaryResult) { + if _, err := cmd.OutOrStdout().Write([]byte(namespacedpolicy.RenderNamespacedPolicyPruneSummary(plan, executed, result) + "\n")); err != nil { cli.ExitWithError("could not write namespaced-policy prune summary", err) } } diff --git a/otdfctl/migrations/namespacedpolicy/prune_commit_confirmation.go b/otdfctl/migrations/namespacedpolicy/prune_commit_confirmation.go new file mode 100644 index 0000000000..e2a32e17df --- /dev/null +++ b/otdfctl/migrations/namespacedpolicy/prune_commit_confirmation.go @@ -0,0 +1,106 @@ +package namespacedpolicy + +import ( + "context" + "fmt" +) + +const ( + confirmPruneDeleteLabel = "Confirm delete" + confirmPruneDeleteDescription = "delete this source object" + abortPruneDeleteLabel = "Abort prune commit" + abortPruneDeleteDescription = "stop before deleting any objects" +) + +// ConfirmPrunePlanDeletes prompts for every delete-status prune item before +// commit execution and lets the user confirm, skip, or abort. +func ConfirmPrunePlanDeletes(ctx context.Context, plan *PrunePlan, prompter InteractivePrompter) error { + if plan == nil { + return nil + } + if prompter == nil { + prompter = &HuhPrompter{} + } + + if err := confirmDeletePruneItems(ctx, prompter, plan.Actions); err != nil { + return err + } + if err := confirmDeletePruneItems(ctx, prompter, plan.SubjectConditionSets); err != nil { + return err + } + if err := confirmDeletePruneItems(ctx, prompter, plan.SubjectMappings); err != nil { + return err + } + if err := confirmDeletePruneItems(ctx, prompter, plan.RegisteredResources); err != nil { + return err + } + if err := confirmDeletePruneItems(ctx, prompter, plan.ObligationTriggers); err != nil { + return err + } + + return nil +} + +func confirmDeletePruneItems[T pruneReviewItem]( + ctx context.Context, + prompter InteractivePrompter, + items []T, +) error { + for _, item := range items { + if !confirmablePruneDeleteItem(item) { + continue + } + prompt := pruneDeleteConfirmationPrompt(item) + if err := applyPruneDeleteConfirmationDecision(ctx, prompter, prompt, func() { markPruneItemSkipped(item) }); err != nil { + return err + } + } + + return nil +} + +func confirmablePruneDeleteItem[T pruneReviewItem](item T) bool { + return item.hasSource() && item.status() == PruneStatusDelete +} + +func markPruneItemSkipped[T pruneReviewItem](item T) { + item.setStatus(PruneStatusSkipped) + item.setReason(newPruneReason(PruneStatusReasonTypeSkippedByUser, skippedByUserReason)) +} + +func applyPruneDeleteConfirmationDecision(ctx context.Context, prompter InteractivePrompter, prompt SelectPrompt, markSkipped func()) error { + choice, err := prompter.Select(ctx, prompt) + if err != nil { + return err + } + + switch choice { + case namespacedPolicyCommitConfirm: + return nil + case namespacedPolicyCommitSkip: + markSkipped() + return nil + case namespacedPolicyCommitAbort: + return ErrInteractiveReviewAborted + default: + return fmt.Errorf("invalid prune commit selection %q", choice) + } +} + +func pruneDeleteConfirmationPrompt(item pruneReviewItem) SelectPrompt { + summary := item.reviewSummary() + + return SelectPrompt{ + Title: fmt.Sprintf("Delete %s %q?", summary.Kind, summary.Label), + Description: summary.Description, + Options: pruneDeleteConfirmationOptions(), + } +} + +func pruneDeleteConfirmationOptions() []PromptOption { + return []PromptOption{ + {Label: confirmPruneDeleteLabel, Value: namespacedPolicyCommitConfirm, Description: confirmPruneDeleteDescription}, + {Label: skipObjectLabel, Value: namespacedPolicyCommitSkip, Description: skipObjectDescription}, + {Label: abortPruneDeleteLabel, Value: namespacedPolicyCommitAbort, Description: abortPruneDeleteDescription}, + } +} diff --git a/otdfctl/migrations/namespacedpolicy/prune_commit_confirmation_test.go b/otdfctl/migrations/namespacedpolicy/prune_commit_confirmation_test.go new file mode 100644 index 0000000000..c1c17729cc --- /dev/null +++ b/otdfctl/migrations/namespacedpolicy/prune_commit_confirmation_test.go @@ -0,0 +1,224 @@ +package namespacedpolicy + +import ( + "errors" + "testing" + + "github.com/opentdf/platform/protocol/go/policy" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestConfirmPrunePlanDeletesConfirmsDeleteItems(t *testing.T) { + t.Parallel() + + plan := &PrunePlan{ + Actions: []*PruneActionPlan{ + { + Source: &policy.Action{Id: "action-1", Name: "archive"}, + Status: PruneStatusDelete, + MigratedTargets: []TargetRef{ + {ID: "target-action-1", NamespaceFQN: "https://example.com"}, + }, + }, + }, + } + prompter := &queuedSelectPrompter{ + selectValues: []string{namespacedPolicyCommitConfirm}, + } + + err := ConfirmPrunePlanDeletes(t.Context(), plan, prompter) + require.NoError(t, err) + + require.Equal(t, 1, prompter.selectCalls) + assert.Equal(t, PruneStatusDelete, plan.Actions[0].Status) + assert.True(t, plan.Actions[0].Reason.IsZero()) +} + +func TestConfirmPrunePlanDeletesSkipsDeleteItems(t *testing.T) { + t.Parallel() + + plan := &PrunePlan{ + Actions: []*PruneActionPlan{ + { + Source: &policy.Action{Id: "action-1", Name: "archive"}, + Status: PruneStatusDelete, + MigratedTargets: []TargetRef{{ID: "target-action-1", NamespaceFQN: "https://example.com"}}, + }, + { + Source: &policy.Action{Id: "action-2", Name: "export"}, + Status: PruneStatusDelete, + MigratedTargets: []TargetRef{{ID: "target-action-2", NamespaceFQN: "https://example.com"}}, + }, + }, + } + prompter := &queuedSelectPrompter{ + selectValues: []string{ + namespacedPolicyCommitSkip, + namespacedPolicyCommitConfirm, + }, + } + + err := ConfirmPrunePlanDeletes(t.Context(), plan, prompter) + require.NoError(t, err) + + require.Equal(t, 2, prompter.selectCalls) + assert.Equal(t, PruneStatusSkipped, plan.Actions[0].Status) + assert.Equal(t, PruneStatusReasonTypeSkippedByUser, plan.Actions[0].Reason.Type) + assert.Equal(t, skippedByUserReason, plan.Actions[0].Reason.Message) + assert.Equal(t, PruneStatusDelete, plan.Actions[1].Status) +} + +func TestConfirmPrunePlanDeletesAbortStopsWithoutMutatingCurrentItem(t *testing.T) { + t.Parallel() + + plan := &PrunePlan{ + Actions: []*PruneActionPlan{ + { + Source: &policy.Action{Id: "action-1", Name: "archive"}, + Status: PruneStatusDelete, + }, + { + Source: &policy.Action{Id: "action-2", Name: "export"}, + Status: PruneStatusDelete, + }, + }, + } + prompter := &queuedSelectPrompter{ + selectValues: []string{namespacedPolicyCommitAbort}, + } + + err := ConfirmPrunePlanDeletes(t.Context(), plan, prompter) + require.ErrorIs(t, err, ErrInteractiveReviewAborted) + + require.Equal(t, 1, prompter.selectCalls) + assert.Equal(t, PruneStatusDelete, plan.Actions[0].Status) + assert.Equal(t, PruneStatusDelete, plan.Actions[1].Status) +} + +func TestConfirmPrunePlanDeletesSkipsNilSourceAndNonDeleteItems(t *testing.T) { + t.Parallel() + + plan := &PrunePlan{ + Actions: []*PruneActionPlan{ + nil, + {Status: PruneStatusDelete}, + { + Source: &policy.Action{Id: "action-blocked", Name: "archive"}, + Status: PruneStatusBlocked, + }, + { + Source: &policy.Action{Id: "action-unresolved", Name: "export"}, + Status: PruneStatusUnresolved, + }, + { + Source: &policy.Action{Id: "action-skipped", Name: "share"}, + Status: PruneStatusSkipped, + }, + }, + } + prompter := &queuedSelectPrompter{ + selectValues: []string{namespacedPolicyCommitSkip}, + } + + err := ConfirmPrunePlanDeletes(t.Context(), plan, prompter) + require.NoError(t, err) + + assert.Equal(t, 0, prompter.selectCalls) + assert.Equal(t, PruneStatusDelete, plan.Actions[1].Status) + assert.Equal(t, PruneStatusBlocked, plan.Actions[2].Status) + assert.Equal(t, PruneStatusUnresolved, plan.Actions[3].Status) + assert.Equal(t, PruneStatusSkipped, plan.Actions[4].Status) +} + +func TestConfirmPrunePlanDeletesPromptsAllConstructs(t *testing.T) { + t.Parallel() + + plan := &PrunePlan{ + Actions: []*PruneActionPlan{ + {Source: &policy.Action{Id: "action-1", Name: "archive"}, Status: PruneStatusDelete}, + }, + SubjectConditionSets: []*PruneSubjectConditionSetPlan{ + {Source: &policy.SubjectConditionSet{Id: "scs-1"}, Status: PruneStatusDelete}, + }, + SubjectMappings: []*PruneSubjectMappingPlan{ + {Source: &policy.SubjectMapping{Id: "mapping-1"}, Status: PruneStatusDelete}, + }, + RegisteredResources: []*PruneRegisteredResourcePlan{ + {Source: testRegisteredResource("resource-1", "dataset"), Status: PruneStatusDelete}, + }, + ObligationTriggers: []*PruneObligationTriggerPlan{ + {Source: &policy.ObligationTrigger{Id: "trigger-1"}, Status: PruneStatusDelete}, + }, + } + prompter := &queuedSelectPrompter{ + selectValues: []string{ + namespacedPolicyCommitConfirm, + namespacedPolicyCommitConfirm, + namespacedPolicyCommitConfirm, + namespacedPolicyCommitConfirm, + namespacedPolicyCommitConfirm, + }, + } + + err := ConfirmPrunePlanDeletes(t.Context(), plan, prompter) + require.NoError(t, err) + + assert.Equal(t, 5, prompter.selectCalls) +} + +func TestApplyPruneDeleteConfirmationDecisionHandlesChoices(t *testing.T) { + t.Parallel() + + promptErr := errors.New("boom") + tests := []struct { + name string + selectValue string + selectErr error + wantSkipped bool + wantErr error + }{ + { + name: "confirm", + selectValue: namespacedPolicyCommitConfirm, + }, + { + name: "skip", + selectValue: namespacedPolicyCommitSkip, + wantSkipped: true, + }, + { + name: "abort", + selectValue: namespacedPolicyCommitAbort, + wantErr: ErrInteractiveReviewAborted, + }, + { + name: "prompt error", + selectErr: promptErr, + wantErr: promptErr, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + prompter := &queuedSelectPrompter{ + selectValues: []string{tt.selectValue}, + selectErr: tt.selectErr, + } + skipped := false + + err := applyPruneDeleteConfirmationDecision(t.Context(), prompter, SelectPrompt{Title: "test prompt"}, func() { + skipped = true + }) + if tt.wantErr == nil { + require.NoError(t, err) + assert.Equal(t, tt.wantSkipped, skipped) + return + } + require.ErrorIs(t, err, tt.wantErr) + assert.False(t, skipped) + }) + } +} diff --git a/otdfctl/migrations/namespacedpolicy/prune_execute_test.go b/otdfctl/migrations/namespacedpolicy/prune_execute_test.go index 0a97d13ba5..40f76af64d 100644 --- a/otdfctl/migrations/namespacedpolicy/prune_execute_test.go +++ b/otdfctl/migrations/namespacedpolicy/prune_execute_test.go @@ -114,6 +114,7 @@ func verifyPruneActionsExecuted(t *testing.T, plan *PrunePlan) { assert.True(t, plan.Actions[0].Execution.Applied) assert.True(t, plan.Actions[1].Execution.Applied) assert.Nil(t, plan.Actions[2].Execution) + assert.Nil(t, plan.Actions[3].Execution) } func verifyPruneSubjectConditionSetsExecuted(t *testing.T, plan *PrunePlan) { @@ -122,6 +123,7 @@ func verifyPruneSubjectConditionSetsExecuted(t *testing.T, plan *PrunePlan) { assert.True(t, plan.SubjectConditionSets[0].Execution.Applied) assert.True(t, plan.SubjectConditionSets[1].Execution.Applied) assert.Nil(t, plan.SubjectConditionSets[2].Execution) + assert.Nil(t, plan.SubjectConditionSets[3].Execution) } func verifyPruneSubjectMappingsExecuted(t *testing.T, plan *PrunePlan) { @@ -129,6 +131,7 @@ func verifyPruneSubjectMappingsExecuted(t *testing.T, plan *PrunePlan) { assert.True(t, plan.SubjectMappings[0].Execution.Applied) assert.True(t, plan.SubjectMappings[1].Execution.Applied) + assert.Nil(t, plan.SubjectMappings[2].Execution) } func verifyPruneRegisteredResourcesExecuted(t *testing.T, plan *PrunePlan) { @@ -136,6 +139,7 @@ func verifyPruneRegisteredResourcesExecuted(t *testing.T, plan *PrunePlan) { assert.True(t, plan.RegisteredResources[0].Execution.Applied) assert.True(t, plan.RegisteredResources[1].Execution.Applied) + assert.Nil(t, plan.RegisteredResources[2].Execution) } func verifyPruneObligationTriggersExecuted(t *testing.T, plan *PrunePlan) { @@ -143,6 +147,7 @@ func verifyPruneObligationTriggersExecuted(t *testing.T, plan *PrunePlan) { assert.True(t, plan.ObligationTriggers[0].Execution.Applied) assert.True(t, plan.ObligationTriggers[1].Execution.Applied) + assert.Nil(t, plan.ObligationTriggers[2].Execution) } func mixedPrunePlan(scope Scope) *PrunePlan { @@ -152,15 +157,18 @@ func mixedPrunePlan(scope Scope) *PrunePlan { {Source: &policy.Action{Id: "action-delete-1"}, Status: PruneStatusDelete}, {Source: &policy.Action{Id: "action-delete-2"}, Status: PruneStatusDelete}, {Source: &policy.Action{Id: "action-blocked"}, Status: PruneStatusBlocked}, + {Source: &policy.Action{Id: "action-skipped"}, Status: PruneStatusSkipped}, }, SubjectConditionSets: []*PruneSubjectConditionSetPlan{ {Source: &policy.SubjectConditionSet{Id: "scs-delete-1"}, Status: PruneStatusDelete}, {Source: &policy.SubjectConditionSet{Id: "scs-delete-2"}, Status: PruneStatusDelete}, {Source: &policy.SubjectConditionSet{Id: "scs-unresolved"}, Status: PruneStatusUnresolved}, + {Source: &policy.SubjectConditionSet{Id: "scs-skipped"}, Status: PruneStatusSkipped}, }, SubjectMappings: []*PruneSubjectMappingPlan{ {Source: &policy.SubjectMapping{Id: "mapping-delete-1"}, Status: PruneStatusDelete}, {Source: &policy.SubjectMapping{Id: "mapping-delete-2"}, Status: PruneStatusDelete}, + {Source: &policy.SubjectMapping{Id: "mapping-skipped"}, Status: PruneStatusSkipped}, }, RegisteredResources: []*PruneRegisteredResourcePlan{ { @@ -185,10 +193,15 @@ func mixedPrunePlan(scope Scope) *PrunePlan { }, Status: PruneStatusDelete, }, + { + Source: &policy.RegisteredResource{Id: "resource-skipped"}, + Status: PruneStatusSkipped, + }, }, ObligationTriggers: []*PruneObligationTriggerPlan{ {Source: &policy.ObligationTrigger{Id: "trigger-delete-1"}, Status: PruneStatusDelete}, {Source: &policy.ObligationTrigger{Id: "trigger-delete-2"}, Status: PruneStatusDelete}, + {Source: &policy.ObligationTrigger{Id: "trigger-skipped"}, Status: PruneStatusSkipped}, }, } } diff --git a/otdfctl/migrations/namespacedpolicy/prune_plan.go b/otdfctl/migrations/namespacedpolicy/prune_plan.go index 583d72e7ea..5a0887f4cc 100644 --- a/otdfctl/migrations/namespacedpolicy/prune_plan.go +++ b/otdfctl/migrations/namespacedpolicy/prune_plan.go @@ -13,6 +13,7 @@ const ( PruneStatusDelete PruneStatus = "delete" PruneStatusBlocked PruneStatus = "blocked" PruneStatusUnresolved PruneStatus = "unresolved" + PruneStatusSkipped PruneStatus = "skipped" targetRefSummaryPartCapacity = 2 ) @@ -26,6 +27,7 @@ const ( PruneStatusReasonTypeInUse PruneStatusReasonType = "InUse" PruneStatusReasonTypeNeedsMigration PruneStatusReasonType = "NeedsMigration" PruneStatusReasonTypeRegisteredResourceSourceMismatch PruneStatusReasonType = "RegisteredResourceSourceMismatch" + PruneStatusReasonTypeSkippedByUser PruneStatusReasonType = "SkippedByUser" pruneStatusReasonMessageMigratedTargetNotFound = "no migrated target was found for this source" pruneStatusReasonMessageInUse = "source object is still referenced by legacy policy" diff --git a/otdfctl/migrations/namespacedpolicy/prune_summary.go b/otdfctl/migrations/namespacedpolicy/prune_summary.go index e0f5e43241..53772de021 100644 --- a/otdfctl/migrations/namespacedpolicy/prune_summary.go +++ b/otdfctl/migrations/namespacedpolicy/prune_summary.go @@ -19,13 +19,18 @@ const ( prunePendingSectionLabel = "Will Delete" ) -func RenderNamespacedPolicyPruneSummary(plan *PrunePlan, commit bool) string { - return renderNamespacedPolicyPruneSummary(plan, commit, "success") -} +// PruneSummaryResult identifies the overall prune command outcome shown in the +// rendered summary. +type PruneSummaryResult string -func RenderNamespacedPolicyPruneSummaryWithResult(plan *PrunePlan, commit bool, result string) string { - return renderNamespacedPolicyPruneSummary(plan, commit, result) -} +const ( + // PruneSummaryResultSuccess indicates the prune command completed without error. + PruneSummaryResultSuccess PruneSummaryResult = "success" + // PruneSummaryResultFailure indicates prune commit execution failed. + PruneSummaryResultFailure PruneSummaryResult = "failure" + // PruneSummaryResultAborted indicates the user aborted an interactive prune flow. + PruneSummaryResultAborted PruneSummaryResult = "aborted" +) func prunePlanScopes(plan *PrunePlan) []Scope { if plan == nil { @@ -34,21 +39,21 @@ func prunePlanScopes(plan *PrunePlan) []Scope { return plan.Scopes } -func renderNamespacedPolicyPruneSummary(plan *PrunePlan, commit bool, result string) string { +func RenderNamespacedPolicyPruneSummary(plan *PrunePlan, executed bool, result PruneSummaryResult) string { styles := migrations.NewDisplayStyles() return renderSummaryDocument(styles, summaryDocument{ plannedTitle: "Namespaced Policy Prune Plan", committedTitle: "Namespaced Policy Prune Committed", operation: summaryOperationPrune, scopes: prunePlanScopes(plan), - commit: commit, - result: result, + commit: executed, + result: string(result), summaries: []constructSummary{ - summarizePruneActions(plan, commit, styles), - summarizePruneSubjectConditionSets(plan, commit, styles), - summarizePruneSubjectMappings(plan, commit, styles), - summarizePruneRegisteredResources(plan, commit, styles), - summarizePruneObligationTriggers(plan, commit, styles), + summarizePruneActions(plan, executed, styles), + summarizePruneSubjectConditionSets(plan, executed, styles), + summarizePruneSubjectMappings(plan, executed, styles), + summarizePruneRegisteredResources(plan, executed, styles), + summarizePruneObligationTriggers(plan, executed, styles), }, }) } @@ -57,7 +62,7 @@ func appendPruneSummaryCountParts(parts []string, counts summaryCounts) []string return append(parts, fmt.Sprintf("blocked=%d", counts.blocked)) } -func summarizePruneActions(plan *PrunePlan, commit bool, styles *migrations.DisplayStyles) constructSummary { +func summarizePruneActions(plan *PrunePlan, executed bool, styles *migrations.DisplayStyles) constructSummary { summary := constructSummary{ label: "Actions", include: includesScope(prunePlanScopes(plan), ScopeActions), @@ -69,12 +74,12 @@ func summarizePruneActions(plan *PrunePlan, commit bool, styles *migrations.Disp if action == nil || action.Source == nil { continue } - appendPruneStatusSummary(&summary, action, commit, styles) + appendPruneStatusSummary(&summary, action, executed, styles) } return summary } -func summarizePruneSubjectConditionSets(plan *PrunePlan, commit bool, styles *migrations.DisplayStyles) constructSummary { +func summarizePruneSubjectConditionSets(plan *PrunePlan, executed bool, styles *migrations.DisplayStyles) constructSummary { summary := constructSummary{ label: "Subject Condition Sets", include: includesScope(prunePlanScopes(plan), ScopeSubjectConditionSets), @@ -86,12 +91,12 @@ func summarizePruneSubjectConditionSets(plan *PrunePlan, commit bool, styles *mi if scs == nil || scs.Source == nil { continue } - appendPruneStatusSummary(&summary, scs, commit, styles) + appendPruneStatusSummary(&summary, scs, executed, styles) } return summary } -func summarizePruneSubjectMappings(plan *PrunePlan, commit bool, styles *migrations.DisplayStyles) constructSummary { +func summarizePruneSubjectMappings(plan *PrunePlan, executed bool, styles *migrations.DisplayStyles) constructSummary { summary := constructSummary{ label: "Subject Mappings", include: includesScope(prunePlanScopes(plan), ScopeSubjectMappings), @@ -103,12 +108,12 @@ func summarizePruneSubjectMappings(plan *PrunePlan, commit bool, styles *migrati if mapping == nil || mapping.Source == nil { continue } - appendPruneStatusSummary(&summary, mapping, commit, styles) + appendPruneStatusSummary(&summary, mapping, executed, styles) } return summary } -func summarizePruneRegisteredResources(plan *PrunePlan, commit bool, styles *migrations.DisplayStyles) constructSummary { +func summarizePruneRegisteredResources(plan *PrunePlan, executed bool, styles *migrations.DisplayStyles) constructSummary { summary := constructSummary{ label: "Registered Resources", include: includesScope(prunePlanScopes(plan), ScopeRegisteredResources), @@ -120,12 +125,12 @@ func summarizePruneRegisteredResources(plan *PrunePlan, commit bool, styles *mig if resource == nil || resource.Source == nil { continue } - appendPruneStatusSummary(&summary, resource, commit, styles) + appendPruneStatusSummary(&summary, resource, executed, styles) } return summary } -func summarizePruneObligationTriggers(plan *PrunePlan, commit bool, styles *migrations.DisplayStyles) constructSummary { +func summarizePruneObligationTriggers(plan *PrunePlan, executed bool, styles *migrations.DisplayStyles) constructSummary { summary := constructSummary{ label: "Obligation Triggers", include: includesScope(prunePlanScopes(plan), ScopeObligationTriggers), @@ -137,15 +142,15 @@ func summarizePruneObligationTriggers(plan *PrunePlan, commit bool, styles *migr if trigger == nil || trigger.Source == nil { continue } - appendPruneStatusSummary(&summary, trigger, commit, styles) + appendPruneStatusSummary(&summary, trigger, executed, styles) } return summary } -func appendPruneStatusSummary[T pruneSummaryItem](summary *constructSummary, item T, commit bool, styles *migrations.DisplayStyles) { +func appendPruneStatusSummary[T pruneSummaryItem](summary *constructSummary, item T, executed bool, styles *migrations.DisplayStyles) { switch item.status() { case PruneStatusDelete: - switch classifyPruneExecution(commit, item.execution()) { + switch classifyPruneExecution(executed, item.execution()) { case operationExecutionStateApplied: summary.counts.applied++ summary.applied = append(summary.applied, item.summaryLine(styles)) @@ -162,11 +167,14 @@ func appendPruneStatusSummary[T pruneSummaryItem](summary *constructSummary, ite case PruneStatusUnresolved: summary.counts.unresolved++ summary.unresolved = append(summary.unresolved, item.summaryLine(styles)) + case PruneStatusSkipped: + summary.counts.skipped++ + summary.skipped = append(summary.skipped, item.summaryLine(styles)) } } -func classifyPruneExecution(commit bool, execution *ExecutionResult) operationExecutionState { - if !commit || execution == nil { +func classifyPruneExecution(executed bool, execution *ExecutionResult) operationExecutionState { + if !executed || execution == nil { return operationExecutionStatePending } if len(strings.TrimSpace(execution.Failure)) != 0 { diff --git a/otdfctl/migrations/namespacedpolicy/prune_summary_test.go b/otdfctl/migrations/namespacedpolicy/prune_summary_test.go index d2fe5e738d..5ce43e93c0 100644 --- a/otdfctl/migrations/namespacedpolicy/prune_summary_test.go +++ b/otdfctl/migrations/namespacedpolicy/prune_summary_test.go @@ -47,7 +47,7 @@ func TestRenderNamespacedPolicyPruneSummaryDryRunShowsWillDelete(t *testing.T) { }, } - summary := stripANSI(RenderNamespacedPolicyPruneSummary(plan, false)) + summary := stripANSI(RenderNamespacedPolicyPruneSummary(plan, false, PruneSummaryResultSuccess)) assert.Contains(t, summary, "Namespaced Policy Prune Plan") assert.Contains(t, summary, "Scopes: actions") @@ -86,7 +86,7 @@ func TestRenderNamespacedPolicyPruneSummaryCommitSeparatesDeletedPendingAndFaile }, } - summary := stripANSI(RenderNamespacedPolicyPruneSummaryWithResult(plan, true, "failure")) + summary := stripANSI(RenderNamespacedPolicyPruneSummary(plan, true, PruneSummaryResultFailure)) assert.Contains(t, summary, "Namespaced Policy Prune Committed") assert.Contains(t, summary, "Result: failure") @@ -99,6 +99,32 @@ func TestRenderNamespacedPolicyPruneSummaryCommitSeparatesDeletedPendingAndFaile assert.Contains(t, summary, `action "share" (source_id=action-failed, found_migrated_targets=(none)): execution_failure=boom`) } +func TestRenderNamespacedPolicyPruneSummaryCommitShowsSkippedDeletes(t *testing.T) { + t.Parallel() + + plan := &PrunePlan{ + Scopes: []Scope{ScopeActions}, + Actions: []*PruneActionPlan{ + { + Source: &policy.Action{Id: "action-skipped", Name: "archive"}, + Status: PruneStatusSkipped, + Reason: newPruneReason( + PruneStatusReasonTypeSkippedByUser, + skippedByUserReason, + ), + }, + }, + } + + summary := stripANSI(RenderNamespacedPolicyPruneSummary(plan, true, PruneSummaryResultSuccess)) + + assert.Contains(t, summary, "Counts: deleted=0 skipped=1 blocked=0 failed=0 unresolved=0") + assert.Contains(t, summary, "Skipped") + assert.Contains(t, summary, `action "archive" (source_id=action-skipped, found_migrated_targets=(none)): reason=SkippedByUser: skipped by user`) + assert.NotContains(t, summary, "Will Delete") + assert.NotContains(t, summary, "\nDeleted\n") +} + func TestRenderNamespacedPolicyPruneSummaryActionsCoversEveryStatus(t *testing.T) { t.Parallel() @@ -132,7 +158,7 @@ func TestRenderNamespacedPolicyPruneSummaryActionsCoversEveryStatus(t *testing.T }, } - summary := stripANSI(RenderNamespacedPolicyPruneSummaryWithResult(plan, true, "failure")) + summary := stripANSI(RenderNamespacedPolicyPruneSummary(plan, true, PruneSummaryResultFailure)) assertPruneSummaryCoversEveryStatus(t, summary, "Actions", pruneSummaryStatusLines{ deleted: `action "archive" (source_id=action-deleted, found_migrated_targets=(none))`, @@ -176,7 +202,7 @@ func TestRenderNamespacedPolicyPruneSummarySubjectConditionSetsCoversEveryStatus }, } - summary := stripANSI(RenderNamespacedPolicyPruneSummaryWithResult(plan, true, "failure")) + summary := stripANSI(RenderNamespacedPolicyPruneSummary(plan, true, PruneSummaryResultFailure)) assertPruneSummaryCoversEveryStatus(t, summary, "Subject Condition Sets", pruneSummaryStatusLines{ deleted: `subject condition set "scs-deleted" (subject_sets=1, found_migrated_targets=(none))`, @@ -225,7 +251,7 @@ func TestRenderNamespacedPolicyPruneSummarySubjectMappingsCoversEveryStatus(t *t }, } - summary := stripANSI(RenderNamespacedPolicyPruneSummaryWithResult(plan, true, "failure")) + summary := stripANSI(RenderNamespacedPolicyPruneSummary(plan, true, PruneSummaryResultFailure)) assertPruneSummaryCoversEveryStatus(t, summary, "Subject Mappings", pruneSummaryStatusLines{ deleted: `subject mapping "mapping-deleted" (attribute_value=https://example.com/attr/classification/value/secret, actions="read", scs_source=scs-source, found_migrated_target=id: "target-mapping-deleted" namespace: "https://example.com")`, @@ -274,7 +300,7 @@ func TestRenderNamespacedPolicyPruneSummaryRegisteredResourcesCoversEveryStatus( }, } - summary := stripANSI(RenderNamespacedPolicyPruneSummaryWithResult(plan, true, "failure")) + summary := stripANSI(RenderNamespacedPolicyPruneSummary(plan, true, PruneSummaryResultFailure)) assertPruneSummaryCoversEveryStatus(t, summary, "Registered Resources", pruneSummaryStatusLines{ deleted: `registered resource "dataset-deleted" (source_id=resource-deleted, source=values="prod" (action_bindings="read" -> https://example.com/attr/classification/value/secret), found_migrated_target=id: "target-resource-deleted" namespace: "https://example.com")`, @@ -311,7 +337,7 @@ func TestRenderNamespacedPolicyPruneSummaryRegisteredResourceMismatchShowsSource }, } - summary := stripANSI(RenderNamespacedPolicyPruneSummary(plan, false)) + summary := stripANSI(RenderNamespacedPolicyPruneSummary(plan, false, PruneSummaryResultSuccess)) assert.Contains(t, summary, `registered resource "dataset" (source_id=resource-1, source=values="prod" (action_bindings="read" -> https://example.com/attr/classification/value/secret), found_migrated_target=id: "target-resource-1" namespace: "https://example.com"): reason=RegisteredResourceSourceMismatch: source mismatch`) assert.NotContains(t, summary, "filtered_source=") @@ -357,7 +383,7 @@ func TestRenderNamespacedPolicyPruneSummaryObligationTriggersCoversEveryStatus(t }, } - summary := stripANSI(RenderNamespacedPolicyPruneSummaryWithResult(plan, true, "failure")) + summary := stripANSI(RenderNamespacedPolicyPruneSummary(plan, true, PruneSummaryResultFailure)) assertPruneSummaryCoversEveryStatus(t, summary, "Obligation Triggers", pruneSummaryStatusLines{ deleted: `obligation trigger "trigger-deleted" (attribute_value=https://example.com/attr/classification/value/secret, action="read", obligation_value=https://example.com/obligation/log/value/default, context=client_id: "tdf-client", found_migrated_target=id: "target-trigger-deleted" namespace: "https://example.com")`, From d543773838bbaa989b58b8cfbc438bf2ab57ac1b Mon Sep 17 00:00:00 2001 From: Chris Reed Date: Wed, 13 May 2026 08:33:03 -0500 Subject: [PATCH 2/3] address gemini comments. --- .../cmd/migrate/prune/namespaced_policy.go | 26 ++++++++++++------- .../prune_commit_confirmation.go | 2 +- .../prune_commit_confirmation_test.go | 2 +- .../migrations/namespacedpolicy/prune_plan.go | 1 + .../namespacedpolicy/prune_summary_test.go | 2 +- 5 files changed, 20 insertions(+), 13 deletions(-) diff --git a/otdfctl/cmd/migrate/prune/namespaced_policy.go b/otdfctl/cmd/migrate/prune/namespaced_policy.go index 7ba0110c84..60264379bd 100644 --- a/otdfctl/cmd/migrate/prune/namespaced_policy.go +++ b/otdfctl/cmd/migrate/prune/namespaced_policy.go @@ -71,7 +71,12 @@ func pruneNamespacedPolicy(cmd *cobra.Command, args []string) { func executeNamespacedPolicyPruneCommit(cmd *cobra.Command, h namespacedpolicy.ExecutorHandler, plan *namespacedpolicy.PrunePlan, interactive bool, prompter namespacedpolicy.InteractivePrompter) { if interactive { - reviewNamespacedPolicyPruneInteractiveCommit(cmd, plan, prompter) + if err := reviewNamespacedPolicyPruneInteractiveCommit(cmd, plan, prompter); err != nil { + if namespacedPolicyPruneCommitAborted(err) { + writeNamespacedPolicyPruneSummary(cmd, plan, false, namespacedpolicy.PruneSummaryResultAborted) + } + cli.ExitWithError("could not review namespaced-policy prune commit", err) + } } executor, err := namespacedpolicy.NewExecutor(h) @@ -85,20 +90,21 @@ func executeNamespacedPolicyPruneCommit(cmd *cobra.Command, h namespacedpolicy.E } } -func reviewNamespacedPolicyPruneInteractiveCommit(cmd *cobra.Command, plan *namespacedpolicy.PrunePlan, prompter namespacedpolicy.InteractivePrompter) { +func reviewNamespacedPolicyPruneInteractiveCommit(cmd *cobra.Command, plan *namespacedpolicy.PrunePlan, prompter namespacedpolicy.InteractivePrompter) error { if err := namespacedpolicy.ConfirmNamespacedPolicyPruneBackup(cmd.Context(), prompter); err != nil { - if errors.Is(err, namespacedpolicy.ErrNamespacedPolicyBackupNotConfirmed) { - writeNamespacedPolicyPruneSummary(cmd, plan, false, namespacedpolicy.PruneSummaryResultAborted) - } - cli.ExitWithError("could not confirm namespaced-policy prune backup", err) + return err } if err := namespacedpolicy.ConfirmPrunePlanDeletes(cmd.Context(), plan, prompter); err != nil { - if errors.Is(err, namespacedpolicy.ErrInteractiveReviewAborted) { - writeNamespacedPolicyPruneSummary(cmd, plan, false, namespacedpolicy.PruneSummaryResultAborted) - } - cli.ExitWithError("could not review namespaced-policy prune commit", err) + return err } + + return nil +} + +func namespacedPolicyPruneCommitAborted(err error) bool { + return errors.Is(err, namespacedpolicy.ErrNamespacedPolicyBackupNotConfirmed) || + errors.Is(err, namespacedpolicy.ErrInteractiveReviewAborted) } func writeNamespacedPolicyPruneSummary(cmd *cobra.Command, plan *namespacedpolicy.PrunePlan, executed bool, result namespacedpolicy.PruneSummaryResult) { diff --git a/otdfctl/migrations/namespacedpolicy/prune_commit_confirmation.go b/otdfctl/migrations/namespacedpolicy/prune_commit_confirmation.go index e2a32e17df..f426b5ddf3 100644 --- a/otdfctl/migrations/namespacedpolicy/prune_commit_confirmation.go +++ b/otdfctl/migrations/namespacedpolicy/prune_commit_confirmation.go @@ -65,7 +65,7 @@ func confirmablePruneDeleteItem[T pruneReviewItem](item T) bool { func markPruneItemSkipped[T pruneReviewItem](item T) { item.setStatus(PruneStatusSkipped) - item.setReason(newPruneReason(PruneStatusReasonTypeSkippedByUser, skippedByUserReason)) + item.setReason(newPruneReason(PruneStatusReasonTypeSkippedByUser, pruneStatusReasonMessageSkippedByUser)) } func applyPruneDeleteConfirmationDecision(ctx context.Context, prompter InteractivePrompter, prompt SelectPrompt, markSkipped func()) error { diff --git a/otdfctl/migrations/namespacedpolicy/prune_commit_confirmation_test.go b/otdfctl/migrations/namespacedpolicy/prune_commit_confirmation_test.go index c1c17729cc..d90bf55f0f 100644 --- a/otdfctl/migrations/namespacedpolicy/prune_commit_confirmation_test.go +++ b/otdfctl/migrations/namespacedpolicy/prune_commit_confirmation_test.go @@ -65,7 +65,7 @@ func TestConfirmPrunePlanDeletesSkipsDeleteItems(t *testing.T) { require.Equal(t, 2, prompter.selectCalls) assert.Equal(t, PruneStatusSkipped, plan.Actions[0].Status) assert.Equal(t, PruneStatusReasonTypeSkippedByUser, plan.Actions[0].Reason.Type) - assert.Equal(t, skippedByUserReason, plan.Actions[0].Reason.Message) + assert.Equal(t, pruneStatusReasonMessageSkippedByUser, plan.Actions[0].Reason.Message) assert.Equal(t, PruneStatusDelete, plan.Actions[1].Status) } diff --git a/otdfctl/migrations/namespacedpolicy/prune_plan.go b/otdfctl/migrations/namespacedpolicy/prune_plan.go index 5a0887f4cc..afbf18e87e 100644 --- a/otdfctl/migrations/namespacedpolicy/prune_plan.go +++ b/otdfctl/migrations/namespacedpolicy/prune_plan.go @@ -35,6 +35,7 @@ const ( pruneStatusReasonMessageMismatchedMigrationLabel = "migrated target carries migrated_from metadata for a different source" pruneStatusReasonMessageMissingMigrationLabel = "migrated target is missing migrated_from metadata for this source" pruneStatusReasonMessageNeedsMigration = "source object does not have a migrated target yet" + pruneStatusReasonMessageSkippedByUser = "skipped by user" pruneStatusReasonMessageRegisteredResourceSourceMismatchFmt = "resolved registered resource view does not match the full source object for target namespace %q; source contains values outside the resolved migration view" ) diff --git a/otdfctl/migrations/namespacedpolicy/prune_summary_test.go b/otdfctl/migrations/namespacedpolicy/prune_summary_test.go index 5ce43e93c0..73f70cd608 100644 --- a/otdfctl/migrations/namespacedpolicy/prune_summary_test.go +++ b/otdfctl/migrations/namespacedpolicy/prune_summary_test.go @@ -110,7 +110,7 @@ func TestRenderNamespacedPolicyPruneSummaryCommitShowsSkippedDeletes(t *testing. Status: PruneStatusSkipped, Reason: newPruneReason( PruneStatusReasonTypeSkippedByUser, - skippedByUserReason, + pruneStatusReasonMessageSkippedByUser, ), }, }, From b5b7bfc930fe43d7ff27442eaee96e56ff6629ac Mon Sep 17 00:00:00 2001 From: Chris Reed Date: Wed, 13 May 2026 10:45:36 -0500 Subject: [PATCH 3/3] address comments. --- otdfctl/cmd/migrate/prune/namespaced_policy.go | 6 +----- .../namespacedpolicy/prune_commit_confirmation.go | 9 +++------ otdfctl/migrations/namespacedpolicy/prune_review.go | 9 +++------ 3 files changed, 7 insertions(+), 17 deletions(-) diff --git a/otdfctl/cmd/migrate/prune/namespaced_policy.go b/otdfctl/cmd/migrate/prune/namespaced_policy.go index 60264379bd..a543a3b584 100644 --- a/otdfctl/cmd/migrate/prune/namespaced_policy.go +++ b/otdfctl/cmd/migrate/prune/namespaced_policy.go @@ -95,11 +95,7 @@ func reviewNamespacedPolicyPruneInteractiveCommit(cmd *cobra.Command, plan *name return err } - if err := namespacedpolicy.ConfirmPrunePlanDeletes(cmd.Context(), plan, prompter); err != nil { - return err - } - - return nil + return namespacedpolicy.ConfirmPrunePlanDeletes(cmd.Context(), plan, prompter) } func namespacedPolicyPruneCommitAborted(err error) bool { diff --git a/otdfctl/migrations/namespacedpolicy/prune_commit_confirmation.go b/otdfctl/migrations/namespacedpolicy/prune_commit_confirmation.go index f426b5ddf3..ecb6be9a46 100644 --- a/otdfctl/migrations/namespacedpolicy/prune_commit_confirmation.go +++ b/otdfctl/migrations/namespacedpolicy/prune_commit_confirmation.go @@ -34,11 +34,8 @@ func ConfirmPrunePlanDeletes(ctx context.Context, plan *PrunePlan, prompter Inte if err := confirmDeletePruneItems(ctx, prompter, plan.RegisteredResources); err != nil { return err } - if err := confirmDeletePruneItems(ctx, prompter, plan.ObligationTriggers); err != nil { - return err - } - return nil + return confirmDeletePruneItems(ctx, prompter, plan.ObligationTriggers) } func confirmDeletePruneItems[T pruneReviewItem]( @@ -59,11 +56,11 @@ func confirmDeletePruneItems[T pruneReviewItem]( return nil } -func confirmablePruneDeleteItem[T pruneReviewItem](item T) bool { +func confirmablePruneDeleteItem(item pruneReviewItem) bool { return item.hasSource() && item.status() == PruneStatusDelete } -func markPruneItemSkipped[T pruneReviewItem](item T) { +func markPruneItemSkipped(item pruneReviewItem) { item.setStatus(PruneStatusSkipped) item.setReason(newPruneReason(PruneStatusReasonTypeSkippedByUser, pruneStatusReasonMessageSkippedByUser)) } diff --git a/otdfctl/migrations/namespacedpolicy/prune_review.go b/otdfctl/migrations/namespacedpolicy/prune_review.go index f44d263c72..cb75adc227 100644 --- a/otdfctl/migrations/namespacedpolicy/prune_review.go +++ b/otdfctl/migrations/namespacedpolicy/prune_review.go @@ -51,11 +51,8 @@ func ReviewPrunePlan(ctx context.Context, plan *PrunePlan, prompter InteractiveP if err := reviewUnresolvedPruneItems(ctx, prompter, plan.RegisteredResources); err != nil { return err } - if err := reviewUnresolvedPruneItems(ctx, prompter, plan.ObligationTriggers); err != nil { - return err - } - return nil + return reviewUnresolvedPruneItems(ctx, prompter, plan.ObligationTriggers) } func reviewUnresolvedPruneItems[T pruneReviewItem]( @@ -76,11 +73,11 @@ func reviewUnresolvedPruneItems[T pruneReviewItem]( return nil } -func reviewablePruneItem[T pruneReviewItem](item T) bool { +func reviewablePruneItem(item pruneReviewItem) bool { return item.hasSource() && item.status() == PruneStatusUnresolved } -func markPruneItemDelete[T pruneReviewItem](item T) { +func markPruneItemDelete(item pruneReviewItem) { item.setStatus(PruneStatusDelete) item.setReason(PruneStatusReason{}) }