helm-charts icon indicating copy to clipboard operation
helm-charts copied to clipboard

fix: rendering custom config tpl

Open kimxogus opened this issue 1 year ago • 20 comments

There's {{ tpl (toYaml .Values.customConfig) . | indent 4 }}, but we can't actually use tpl becuase there's several points accessing and iterating child objects of .Values.customConfig. So I made them not to access or iterate child objects if customConfig is string.

kimxogus avatar Nov 17 '23 11:11 kimxogus

Can anyone review this?

kimxogus avatar Nov 22 '23 10:11 kimxogus

Hi @kimxogus, thank you for opening the pull request. To help me better understand and review this change, could you please provide an example?

dsmith3197 avatar Dec 01 '23 19:12 dsmith3197

Hi @dsmith3197 , this is a simple example of usage of tpl config. In various environments, you can use only necessary resources in each environments with this config. Without tpl, you have to add every resources because you cannot use conditional statements.

global:
  feature:
    a: true
    b: false

vector:
  customConfig: |
    sources:
      kafka:
        type: kafka
        bootstrap_servers: KAFKA:9092
        decoding:
          codec: json
        group_id: MY_GROUP
        topics:
          - common-topic
          {{- if .Values.global.feature.a }}
          - feature-a-topic
          {{- end }}
          {{- if .Values.global.feature.b }}
          - feature-b-topic
          {{- end }}

kimxogus avatar Dec 04 '23 05:12 kimxogus

@dsmith3197 Is there any progress reviewing this pr?

kimxogus avatar Jan 02 '24 07:01 kimxogus

@dsmith3197 Yes, this could break those container port related templates, but I think adding all those additional values for container ports will make this pr too large

kimxogus avatar Jan 24 '24 04:01 kimxogus

@dsmith3197 Is this ok?

kimxogus avatar Apr 22 '24 08:04 kimxogus

Hi @kimxogus ,

Apologies in the delay of review of this. I'm admittedly having trouble with the whitespace trying to test this out but templating like this should be fine for templating the list value rather than a string, no? I'm looking at https://helm.sh/docs/chart_template_guide/control_structures/ which shows an example of doing it to add a key/value to a map.

jszwedko avatar Apr 22 '24 22:04 jszwedko

@jszwedko I just wanted to fix the broken tpl config. Also, templating in string config let you handle different configs in single template. as I mentioned above. Using list or key-value config, you have to copy and paste your values.yaml everywhere or write very complicated templates and conditions in your helm chart.

kimxogus avatar Apr 23 '24 05:04 kimxogus

@jszwedko I just wanted to fix the broken tpl config.

Ah I see, Focusing in on this bit, can you explain the issue a bit more clearly? I'm still not seeing it. It seems to imply the customConfig option isn't usable, but that isn't the case and so it sounds like there is some specific issue it is causing?

jszwedko avatar Apr 23 '24 13:04 jszwedko

@jszwedko Sorry, I was confused the context as it's 4months old. The original intention was to support string type customConfig because it's the best way to utilize tpl. In current helm templates, is there any way to implement conditional features like below? We are using our own helm chart wrapping vector's helm chart with those types of helm template, and deployed in various environments. I believe this kind of pattern is the most reusable and convenient way to manage various environments with a single template. If not, we have to write different values.yaml and edit in every helm releases(like adding every single topic names and additional configs in each values.yaml), which is not reusable and make us too hard to manage our data pipelines with vector.

global:
  feature:
    a: true
    b: false

vector:
  customConfig: |
    sources:
      kafka:
        type: kafka
        bootstrap_servers: KAFKA:9092
        decoding:
          codec: json
        group_id: MY_GROUP
        topics:
          - common-topic
          {{- if .Values.global.feature.a }}
          - feature-a-topic
          {{- end }}
          {{- if .Values.global.feature.b }}
          - feature-b-topic
          {{- end }}

kimxogus avatar Apr 24 '24 06:04 kimxogus

Thanks @kimxogus ! My understanding is that it should be possible to template the YAML as-is like I mentioned in https://github.com/vectordotdev/helm-charts/pull/346#issuecomment-2071065355 (though admittedly I was still struggling with the formatting). Would that work for you?

jszwedko avatar Apr 25 '24 00:04 jszwedko

E.g. like they show here:

data:
  myvalue: "Hello World"
  drink: {{ .Values.favorite.drink | default "tea" | quote }}
  food: {{ .Values.favorite.food | upper | quote }}
  {{ if eq .Values.favorite.drink "coffee" }}mug: "true"{{ end }}

jszwedko avatar Apr 25 '24 00:04 jszwedko

The code {{ tpl (toYaml .Values.customConfig) . | indent 4 }} converts the yaml to string first, and then runs the tpl function. That means {{ if eq .Values.favorite.drink "coffee" }}mug: "true"{{ end }} won't work in toYaml function before tpl because it's go template format, not valid yaml format.

Helm raised this error with the template. Error: cannot load values.yaml: error converting YAML to JSON: yaml: line ___: did not find expected key

kimxogus avatar Apr 25 '24 03:04 kimxogus

@kimxogus Ah I see. Would it also work to run the tpl function on the string before doing toYaml then?

jszwedko avatar Apr 26 '24 19:04 jszwedko

Yes

kimxogus avatar Apr 27 '24 00:04 kimxogus

Yes

👍 do you mind updating this PR to do that instead, then?

jszwedko avatar Apr 29 '24 14:04 jszwedko

@jszwedko I've just read helm documents. tpl function generates string output, and toYaml function converts key-value map to string in yaml format. tpl can't be run before toYaml. toYaml should be before tpl as here.

kimxogus avatar Apr 30 '24 10:04 kimxogus

Hi! Any updates for this PR? @jszwedko

DingGGu avatar Jul 09 '24 01:07 DingGGu

@jszwedko Hi, toYaml's input is object and tpl consume and produces string, so it's not possible to run tpl before toYaml. Can we merge this?

kimxogus avatar Jul 10 '24 06:07 kimxogus

Hey! Apologies, i still have to circle back to this PR. I'm still not really sure I understand the why change is necessary here but that is likely due to my lack of familiarity with Helm. I'll try to spend more time with this soon.

jszwedko avatar Jul 12 '24 20:07 jszwedko