-
Notifications
You must be signed in to change notification settings - Fork 14
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
delete by query and morgue nuke protections #97
base: master
Are you sure you want to change the base?
Conversation
jshufro
commented
Jan 22, 2023
•
edited
Loading
edited
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly ok. Some minor concerns in the comments.
bin/morgue.js
Outdated
@@ -5067,6 +5068,24 @@ function coronerNuke(argv, config) { | |||
errx('No such object.'); | |||
} | |||
|
|||
if (!argv["-f"]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we sure we want to provide this opt out from the verification? We're going to break folks' command lines anyway with this change to prevent accidental foot gunning, but if we provide an opt out, then the command lines will simply adapt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The footgunning that this is reacting to was an interactive session, so it would prevent repeats of that.
I don't think removing the ability to script around morgue nuke
is a sensible thing to do without more discussion
bin/morgue.js
Outdated
terminal: true | ||
}); | ||
await new Promise(done => { | ||
rl.question("You are about to nuke " + name + ". Please type '" + name +"' to confirm: ", confirmation => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In former projects I've used short random nonce for this kind of thing, but I suppose adding an "echo $name" to your script really means you mean it. Maybe to make it super clear in such scripts, consider asking for "really nuke $name" or something like that, like AWS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AWS, to my knowledge, either requires strictly the name
or the word delete
in some cases
c51c628
to
aa80580
Compare