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

adds a guard to prevent overriding existing sealed-secrets installations #141

Merged
merged 2 commits into from
Mar 20, 2018

Conversation

prydonius
Copy link
Contributor

Since Kubeapps installs sealed secrets in the kube-system namespace, if you already have it installed it can accidentally override this. This PR adds a prompt to the user to check if they really want to override sealed-secrets.

@prydonius prydonius requested review from jjo and sameersbn March 1, 2018 11:21
@prydonius
Copy link
Contributor Author

@sameersbn can I get 👀 on this?

Copy link
Contributor

@sameersbn sameersbn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! I think that message should be displayed as a warning and since the effects of inadvertently overriding the existing controller could be catastrophic, I suggest adding a double confirmation or at the least making the user type "yes" or "no".

WARNING: sealed-secrets exists and was not installed by Kubeapps, continuing could override and interfere with your existing sealed-secrets controller.
Continue? (y/n): y
Are you really sure? (yes/no):  

@prydonius
Copy link
Contributor Author

prydonius commented Mar 19, 2018

@sameersbn applied your changes. r.e. the second prompt, I haven't changed the confirmPrompt function to actually only accept "yes" or "no" in the second case (it will still accept "y"), but the message does indicate "yes"/"no" which will hopefully direct most people to type it out.

@prydonius prydonius merged commit 4a66d1c into vmware-tanzu:master Mar 20, 2018
@prydonius prydonius deleted the guard branch March 20, 2018 11:02
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