feat(drain-plan): one deliberate choice for emptyDir data, not two - #1784
Merged
Merged
Conversation
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
PR Summary by QodoRemove redundant emptyDir acknowledgement from drain plans
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can group findings by type and pick your Finding display, from Minimal to Full |
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
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.
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
warningrather than anerror, and stops being a gate.What changes
canConfirmDrainno longer takesacknowledgedEmptyDir, and no longer blocks on it.DrainPlanContentdrops theacknowledgedEmptyDir/onAcknowledgeEmptyDirprops.DrainPlanDialogdrops the acknowledgement state, the effect that reset it on option and plan changes, and the refresh wrapper that cleared it at click time.Unchanged: the option is still off by default,
ResourceActionsBaris untouched, and hosts without plan support behave as before.Note for library consumers
canConfirmDrainis 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.tsx16/16, webdrain-plan.test.ts7/7,make tscclean.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.
canConfirmDraindropsacknowledgedEmptyDirfrom its input and no longer blocks confirm whendeleteEmptyDirDatais 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.DrainPlanContentreplaces the red error banner + acknowledgement checkbox with a warningAlertBannerthat names at-risk pods (or the honest “no emptyDir in the estimate” text) without gating.DrainPlanDialogremoves 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.