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 incorrect indentation of the PodMonitor template in the Helm chart #7190

Conversation

wallrj
Copy link
Member

@wallrj wallrj commented Jul 19, 2024

In #6459 @shlomitubul introduced a PodMonitor resource.
There was a bug in the PodMonitor template where the servicemonitor.endpointAdditionalProperties were being added instead of podmonitor.endpointAdditionalProperties and that bug has been recently fixed by @inteon in #7069 (review)

But there is an additional mistake in the indentation of those values which is only now apparent.

Here's how to recreate the issue:

#value.yaml
prometheus:
  enabled: true
  podmonitor:
    enabled: true
    endpointAdditionalProperties:
      scheme: https
      tlsConfig:
        serverName: cert-manager-metrics
        ca:
          secret:
            name: cert-manager-metrics-ca
            key: "tls.crt"
git fetch origin master
git checkout FETCH_HEAD
make helm-chart
CHART=$(ls -1t _bin/cert-manager-*.tgz | head -n1)
helm template $CHART --values values.yaml --debug
...
Error: YAML parse error on cert-manager/templates/podmonitor.yaml: error converting YAML to JSON: yaml: line 27: did not find expected '-' indicator
helm.go:84: [debug] error converting YAML to JSON: yaml: line 27: did not find expected '-' indicator
YAML parse error on cert-manager/templates/podmonitor.yaml

/kind bug

Fix incorrect indentation of `endpointAdditionalProperties` in the `PodMonitor` template of the Helm chart

Testing

git fetch origin pull/7190/head
git checkout FETCH_HEAD
make helm-chart
CHART=$(ls -1t _bin/cert-manager-*.tgz | head -n1)
helm template $CHART --values values.yaml --debug | grep -A 30 PodMonitor
kind: PodMonitor
metadata:
  name: release-name-cert-manager
  namespace: default
  labels:
    app: cert-manager
    app.kubernetes.io/name: cert-manager
    app.kubernetes.io/instance: release-name
    app.kubernetes.io/component: "controller"
    app.kubernetes.io/version: "v1.15.0-beta.1-103-gc5e95aac633b68"
    app.kubernetes.io/managed-by: Helm
    helm.sh/chart: cert-manager-v1.15.0-beta.1-103-gc5e95aac633b68
    prometheus: default
spec:
  jobLabel: release-name-cert-manager
  selector:
    matchLabels:
      app.kubernetes.io/name: cert-manager
      app.kubernetes.io/instance: release-name
      app.kubernetes.io/component: "controller"
  podMetricsEndpoints:
    - port: http-metrics
      path: /metrics
      interval: 60s
      scrapeTimeout: 30s
      honorLabels: false
      scheme: https
      tlsConfig:
        ca:
          secret:
            key: tls.crt
            name: cert-manager-metrics-ca
        serverName: cert-manager-metrics
---

@cert-manager-prow cert-manager-prow bot added kind/bug Categorizes issue or PR as related to a bug. release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. area/deploy Indicates a PR modifies deployment configuration size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 19, 2024
@@ -45,6 +45,6 @@ spec:
scrapeTimeout: {{ .Values.prometheus.podmonitor.scrapeTimeout }}
honorLabels: {{ .Values.prometheus.podmonitor.honorLabels }}
{{- with .Values.prometheus.podmonitor.endpointAdditionalProperties }}
{{- toYaml . | nindent 4 }}
{{- toYaml . | nindent 6 }}
Copy link
Member

Choose a reason for hiding this comment

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

Good catch.
We should create an "all options enabled" values.yaml file and apply some yaml linting on the result.
Helm tests could be another solution for this problem long-term.

@inteon
Copy link
Member

inteon commented Jul 19, 2024

/cherrypick release-1.15
/lgtm
/approve

@cert-manager-bot
Copy link
Contributor

@inteon: once the present PR merges, I will cherry-pick it on top of release-1.15 in a new PR and assign it to you.

In response to this:

/cherrypick release-1.15
/lgtm
/approve

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@cert-manager-prow cert-manager-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2024
@cert-manager-prow
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@cert-manager-prow cert-manager-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 19, 2024
@cert-manager-prow cert-manager-prow bot merged commit fc198e9 into cert-manager:master Jul 19, 2024
6 checks passed
@wallrj wallrj deleted the fix-podmonitor-template-indentation branch July 19, 2024 09:11
@cert-manager-bot
Copy link
Contributor

@inteon: #7190 failed to apply on top of branch "release-1.15":

Applying: Fix incorrect indentation of the PodMonitor template in the Helm chart
Using index info to reconstruct a base tree...
M	deploy/charts/cert-manager/templates/podmonitor.yaml
Falling back to patching base and 3-way merge...
Auto-merging deploy/charts/cert-manager/templates/podmonitor.yaml
CONFLICT (content): Merge conflict in deploy/charts/cert-manager/templates/podmonitor.yaml
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix incorrect indentation of the PodMonitor template in the Helm chart
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherrypick release-1.15
/lgtm
/approve

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/deploy Indicates a PR modifies deployment configuration dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants