chore(cli): direct tests for the deletion-retention click entry points; non-zero exit on blocked/not-found force-purge - #44252
Merged
richardfogaca merged 2 commits intoSep 14, 2026
Conversation
…sc-115409) Raised twice by rusackas on apache#41549 and by bito: superset/cli/deletion_retention.py sat at 0% coverage. The command classes underneath were covered; the CLI surface itself — option parsing, the irreversible confirmation prompt on force-purge, the operator-visible output, exit codes — was not. 31 CliRunner tests, unit-scoped (command objects mocked at their import source, no database): - group registers exactly set-window / show-window / force-purge - set-window: upserts the shared value for 0 / N / large; short option; negative is a usage error (exit 2) with nothing written; missing or non-integer --days never reaches the upsert - show-window: N day(s) vs "disabled" for zero - force-purge parsing: malformed --uuid fails up front (exit 2) BEFORE the prompt and before the command is constructed; --uuid required; unknown --type rejected - the prompt: shown without --yes; empty answer and "n" abort (exit 1) with no command constructed; "y" runs it; --yes bypasses it with no prompt text - --type resolution: each type maps to its soft-delete model, None means every model, case-insensitive, and a type-map drift is a clean ClickException rather than a silent widen to every model; the resolved model reaches the command - outcomes: successful purge output with counts (and with counts absent), blocked (names the blocking reason), nothing to purge, and an ambiguous bare UUID surfacing as a clean exit-1 error naming --type Pinned as-is, flagged for a decision rather than changed here: a blocked or not-found purge currently exits 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
`superset deletion-retention force-purge` reported a refused (blocked by a deletion rule) or absent target on stdout and exited 0 — indistinguishable, to a script, from a completed purge. For an operator-facing compliance erasure that is a footgun: a runbook could believe an entity was erased when it was refused. Both outcomes now exit 1 with the messages unchanged; a completed purge still exits 0 and a usage error still exits 2. Deliberate contract change, noted in UPDATING; the two pinned tests flipped. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01267VBWbvWTNZUg9GvXKgkC
Contributor
Code Review Agent Run #b59b07Actionable Suggestions - 0Additional Suggestions - 1
Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #44252 +/- ##
==========================================
+ Coverage 76.29% 80.19% +3.89%
==========================================
Files 2925 2925
Lines 172692 172692
Branches 40090 40090
==========================================
+ Hits 131758 138487 +6729
+ Misses 38247 31612 -6635
+ Partials 2687 2593 -94
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
richardfogaca
approved these changes
Sep 14, 2026
richardfogaca
left a comment
richardfogaca
left a comment
Contributor
There was a problem hiding this comment.
Richard's agent here:
Reviewed 7cd6b44590b2; no substantive findings. CI is green. Review was based on source and available test evidence; full local integration/browser validation was not performed.
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
2 participants
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
superset/cli/deletion_retention.pysat at 0% coverage. The command classes underneath (ForcePurgeCommand,resolve_retention_window) are covered, but nothing drove the click entry points themselves — option parsing, the irreversible confirmation prompt onforce-purge, the operator-visible output the CLI is the only consumer of, and exit codes. Raised twice by @rusackas on #41549 (07-13 and 07-29) and independently by bito; committed to as a follow-up there. Ref SC-115409.31 CliRunner tests, unit-scoped (the command objects are mocked at their import source, so no database):
set-window/show-window/force-purgeset-window: upserts the shared value for0/N/ large; short option; a negative window is a usage error (exit 2) with nothing written; missing or non-integer--daysnever reaches the upsertshow-window:N day(s)vsdisabledfor zeroforce-purgeparsing: a malformed--uuidfails as a clean usage error before the prompt and before the command is constructed (the reason that option isclick.UUID);--uuidrequired; unknown--typerefused--yes; an empty answer andnabort (exit 1) with no command constructed;yruns it;--yesbypasses with no prompt text--typeresolution: each type maps to its soft-delete model,Nonemeans every model, case-insensitive, and a drift in the type map surfaces as aClickExceptionrather than silently widening the purge to every model; the resolved model reaches the command--typeOne deliberate contract change (second commit): a
force-purgewhose target is blocked by a deletion rule or not found now exits 1 instead of 0, with the messages unchanged. Exiting 0 on a refusal was an operator footgun — a scripted compliance-erasure runbook could not tell a refused purge from a completed one. A completed purge still exits 0; a usage error still exits 2. Noted inUPDATING.md.Two things the ticket's original wording assumed that the shipped CLI does not have: there is no dry-run option on
force-purge(nothing to pin), and the exit-code point above was the "non-zero on blocked" it asked for.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
No UI. Operator-visible change:
force-purgeexit status on blocked / not-found (messages identical).TESTING INSTRUCTIONS
Manual:
superset deletion-retention force-purge --uuid <uuid-of-a-chart-referenced-by-a-report> --yes; echo $?→ the "not purged because existing deletion rules block it" message and exit status1.ADDITIONAL INFORMATION
This PR was developed with AI assistance (Claude Code); a human (@mikebridge) reviews before merge.