Ignore soft dependencies of LCM and chart prioritization - #559
Conversation
If Rancher chart is listed in `release.yaml` or custom certificate is configured through `values.yaml` for LCM chart, `elemental3 customize` would implicitly remove SUC and cert-manager from LCM's dependencies. Signed-off-by: Dharmit Shah <dharmit.shah@suse.com>
To prevent users from overriding the version of chart(s) required by Elemental, this commit prioritizes chart in Core manifest over the same one in Solution manifest Signed-off-by: Dharmit Shah <dharmit.shah@suse.com>
ipetrov117
left a comment
ipetrov117
left a comment
There was a problem hiding this comment.
Good job on this. I left a couple of suggestions/comments, please let me know what you think.
| ) | ||
|
|
||
| // lcmWebhookValues is used to marshal data from values file for LCM chart | ||
| type lcmWebhookValues struct { |
There was a problem hiding this comment.
I do not foresee this type being every used outside of evaluateLCMDeps, as such would it make sense to include it as an anonymous struct inside the func?
Also we could maybe simplify the struct itself to something like:
var lcmWebhookValues struct {
Webhook struct {
Cert *struct {
CreateDefault bool `yaml:"createDefault"`
} `yaml:"cert"`
} `yaml:"webhook"`
}And then check against the Cert struct pointer and the CreateDefault field themselves. That would be enough, as if the user has continuously provided the "cert:" field with a non-true createDefault, then it is up to them to define the other fields, in our eyes this should be enough for the trigger to disable the cert-manager dependency.
| // evaluateLCMDeps removes dependencies of LCM if they are satisfied separately, i.e., | ||
| // - if Rancher chart is enabled in release.yaml, it removes dependency on system-upgrade-controller | ||
| // - if the values files contains custom certificate configuration, it removes dependency on cert-manager | ||
| func evaluateLCMDeps(enabled []release.HelmChart, corePlatform *core.ReleaseManifest, valueFiles map[string]string, valuesResolver helmValuesResolver) error { |
There was a problem hiding this comment.
A couple of suggestions here:
- I am not sure that the
valueFiles map[string]stringparameter is needed here. The values file location can be taken from the LCM entry in theenabled []release.HelmChartslice. - This function seems to be doing too much, could we perhaps split out some of its logic in separate functions. For example:
- We could extract the logic to find a chart from the core manifest in a separate generic function that could be reused for other use-cases in the future.
- We could extract the logic to resolve a specific charts values (or a portion of it) in a separate generic function that could then be reused for other use-cases in the future.
| ) | ||
| for _, chart := range enabled { | ||
| if chart.Name == rancher { | ||
| rancherEnabled = true |
There was a problem hiding this comment.
This may be a personal preference, but instead of populating boolean values, would it make sense to convert the enabled slice to a map and check against the map keys. That way we could do the dependency removal in the same block that we check whether the chart is enabled.
| } | ||
| if !lcmEnabled { | ||
| // nothing to do! | ||
| return nil |
There was a problem hiding this comment.
Shouldn't we error out here? IIRC, we agreed that LCM is a required chart that should be specified by the user under release.yaml. By returning nil here, we do not have any gate that ensures that LCM will be on the cluster. Probably missing something.
| if lcmChart == nil { | ||
| // this could be the case if using core manifest that doesn't contain LCM charts which is the case currently | ||
| // TODO (dharmit): remove this check once the core manifest includes LCM chart by default | ||
| return nil // nothing to do! |
There was a problem hiding this comment.
Shouldn't we also error out here as well? My main thinking is that LCM will always be a part of the core manifest, so it not being present in the parsed corePlatform struct should be flagged.
Or did you want to make the elemental3 binary backwards compatible? If that is the case, I guess we need to decide how we want to handle this. IMO if LCM is going to be an expected component in our setup we need to be explicit here and guard against a missing LCM in release.yaml as well as in the core manifest. Let me know what you think.
| // remove system-upgrade-controller from list of dependencies | ||
| for i, dep := range lcmChart.DependsOn { | ||
| if dep.Name == systemUpgradeController { | ||
| lcmChart.DependsOn = slices.Delete(lcmChart.DependsOn, i, i+1) |
There was a problem hiding this comment.
This may be a personal preference, so feel free to ignore, but would it make sense to use the slices.DeleteFunc here and instead of this whole block, have something like:
lcmChart.DependsOn = slices.DeleteFunc(lcmChart.DependsOn,
func(d api.HelmChartDependency) bool { return d.Name == systemUpgradeController })There was a problem hiding this comment.
Since this file consists of a single function and is doing essentially a helm chart configuration, would it make sense to merge its logic and its _test.go with the helm.go file? That way we would have all the chart handling logic at a single place and will keep the number of files for this package to a minimum.
2 participants
Ignore soft dependencies, i.e., SUC and cert-manager, of LCM during
elemental3 customize.Also, prioritze charts in core manifest over the one in solution manifest.