Skip to content
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

fix: rendering custom config tpl #346

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

kimxogus
Copy link

@kimxogus kimxogus commented Nov 17, 2023

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.

@bruceg bruceg requested a review from a team November 17, 2023 18:18
@bruceg bruceg added the bug Something isn't working label Nov 17, 2023
@kimxogus
Copy link
Author

Can anyone review this?

@dsmith3197
Copy link
Contributor

dsmith3197 commented Dec 1, 2023

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

@kimxogus
Copy link
Author

kimxogus commented Dec 4, 2023

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
Copy link
Author

kimxogus commented Jan 2, 2024

@dsmith3197 Is there any progress reviewing this pr?

Copy link
Contributor

@dsmith3197 dsmith3197 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned that the current approach breaks the automatic detection of container ports and the data directory when using templates in the Vector config. Is there a way we can work around this?

@kimxogus
Copy link
Author

@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
Copy link
Author

@dsmith3197 Is this ok?

@jszwedko
Copy link
Member

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.

@kimxogus
Copy link
Author

kimxogus commented Apr 23, 2024

@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.

@jszwedko
Copy link
Member

@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?

@kimxogus
Copy link
Author

kimxogus commented Apr 24, 2024

@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 }}

@jszwedko
Copy link
Member

Thanks @kimxogus ! My understanding is that it should be possible to template the YAML as-is like I mentioned in #346 (comment) (though admittedly I was still struggling with the formatting). Would that work for you?

@jszwedko
Copy link
Member

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 }}

@kimxogus
Copy link
Author

kimxogus commented Apr 25, 2024

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

@jszwedko
Copy link
Member

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

@kimxogus
Copy link
Author

Yes

@jszwedko
Copy link
Member

Yes

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

@kimxogus
Copy link
Author

kimxogus commented Apr 30, 2024

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants