mykso / myks

🧙‍♂️
MIT License
10 stars 3 forks source link

feat: per-chart helm configuration #310

Closed Zebradil closed 2 months ago

Zebradil commented 2 months ago

This PR adds per-chart Helm configuration, resolving #261.

There is a new helm.charts subsection in the configuration schema:

helm:
  #! ...

  #! Per-chart configuration. Values override the global configuration.
  #! The `name` field is used to match the chart in the `charts` directory.
  #! The list is used instead of a map due to a limitation in ytt schema spec.
  #! See https://github.com/carvel-dev/ytt/issues/656 for more information.
  #@schema/validation ("chart names must be unique", lambda x: len(set([c["name"] for c in x])) == len(x))
  charts:
    - buildDependencies: false
      includeCRDs: false
      #@schema/validation min_len=1
      name: ""
      namespace: ""
      releaseName: ""

It's a list of per-chart overrides of the common options under the helm key in the config. Selecting the chart-specific configuration is done via matching the name field with the chart's base name. For example, if a chart's path defined in the vendir config is charts/CHART_NAME, then CHART_NAME is the name of the chart and is used for selecting the options overrides.

fritzduchardt commented 2 months ago

Hi @Zebradil , while testing this, I conceived an app-data.ytt.yaml that would currently fail:

#@data/values-schema
#@overlay/match-child-defaults missing_ok=True
---
helm:
  namespace: monitoring
  charts:
    - buildDependencies: true
      name: "kube-prometheus-stack"
    - buildDependencies: false
      name: "kube-prometheus-stack-exporter"
application:
  namespace:
    create: true

It would lead to the following error:

Invalid schema - wrong number of items in array definition
==========================================================

data-schema.ytt.yaml:
      |
  108 |   charts:
      |

      = found: 2 array items
      = expected: exactly 1 array item, of the desired type

Is this a case you have considered?

Zebradil commented 2 months ago

Hi @fritzduchardt, thanks for testing!

It is a bug, and a new test case. I'll implement it shortly.

Zebradil commented 2 months ago

@fritzduchardt it turns out that it's not a bug, but a limitation of ytt. You can't define default values for arrays like that in a ytt schema file (#@data/values-schema). You have to either use a data file (#@data/values) or use annotations in the schema file:

#@ def charts():
- name: render-test-1
  releaseName: overriden-release-name-1
- name: render-test-2
  releaseName: overriden-release-name-2
#@ end

#@data/values-schema
---
helm:
  #@schema/default charts()
  #@overlay/replace
  #@schema/validation ("chart names must be unique", lambda x: len(set([c["name"] for c in x])) == len(x))
  charts:
    - buildDependencies: false
      includeCRDs: false
      #@schema/validation min_len=1
      name: ""
      namespace: ""
      releaseName: ""

The latter I find very inconvenient, because you have to copy the original array item definition, as well as the validation annotations.

fritzduchardt commented 2 months ago

The latter I find very inconvenient, because you have to copy the original array item definition, as well as the validation annotations.

Nice workaround! Since you also do validation in config_helm.go, I guess the validations are not strictly required.

Zebradil commented 2 months ago

Since you also do validation in config_helm.go, I guess the validations are not strictly required.

That's right. However, ytt validations give better error message, in my opinion. But of course, it is completely up to the user :-)