feat(delete/node): challenge deletion by text phrase #2248
Conversation
There was a problem hiding this comment.
@hansbogert Thank you Hans for this PR! Please view comment.
internal/ui/dialog/delete.go
Outdated
|
|
||
| confirmCheck := "" | ||
| if challenge { | ||
| f.AddInputField("Confirm with \"yes, I want to delete\":", confirmCheck, 30, nil, func(text string) { |
There was a problem hiding this comment.
It may make sense to actually pass in the challenge string ie if set add the form field and validate. Then we can reuse this functionality to namespace to fix the original PR.
Also not sure how much we want to make folks type ie challenge: Sanitize me! or a simple `Yes!' might be enough to disrupt the flow and make users stop/think??
There was a problem hiding this comment.
I made the changes, and at least for deletion of nodes, it is now "delete!"
Do we want to make this default behaviour, or do we want to add a configuration toggle for this?
Would be nice to have the option as a user, to choose the resources you want to be challenged.
There was a problem hiding this comment.
Thank you for the updates Hans! Good point!!
I think doing this on a per resource basis would be a bit much. In k9sAlpha, I have the concept of an idiotLight. When this config is set, then confirm and deletion dialogs surface a new field aka Confirm with a challenge phrase. I think it would be a good idea to do the same here so folks can opt-in/out for additional guardrails. Would this make sense?
There was a problem hiding this comment.
Agreed, will look into it when i have the time.
There was a problem hiding this comment.
in constrast to K9sa I made the idiot-light option false by default, to not surprise users after an update.
hansbogert
force-pushed
the
master
branch
2 times, most recently
from
e837b31
to
c4b8539
Compare
derailed
added
enhancement
|
FYI This PR might resolve #2076 especially if this behavior is opt-in via config toggle or cli argument |
|
Just rebased to master, @derailed when you have the time, can you give a final verdict and/or merge this? |
|
@derailed is there something I can do in regards to the 'tlc' tag? |
|
@hansbogert Yes please take a peek at the conflicts with the latest drop. Thank you! |
|
@derailed Updated. |
|
@derailed Gentle nudge, I have to keep rebasing/conflict-resolve. Is the current change good enough? |
And put node deletion challenge behind the idiot-light
|
@derailed is this getting in? |
A really simplistic change to not get burned by an accidental deletion of a node.
This is after 15min of looking through the k9s code and getting the functionality in asap; meant as inspiration for #1016
Help to make this a production ready change would be welcome.