vidispine / hull

The incredible HULL - Helm Uniform Layer Library - is a Helm library chart to improve Helm chart based workflows
https://github.com/vidispine/hull
Apache License 2.0
213 stars 12 forks source link

helm lint : invalid Yaml document separator: apiVersion #186

Closed matthias4217 closed 1 year ago

matthias4217 commented 1 year ago

I've encountered an error when linting my charts : [ERROR] templates/hull.yaml: unable to parse YAML: invalid Yaml document separator: apiVersion: v1

This happens with helm 3.7+. Helm 3.5 and 3.6 are fine and do not show such an error. Now, generating the template with helm template gives correct results, so this might be a helm issue.


A non-hull helm chart with this basic template succesfullt passes the lint. This is a similar output to helm template with the hull chart. :

templates/test.yaml (there is nothing else in the templates folder) :

apiVersion: v1
data:
  testdata: lorem-ipsum
kind: ConfigMap
metadata:
  name: release-name-test-testconfigmap1
---
apiVersion: v1
data:
  testdata: dolor sic amet
kind: ConfigMap
metadata:
  name: release-name-test-testconfigmap2

However, such a Hull chart fails :

values.yaml :

hull:
  objects:
    configmap:
      testConfigMap:
        data:
          testdata:
            inline: "lorem ipsum dolor"

templates/hull.yaml (there is nothing else in the templates folder) :

{{- include "hull.objects.prepare.all" (dict "HULL_ROOT_KEY" "hull" "ROOT_CONTEXT" $) }}

I don't really understand the cause, and if hull is at fault, or helm itself. I will continue investigating this.

gre9ory commented 1 year ago

Thanks @matthias4217 for reporting this.

First glance it seems to be related to this helm issue. Will investigate this tomorrow but likely the (to my knowledge YAML conforming?) --- seperators seem to be involved in this ...

Anyhow linting should be added to the tests. At the moment they only excessively do helm template and check the rendered results against the expectations which requires all tests to pass to successfuly create a build.

matthias4217 commented 1 year ago

I guess I've got a simple fix for this.

In hull/templates/_objects.tpl, at the very end of the file :

[...]
{{- /*
### Now render the result object instance
*/ -}}
{{- $objectSpec := dict }}
{{- $objectSpec = include "hull.util.merge" (merge (dict "PARENT_CONTEXT" $rootContext "PARENT_TEMPLATE" $parentTemplate "API_VERSION" (default $apiVersion $spec.apiVersion) "API_KIND" (default $apiKind $spec.apiKind) "COMPONENT" $objectKey "SPEC" $spec "DEFAULT_COMPONENT" $defaultSpec "HULL_ROOT_KEY" $hullRootKey "NO_SELECTOR" $noSelector "OBJECT_TYPE" $objectType) (dict "LOCAL_TEMPLATE" (printf "%s" $hullTemplate))) | fromYaml }}
{{- if (gt (len (keys (default dict $objectSpec))) 0) -}}
{{ toYaml $objectSpec }}

---
{{ end -}} # this is where I have removed a dash
{{- end -}}
{{- end -}}
{{- end -}}
{{- end -}}
{{- end -}}
{{- end -}}

Then there is no linting issue, and everything works fine. The only thing troubling me is that I understand why the linting fails, but not why the template or upgrade have no problem.

gre9ory commented 1 year ago

Thank you @matthias4217 , yes that does fix it! I was able to quickly reproduce the problem and verify the fix is working.

By removing the single dash, the beginning of a new objects specification is on a new line and all is good in terms of linting. As to why this is not a problem when templateing or installing - same here, no clue ... but someone also noted the same (inconsistent) behavior here. If I were to guess, the code to render objects appears somewhat different between helm template and helm lint and some compensation for this type of problem takes place when templateing.

Think it makes sense to add strict linting to the test cases so linting errors are catched early on and the test output is cleaner. Unfortunately I had a tendency to use underscores in almost all test object names which is also producing several dozens of linter errors like this:

[WARNING] templates/hull.yaml: object name does not conform to Kubernetes naming requirements: "test-release-hull-test-quota_one": metadata.name: Invalid value: "test-release-hull-test-quota_one": a lowercase RFC 1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

So its gonna take a moment to correct the tests but I think it is worth it. New (linted) HULL versions should drop later today.

gre9ory commented 1 year ago

Hmm, after correcting all obvious linting error in the tests, it now turns out lint has particular issues with Helm versions 3.1.0 - 3.2.x: image

It seems like the lint command does immediately fail on code like this:

{{- if and $spec (or $spec.staticName (index $parent.Values $hullRootKey).config.general.noObjectNamePrefixes) -}}

with:

error calling include: template: hull-test/charts/hull/templates/_metadata_fullname.tpl:22:26: executing "hull.metadata.fullname" at <$spec.staticName>: nil data; no entry for key "staticName"

Later Helm versions seem more tolerant and interpret this as false if staticName is not present in $spec dict. Again weirdly this does not affect templateing which works perfectly fine for the tests and Helm versions 3.1.0 - 3.2.x.

There will be a lot of effort I estimate to workaround all occurences like that in the code even though it would be possible to check key presence before accessing it everywhere. Hence I think I will drop said Helm versions from the new full (including linting) compatibility list for HULL. According to Helm and related Kubernetes version support policy they are outdated anyways so I think there is no need to add effort in fully supporting them at the cost of major rewriting - just to have linting working.

So linting of test cases will thus be done on Helm vesions > 3.3.0 and those versions are now fully supported including linting with HULL. Limited support excluding linting will be given for Helm versions 3.1.0 - 3.2.x since I will keep the templateing tests in place so general functionality is guaranteed (for the time being).

gre9ory commented 1 year ago

Strict linting for all release tests implemented. Done for all Helm versions >= 3.3.0.

Releases: 1.23.14 1.24.9 1.25.8

matthias4217 commented 1 year ago

Thank you very much for your swift action !