-
Notifications
You must be signed in to change notification settings - Fork 357
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
feat(vclusterctl): Prevent distro/backingstore switch #1694
feat(vclusterctl): Prevent distro/backingstore switch #1694
Conversation
✅ Deploy Preview for vcluster-docs canceled.
|
507de68
to
caaa445
Compare
caaa445
to
6c8872b
Compare
6c8872b
to
6759581
Compare
return err | ||
} | ||
|
||
if ok, err := setup.CheckUsingHelm(ctx, cmd.kubeClient, args[0], cmd.Namespace, cmd.Distro, cfg.BackingStoreType()); err != nil { |
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.
Since people can configure the distro via values now as well, I think this can be problematic here. I would leave out the CLI check for now
// TODO(johannesfrey): We would also need to validate here if the user is about to perform changes which would lead to distro/store changes | ||
// (johannesfrey): We would also need to validate here if the user is about to perform changes which would lead to distro/store changes | ||
if isCurrentlyDeployed { | ||
if ok, err := setup.CheckUsingHelm(ctx, cmd.kubeClient, args[0], cmd.Namespace, cfg.Distro(), cfg.BackingStoreType()); err != nil { |
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 above, maybe we should wait with CLI check for now
What issue type does this pull request address? (keep at least one, remove the others)
/kind bugfix
/kind enhancement
/kind feature
/kind documentation
/kind test
What does this pull request do? Which issues does it resolve? (use
resolves #<issue_number>
if possible)resolves #
Please provide a short message that should be published in the vcluster release notes
Fixed an issue where vcluster ...
What else do we need to know?