-
Notifications
You must be signed in to change notification settings - Fork 31
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
Fix the "delete namespace" prompt #406
Conversation
We were improperly lower-casing the user's input when asking them to confirm the namespace name, and then providing no feedback to the user if the name didn't match, leaving them with the false impression that the namespace was deleted even though we didn't even try.
} | ||
fmt.Println() | ||
|
||
if len(expectedInputs) == 0 { | ||
return true | ||
} | ||
|
||
textLower := strings.ToLower(strings.TrimSpace(text)) | ||
for _, expectedInput := range expectedInputs { |
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.
nit: can use https://pkg.go.dev/slices#Contains
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.
Same as below, I don't really want to make any more changes than absolutely necessary since this is a hotfix.
@@ -303,7 +303,7 @@ func DeleteNamespace(c *cli.Context) error { | |||
|
|||
promptMsg := color.Red(c, "Are you sure you want to delete namespace %s? Type namespace name to confirm:", ns) | |||
if !common.Prompt(promptMsg, c.Bool(common.FlagYes), ns) { |
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.
We shouldn't print the prompt if the user wants to auto-confirm.
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.
We print both the prompt and response, which I think is okay for this hotfix. I don't really want to expand the scope of this PR since we're working on a rewrite.
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.
Nothing blocking. We can handle these comments in the rewrite.
@cretz FYI.
We were improperly lower-casing the user's input when asking them to confirm the namespace name, and then providing no feedback to the user if the name didn't match, leaving them with the false impression that the namespace was deleted even though we didn't even try.
Closes #405