Skip to content

feat(drain-plan): one deliberate choice for emptyDir data, not two - #1784

Merged
hisco merged 1 commit into
mainfrom
feat/drain-emptydir-single-gate
Sep 16, 2026
Merged

hisco merged 1 commit into
mainfrom
feat/drain-emptydir-single-gate

Conversation

@hisco

@hisco hisco commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #1718 and #1758.

Enabling "Delete emptyDir data" in the drain dialog required ticking a second checkbox, inside a red banner, before Drain would enable. On every drain.

The second tick isn't earning its click. The option is off by default, so turning it on is already the deliberate act. The banner names the exact pods whose data would go. Nothing about the second checkbox tells the operator something the first one and the warning didn't.

So the banner stays, as a warning rather than an error, and stops being a gate.

What changes

  • canConfirmDrain no longer takes acknowledgedEmptyDir, and no longer blocks on it.
  • DrainPlanContent drops the acknowledgedEmptyDir / onAcknowledgeEmptyDir props.
  • DrainPlanDialog drops the acknowledgement state, the effect that reset it on option and plan changes, and the refresh wrapper that cleared it at click time.
  • The three warning texts are kept, including the honest one for when the estimate lists no emptyDir pod, since the drain runs against live state.

Unchanged: the option is still off by default, ResourceActionsBar is untouched, and hosts without plan support behave as before.

Note for library consumers

canConfirmDrain is exported from @skyhook-io/k8s-ui, and this removes a field from its input type. Radar is the only caller, but anyone importing it directly would need to drop that argument.

Background

Issue #1584 asked for the option to start disabled and to require an explicit acknowledgement. It got both, as a checkbox plus a second checkbox. The first one satisfies the requirement on its own.

The contributor who built the original dialog flagged that the acknowledgement fires even when the plan lists no emptyDir pod, and explained why he kept it: the plan is a snapshot and the drain runs against live state. That reasoning is right, which is why the warning keeps saying exactly that. It just doesn't need to be a gate to say it. He's been told this is happening on #1718.

Verified

DrainPlanDialog.test.tsx 16/16, web drain-plan.test.ts 7/7, make tsc clean.

https://claude.ai/code/session_01NE55VsYGkb5tV3neKDmtZw


Note

Low Risk
UI-only change to drain confirmation flow; emptyDir deletion remains opt-in with explicit warnings, but one fewer confirmation step before data loss.

Overview
Removes the second acknowledgement checkbox for draining nodes when Delete emptyDir data is enabled. Enabling that option (still off by default) is treated as the deliberate choice; the drain button no longer waits on a separate tick inside the banner.

canConfirmDrain drops acknowledgedEmptyDir from its input and no longer blocks confirm when deleteEmptyDirData is on. Plan-based gating is unchanged: with plan support you still need a current matching plan (and no plan error); without plan support you can confirm with or without emptyDir enabled.

DrainPlanContent replaces the red error banner + acknowledgement checkbox with a warning AlertBanner that names at-risk pods (or the honest “no emptyDir in the estimate” text) without gating. DrainPlanDialog removes acknowledgement state, reset effects, and the refresh wrapper that cleared acknowledgement on recompute.

Tests are updated to match the new confirm rules and markup (no acknowledgement checkbox).

Reviewed by Cursor Bugbot for commit 3262279. Bugbot is set up for automated code reviews on this repo. Configure here.

Enabling "Delete emptyDir data" required ticking a second checkbox in a
red banner before Drain would enable, on every drain. The option is off
by default and is already the deliberate act, and the banner names the
pods whose data would go, so the second tick cost a click on every
routine drain without telling the operator anything new. It is now a
warning rather than a gate.

Claude-Session: https://claude.ai/code/session_01NE55VsYGkb5tV3neKDmtZw
@hisco
hisco requested a review from nadaverell as a code owner September 16, 2026 09:42
@qodo-free-for-open-source-projects

Copy link
Copy Markdown

PR Summary by Qodo

Remove redundant emptyDir acknowledgement from drain plans

✨ Enhancement 🧪 Tests 🕐 10-20 Minutes

Grey Divider

AI Description

• Removes the redundant emptyDir acknowledgement while keeping the destructive option disabled by
 default.
• Presents pod-specific data-loss guidance as a warning instead of an error gate.
• Updates confirmation and rendering tests for the single-choice workflow.
Diagram

graph TD
  A["Operator"] --> B["Drain options"] --> C{"Plan supported?"}
  C -- "Yes" --> D["Current plan"] --> E["Drain enabled"]
  C -- "No" --> E
  B --> F["emptyDir warning"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Deprecated compatibility field
  • ➕ Keeps existing external canConfirmDrain callers compiling unchanged.
  • ➕ Allows the obsolete argument to be removed in a later breaking release.
  • ➖ Retains a misleading, unused field in the public API.
  • ➖ Delays cleanup and requires deprecation communication.

Recommendation: The warning-only interaction is preferable because enabling the default-off destructive option is already deliberate, while the warning preserves risk visibility. Since canConfirmDrain is publicly exported, consider retaining acknowledgedEmptyDir as an ignored deprecated optional field for one release if package compatibility guarantees apply; otherwise the PR's clean removal is appropriate.

Files changed (2) +41 / -90

Enhancement (1) +22 / -55
DrainPlanDialog.tsxReplace emptyDir acknowledgement gate with a warning +22/-55

Replace emptyDir acknowledgement gate with a warning

• Removes acknowledgement state, props, reset effects, and confirmation gating from the drain dialog. The default-off emptyDir option now triggers a warning banner that identifies affected pods when available and explains live-state data-loss risk when the estimate is empty or unavailable.

packages/k8s-ui/src/components/shared/DrainPlanDialog.tsx

Tests (1) +19 / -35
DrainPlanDialog.test.tsxTest warning-only emptyDir confirmation behavior +19/-35

Test warning-only emptyDir confirmation behavior

• Removes acknowledgement arguments and assertions from confirmation tests. Adds coverage that emptyDir deletion does not introduce a second gate, including hosts without plan support, while verifying pod-specific and live-state warning text without an acknowledgement checkbox.

packages/k8s-ui/src/components/shared/DrainPlanDialog.test.tsx

@qodo-free-for-open-source-projects

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Tip of the day
💡 Did you know, you can group findings by type and pick your Finding display, from Minimal to Full

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

@hisco
hisco merged commit 77a4a2f into main Sep 16, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant