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(deploy): move '$leading_path' variable definition in eck-operator #8075

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

devzizu
Copy link

@devzizu devzizu commented Sep 30, 2024

Description

This PR fixes an error that is thrown by Helm when templating the eck-operator chart. In this case, consider the following variables file:

config:
  metrics:
    port: "6000"
    secureMode:
      enabled: true
      tls:
        certificateSecret: "my-metrics-cert-secret"
        caSecret: "my-metrics-cert-secret"
        insecureSkipVerify: false

Running helm template with the above values generates the following error:

Error: template: eck-operator/templates/serviceMonitor.yaml:25:49: executing "eck-operator/templates/serviceMonitor.yaml" at <.Values.config.metrics.secureMode.tls.caMountDirectory>: can't evaluate field Values in type string

Use --debug flag to render out invalid YAML

This error occurs because when modifying the scope using with, the reference for .Values, for example, is lost (helm docs).

Also, unit tests pass with make helm-test (using Helm v3.14 and unittest v0.2.8).

PR requirements

@botelastic botelastic bot added the triage label Sep 30, 2024
@devzizu devzizu marked this pull request as ready for review September 30, 2024 15:46
@devzizu devzizu changed the title fix(deploy): move '$leading_path' variable definition in eck-operator… fix(deploy): move '$leading_path' variable definition in eck-operator Sep 30, 2024
@thbkrkr thbkrkr added >bug Something isn't working :helm-charts labels Sep 30, 2024
@botelastic botelastic bot removed the triage label Sep 30, 2024
@thbkrkr
Copy link
Contributor

thbkrkr commented Sep 30, 2024

buildkite test this

@thbkrkr
Copy link
Contributor

thbkrkr commented Sep 30, 2024

Thank you!

@devzizu
Copy link
Author

devzizu commented Sep 30, 2024

@thbkrkr Thanks for the review! Can we backport this to prior versions of the chart as well?

@devzizu devzizu force-pushed the fix/eck-operator-service-monitor branch from 8422ca9 to 11b6572 Compare October 1, 2024 10:37
@thbkrkr
Copy link
Contributor

thbkrkr commented Oct 1, 2024

buildkite test this

@thbkrkr
Copy link
Contributor

thbkrkr commented Oct 1, 2024

@thbkrkr Thanks for the review! Can we backport this to prior versions of the chart as well?

Thank you for the suggestion! We generally release prior versions only for critical bug fixes, so we might not backport this change to earlier versions unless it is deemed necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants