-
Notifications
You must be signed in to change notification settings - Fork 17
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
Implement Enabled field on ManagedOSVersionChannels #800
Implement Enabled field on ManagedOSVersionChannels #800
Conversation
327a945
to
53182e4
Compare
@@ -9,11 +9,6 @@ seedImage: | |||
tag: "%VERSION%" | |||
imagePullPolicy: IfNotPresent | |||
|
|||
channel: |
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 is still supported if users want to bring in (or are already bringing in) a channel from a value. We just removed the default.
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.
what about keeping them with empty values or even just have them commented out? 🤔
Could be handy to have them mentioned somehow in the values.yaml to easily keep track of what we reference in the templates yaml.
ff8f778
to
a8e9efc
Compare
a57628e
to
5b8c9f3
Compare
5b8c9f3
to
2e9637f
Compare
Signed-off-by: Andrea Mazzotti <[email protected]>
2e9637f
to
94cac88
Compare
Signed-off-by: Andrea Mazzotti <[email protected]>
Signed-off-by: Andrea Mazzotti <[email protected]>
Signed-off-by: Andrea Mazzotti <[email protected]>
Signed-off-by: Andrea Mazzotti <[email protected]>
@@ -9,11 +9,6 @@ seedImage: | |||
tag: "%VERSION%" | |||
imagePullPolicy: IfNotPresent | |||
|
|||
channel: |
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.
what about keeping them with empty values or even just have them commented out? 🤔
Could be handy to have them mentioned somehow in the values.yaml to easily keep track of what we reference in the templates yaml.
.obs/chartfile/operator/values.yaml
Outdated
included: true | ||
# default channels to include | ||
includes: | ||
sle_micro_5_5: 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.
nit: we should try to keep the vars naming camelCase (https://helm.sh/docs/chart_best_practices/values/#naming-conventions)
wondering if we could have something more generic as var names 🤔
Even at the cost to have some more structured vars, something like:
defautlChannels:
sleMicro:
name: ....
image: ...
tag: ...
enabled: ...
sleMicroKVM:
name: ...
image: ...
....
... which still don't look that generic 🤦🏼 😆
BTW, I see the point of it, to enable the new questions.yaml.
Could make it even more generic, dropping any reference to the channel content (e.g., defaultChannels.chan1{name, image, tag, enabled}, defaultChannels:chan2, ... and use something like show_if:defaultChannels.chan2!=""
in questions.yaml)? 🤔
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 like the idea of making it more generic, but I'm struggling to make it working nice with the questions.
I also think we don't need to make the default channels configurable, we can hardcode all the values, as it's an opinionated choice.
I'll fix the snake_case variables as you mentioned and see if it can be more generic.
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.
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.
Picture above is how it looks like after the changes.
I made the structure a bit more generic.
I'm still playing out with the show_if
. I wonder if it's even necessary. Since chart and questions are linked, the expected map keys should always be found, show_if
only plays nice if we somehow keep the question but remove the default values, which I hope we won't. :D
I also exposed a boolean flag only to include/exclude the channels, further settings can be edited through values, but not from the Rancher UI when installed. I think this is reasonable, otherwise we will have too many options and checkboxes when we assume most users will use the default values.
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.
Just out of curiosity I tested the following and it does not show in the UI as expected:
- variable: defaultChannels.doesNotExist.included
show_if: "defaultChannels.doesNotExist"
default: true
description: "Foo bar"
type: boolean
label: Just for testing
group: "Default Elemental OS Channels"
So I think we can keep the show_if
just for safety.
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.
Well, we can drop show_if... if it was handy, great, otherwise...
My idea was to have "generic" default channel templates that we fill with values (today is SLEM-5.5, tomorrow SLEM-6.x) so to have a quicker way to update (i.e., just update the values from our side).
But yes, we are shipping the questions.yaml with the associated values, so it is just a matter of updating the values there... and I guess we are not going to automatically update channel SLE 5.5 to SLE 6.0, but we will keep them separated when will be the moment (or SLE-6.0 to SLE-6.1) and users will have to opt in 🤔
Or we may want SLE-6.x channels which are updated with all the supported minor versions... these anyway is on the released channels, and this chart PR would be ready for it.
Signed-off-by: Andrea Mazzotti <[email protected]>
Signed-off-by: Andrea Mazzotti <[email protected]>
Signed-off-by: Andrea Mazzotti <[email protected]>
Signed-off-by: Andrea Mazzotti <[email protected]>
475992e
to
5fa2b2e
Compare
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.
LGTM
Closes rancher/elemental#1462
This PR introduces a new
ManagedOSVersionChannel.spec.enabled
flag that can be used to disable a channel.Channels are always enabled by default, so disabling has to be explicit.
Disabling channels will also flag all linked ManagedOSVersions as no longer in sync.
Additionally, if
ManagedOSVersionChannel.spec.deleteNoLongerInSyncVersions
is true, all linked ManagedOSVersions will be flagged for deletion.