-
Notifications
You must be signed in to change notification settings - Fork 17
delete by query and morgue nuke protections #97
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
base: master
Are you sure you want to change the base?
Conversation
jpihlaja-bt
left a comment
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
| 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
Uh oh!
There was an error while loading. Please reload this page.