mikebridge commented on PR #41549: URL: https://github.com/apache/superset/pull/41549#issuecomment-5109467545
Thanks for digging those out of the accordions — two of them were worth taking, and they're in `db7ed72c52`: - **Dev config feature gate.** You're right that this is a trap: the beat schedule registers `deletion_retention.purge_soft_deleted` but `SOFT_DELETE` is off by default, so it runs nightly and purges nothing. I've noted the gate inline at the schedule entry, where someone reading it will actually see it. I stopped short of flipping `SOFT_DELETE: True` in the dev `FEATURE_FLAGS` — that changes deletion behaviour for everyone using the dev compose stack, which feels like its own decision rather than a drive-by in this PR. Happy to do it if you'd rather. - **Test name.** Renamed to `test_force_purge_counts_removed_dashboard_slices_before_db_cascade`. Bito's stated reason was off (the "reports" in the old name was the verb, not `ReportSchedule`), but it lands on a fair point regardless — "reports" is a loaded noun in this codebase and the name read ambiguously. Two I'd push back on: - **`except Exception` in `window_tests.py`.** That's in `tearDown`, wrapping a `KeyValueDAO.delete_entry` with a rollback, and it already carries `# pylint: disable=broad-except`. Broad catching is the correct shape there — narrowing it would let a teardown failure mask the actual test failure, which is the opposite of what you want when a purge test goes wrong. - **`dict[str, object]` → `dict[str, Any]`.** The suggestion's rationale doesn't hold up: it claims this "causes a type-checker to flag a type mismatch on every assertion," but `==` is defined on `object` and mypy is green on this branch. `Any` would be marginally more consistent with the declared return type, so I'm not against it — I just didn't want to churn a line on a premise that isn't true. Say the word if you'd like it changed. **CLI-level tests — agreed, and taking it as a follow-up.** The gap is real: `set_window`, `show_window`, and `force_purge` are tested at the command layer but not through the click entry points, so argument parsing, the confirmation prompt, and output formatting are uncovered. That's meaningful surface for an operator-facing destructive command. It's also a chunk of new test infrastructure on a PR that's already large, and this branch has been through a rebase and a review round since your approval, so I'd rather not reopen it for test scaffolding. Filing it alongside the other two follow-ups from this PR. One thing worth flagging since you're here: your approval is from 07-13, and the head has moved a fair bit since — a 7-lens review round hardened the purge (cutoff-clock semantics reverted to match `soft_delete()`'s naive-local stamp, explicit `embedded_dashboards` deletion, suppression ordering, chunked `IN ()`, `PurgeAuditLog` moved under `superset/models/` so it registers with `Model.metadata`, and fail-closed write-ahead auditing for the scheduled path). CI is green on the current head. No obligation, but if you'd rather re-read it before it merges, now's the moment. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
