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

Support Helm Yaml Superset in Helmfile manager #17199

Closed
SebastianGoeb opened this issue Aug 15, 2022 · 7 comments · Fixed by #17210
Closed

Support Helm Yaml Superset in Helmfile manager #17199

SebastianGoeb opened this issue Aug 15, 2022 · 7 comments · Fixed by #17210
Assignees
Labels
manager:helmfile Related to the helmfile package manager priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:in-progress Someone is working on implementation type:bug Bug fix of existing functionality

Comments

@SebastianGoeb
Copy link

What would you like Renovate to be able to do?

It would be great if the Helmfile manager was able to parse all possible helmfile.yamls, however a lot of helm/helmfile templates are not strictly valid yaml afaict, e.g.

releases:
- name: my-release
  chart: my-custom-chart
  version: "1.2.3"
  namespace: {{ env "NAMESPACE" | quote }}

where namespace: {{...}} breaks the yaml parser because it looks like a broken object literal, when in reality templating will get rid of it.

This either locks me out of doing templating in my helmfiles, if I want them to be renovate-friendly, or at the very least forces me to do some unsafe string sanitization:

Not quoting the template string and instead piping to the quote function is common practice, as it is the only way to correctly sanitize all possible strings. For a namespace I can kinda get away with namespace: '{{ env "NAMESPACE" | quote }}', as namespaces are generally alphanumeric, but this is not the case for all properties, especially labels, annotations, and such.

If you have any ideas on how this should be implemented, please tell us here.

Is there maybe a different parser library that can handle these things out-of-the-box? I think Helm itself (which probably faces the same issue) uses ghodss/yaml (go of course, so not helpful here, but maybe worth a look at). Or is this fundamentally unsolvable?

Is this a feature you are interested in implementing yourself?

No

@SebastianGoeb SebastianGoeb added priority-5-triage status:requirements Full requirements are not yet known, so implementation should not be started type:feature Feature (new functionality) labels Aug 15, 2022
@rarkins rarkins added manager:helmfile Related to the helmfile package manager priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others and removed priority-5-triage labels Aug 16, 2022
@rarkins
Copy link
Collaborator

rarkins commented Aug 16, 2022

These files use Go templating. Some possible approaches:

  1. Stop using YAML parsing, instead make our own parser to understand "just enough" of the file to extract dependencies, ignoring templates
  2. Fully strip any line containing {{ and }} before YAML parsing. Maybe some work?
  3. Reverse engineer Go's templating and compile the templates before parsing
  4. Shell out to CLI and run Go templating first before YAML parsing

We want to avoid (4) because we currently have no CLI/shell in our extract phase

@viceice
Copy link
Member

viceice commented Aug 16, 2022

@rarkins We're already removing any templates from yaml before parsing, so it seems like a bug to me

docs = loadAll(extractYaml(content), null, { json: true }) as Doc[];

function extractYaml(content: string): string {
// regex remove go templated ({{ . }}) values
return content.replace(/(^|:)\s*{{.+}}\s*$/gm, '$1');
}

@viceice viceice added the auto:reproduction A minimal reproduction is necessary to proceed label Aug 16, 2022
@github-actions
Copy link
Contributor

Hi there,

Get your issue fixed faster by creating a minimal reproduction. This means a repository dedicated to reproducing this issue with the minimal dependencies and config possible.

Before we start working on your issue we need to know exactly what's causing the current behavior. A minimal reproduction helps us with this.

To get started, please read our guide on creating a minimal reproduction.

We may close the issue if you, or someone else, haven't created a minimal reproduction within two weeks. If you need more time, or are stuck, please ask for help or more time in a comment.

Good luck,

The Renovate team

@SebastianGoeb
Copy link
Author

Ah, then actually it seems I jumped to the wrong conclusion.

So the actual file that is broken in our project has the namespace formulated like this:

releases:
- name: my-release
  chart: my-custom-chart
  version: "1.2.3"
  namespace: "{{ env "NAMESPACE" }}"

which is improper quoting, but does work after templating, despite the nested double quotes. This also shows up as invalid YAML in VSCode.

I patched it like this:

releases:
- name: my-release
  chart: my-custom-chart
  version: "1.2.3"
  namespace: '{{ env "NAMESPACE" }}'

which looks like a proper yaml string to a yaml parser, but turns in to a different yaml string after templating. It's still improper quoting though, however VSCode does not complain about syntax.

When I try proper quoting:

releases:
- name: my-release
  chart: my-custom-chart
  version: "1.2.3"
  namespace: {{ env "NAMESPACE" | quote }}

VSCode again complain about invalid yaml. This led me to believe I would run into the same issue. Turns out, VSCode reads helmfile.yaml as "Language: YAML, JSON schema: helmfile", hence the yaml syntax errors for {{ }}, whereas actual Helm template.yaml files are read as "Language: helm-template", no syntax errors for {{ }}.

@SebastianGoeb
Copy link
Author

So actually, this works:

namespace: {{ ... ""  | quote }}

this works, but is improper quoting:

namespace: '{{ ... "" }}'

and this doesn't work, also improper quoting but does template fine:

namespace: "{{ ... "" }}"

Are there use cases for having property: "{{ template }}"? I feel like ideally, everyone would use {{ | quote }} everywhere, but the reality is that not everyone always does. Maybe it would be a good idea to simplify the regex by removing the (^|:)\s* and \s*$ bits? It just occurred to me that templates with ... # bla line-comments on the right would probably also be broken because of the $. Or would that match too much? I feel like that would probably be fine...

@viceice viceice self-assigned this Aug 16, 2022
@viceice viceice added type:bug Bug fix of existing functionality status:in-progress Someone is working on implementation and removed type:feature Feature (new functionality) auto:reproduction A minimal reproduction is necessary to proceed status:requirements Full requirements are not yet known, so implementation should not be started labels Aug 16, 2022
@viceice
Copy link
Member

viceice commented Aug 16, 2022

that should work for all go templates

@renovate-release
Copy link
Collaborator

🎉 This issue has been resolved in version 32.160.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
manager:helmfile Related to the helmfile package manager priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others status:in-progress Someone is working on implementation type:bug Bug fix of existing functionality
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants