-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Enable Helm values.yaml jsonschema validation #7069
Enable Helm values.yaml jsonschema validation #7069
Conversation
@@ -44,7 +44,7 @@ spec: | |||
interval: {{ .Values.prometheus.podmonitor.interval }} | |||
scrapeTimeout: {{ .Values.prometheus.podmonitor.scrapeTimeout }} | |||
honorLabels: {{ .Values.prometheus.podmonitor.honorLabels }} | |||
{{- with .Values.prometheus.servicemonitor.endpointAdditionalProperties }} | |||
{{- with .Values.prometheus.podmonitor.endpointAdditionalProperties }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
helm-tool found that there was a typo here.
# The namespace that the service monitor should live in, defaults | ||
# to the cert-manager namespace. | ||
# +docs:property | ||
# namespace: cert-manager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These fields were not documented.
7f12dd2
to
960ba47
Compare
Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>
960ba47
to
ad09af8
Compare
@@ -0,0 +1,3 @@ | |||
value missing from templates: crds.enabled | |||
value missing from templates: crds.keep | |||
value missing from templates: acmesolver.image.pullPolicy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usages of crds.enabled
and crds.keep
are only added later in the Helm chart build process.
acmesolver.image.pullPolicy
is a property that we currently do not support, this seems to be a bug in the Helm chart.
@@ -1345,5 +1368,11 @@ startupapicheck: | |||
# apiVersion: v1 | |||
# kind: ConfigMap | |||
# metadata: | |||
# name: '{{ template "cert-manager.name" . }}-extra-configmap' | |||
# name: '{{ template "cert-manager.fullname" . }}-extra-configmap' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We we giving an incorrect example here, "cert-manager.name" is never used to generate a resource name. It is only used to generate the app.kubernetes.io/name
and app
annotations.
The generated values schema looks good, it may be worth cutting an alpha with this so we can be sure we are not gonna be breaking anyones deployment (if they were injecting strange things in order to template later for example) /lgtm |
/approve |
[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 |
Just passing by: well done self-reviewing on this PR, Tim! This is greatly appreciated!! 👍👍👍 |
An alpha pre-release is now available containing the fix for this issue. Please test and feedback if you have time. |
Adds a
values.schema.json
file generated using helm-tool.Also adds the
verify-helm-values
target which checks that the options defined in the template and the values.yaml file match.Kind
/kind feature
Release Note