Skip to content
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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

jshufro
Copy link
Contributor

@jshufro jshufro commented Jan 22, 2023

image

Copy link

@jpihlaja-bt jpihlaja-bt left a 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"]) {

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.

Copy link
Contributor Author

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 => {

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.

Copy link
Contributor Author

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

bin/morgue.js Outdated Show resolved Hide resolved
@jshufro jshufro force-pushed the jms/T22737 branch 3 times, most recently from c51c628 to aa80580 Compare January 31, 2023 00:16
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.

2 participants