-
Notifications
You must be signed in to change notification settings - Fork 83
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
make cli-install feature optional #2072
Conversation
charts/astronomer/values.yaml
Outdated
@@ -423,6 +423,7 @@ registry: | |||
logLevel: info | |||
|
|||
install: | |||
enabled: true |
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.
.Values.install.enabled
is not conveying the reference to CLI
We should rename the flag to cliEnabled
for better clarity
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.
this might affect existing deployment config since customers are using this for a long time we can add both configs and we can state that old config will be removed in 0.34
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.
i agree with @rishkarajgi that having cliInstall
would have been better, but i also agree with @pgvishnuram that changing the parent namespace of the new enabled
parameter is outside of the scope of this ticket and could introduce problems. if we want to improve the install
value namespace, we should do that on a minor version bump and address that in another ticket.
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.
This all looks great to me. If we want to address the install
-> cliInstall
change, we should open another ticket and schedule that work.
Description
make cli install service optionable
Related Issues
https://github.com/astronomer/issues/issues/6015
Testing
Yet to update
Merging
Yet to update