allenporter / flux-local

flux-local is a set of tools and libraries for managing a local flux gitops repository focused on validation steps to help improve quality of commits, PRs, and general local testing.
https://allenporter.github.io/flux-local/
Apache License 2.0
155 stars 22 forks source link

Change VALUE_PLACEHOLDER to be yaml-safe #620

Closed jfroy closed 7 months ago

jfroy commented 7 months ago

In templates that don't quote, the previous placeholder value **PLACEHOLDER** would be interpreted as a yaml anchor reference and potentially cause templating exceptions.

For example, with the tailscale-operator helm chart, it has this resource:

# Copyright (c) Tailscale Inc & AUTHORS
# SPDX-License-Identifier: BSD-3-Clause

{{ if and .Values.oauth .Values.oauth.clientId -}}
apiVersion: v1
kind: Secret
metadata:
  name: operator-oauth
  namespace: {{ .Release.Namespace }}
stringData:
  client_id: {{ .Values.oauth.clientId }}
  client_secret: {{ .Values.oauth.clientSecret }}
{{- end -}}

Since oauth.clientId and oauth.clientSecret are expected to be secrets, when running with flux-local they would end up with the placeholder value, yielding this exception:

Error: YAML parse error on tailscale-operator/templates/oauth-secret.yaml: error converting YAML to JSON: yaml: unknown anchor 'PLACEHOLDER' referenced
helm.go:84: [debug] error converting YAML to JSON: yaml: unknown anchor 'PLACEHOLDER' referenced
YAML parse error on tailscale-operator/templates/oauth-secret.yaml
helm.sh/helm/v3/pkg/releaseutil.(*manifestFile).sort
        helm.sh/helm/v3/pkg/releaseutil/manifest_sorter.go:146
helm.sh/helm/v3/pkg/releaseutil.SortManifests
        helm.sh/helm/v3/pkg/releaseutil/manifest_sorter.go:106
helm.sh/helm/v3/pkg/action.(*Configuration).renderResources
        helm.sh/helm/v3/pkg/action/action.go:168
helm.sh/helm/v3/pkg/action.(*Install).RunWithContext
        helm.sh/helm/v3/pkg/action/install.go:304
main.runInstall
        helm.sh/helm/v3/cmd/helm/install.go:310
main.newTemplateCmd.func2
        helm.sh/helm/v3/cmd/helm/template.go:95
github.com/spf13/cobra.(*Command).execute
        github.com/spf13/cobra@v1.8.0/command.go:983
github.com/spf13/cobra.(*Command).ExecuteC
        github.com/spf13/cobra@v1.8.0/command.go:1115
github.com/spf13/cobra.(*Command).Execute
        github.com/spf13/cobra@v1.8.0/command.go:1039
main.main
        helm.sh/helm/v3/cmd/helm/helm.go:83
runtime.main
        runtime/proc.go:271
runtime.goexit
        runtime/asm_amd64.s:1695
allenporter commented 7 months ago

Hi, appreciate the PR. The tests are failing.

Even better would be also updating a test that reproduces the original problem, but i'm ok with any placeholder value really.

jfroy commented 7 months ago

Hi, appreciate the PR. The tests are failing.

Even better would be also updating a test that reproduces the original problem, but i'm ok with any placeholder value really.

I forgot to update the tests :cry:

jfroy commented 7 months ago

Even better would be also updating a test that reproduces the original problem, but i'm ok with any placeholder value really.

I am not sure how I would write that test. I'd have to define a chart with a template component to invoke helm template against, or use an existing chart online somewhere (but it seems like a bad idea to pull some external chart for tests).

codecov-commenter commented 7 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 93.49%. Comparing base (0b49728) to head (745632a). Report is 2 commits behind head on main.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #620 +/- ## ======================================= Coverage 93.49% 93.49% ======================================= Files 19 19 Lines 1984 1984 ======================================= Hits 1855 1855 Misses 129 129 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

allenporter commented 7 months ago

Even better would be also updating a test that reproduces the original problem, but i'm ok with any placeholder value really.

I am not sure how I would write that test. I'd have to define a chart with a template component to invoke helm template against, or use an existing chart online somewhere (but it seems like a bad idea to pull some external chart for tests).

Yes, I agree this is in general a bad practice. However, this i show the tests in tests/tool/test_get_hr.py work where each directory under https://github.com/allenporter/flux-local/tree/main/tests/testdata has a test repo in it. A better option is to have a local helm chart with the problem capabilities that also reproduces the issue.

Consider it for a future PR.

allenporter commented 7 months ago

Thanks @jfroy 💯