chore(chart): add conditional include on optional Deployment schedule…

[senges]

What this PR does / why we need it:

spec.template.spec.schedulerName is an optional Deployment field. By default, it is set to empty string in Values.yaml file which is then treated as nil value from helm.

Consequence is that helm template will generate an invalid YAML with an empty value:

---
# Source: metrics-server/templates/deployment.yaml
apiVersion: apps/v1
kind: Deployment
[...]
spec:
  template:
    spec:
      schedulerName: 
      serviceAccountName: release-name-metrics-server
      priorityClassName: "system-cluster-critical"
[...]

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: senges / name: Charles SENGES (c2b46c4)

[k8s-ci-robot]

This issue is currently awaiting triage.

If metrics-server contributors determine this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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/test-infra repository.

[k8s-ci-robot]

Welcome @senges!

It looks like this is your first PR to kubernetes-sigs/metrics-server 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/metrics-server has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @senges. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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/test-infra repository.

[stevehipwell]

Thanks for the PR @senges. I just checked the history of this change and it's been like this for a long time and the chart has been working correctly. Also AFAIK an empty key is valid YAML syntax, that said I'd prefer to not set a field to empty if there hasn't been a user input. I've added a suggestion to how to implement this to be idiomatic with the existing chart. Could you aso add an entry to the CHANGELOG under the unreleased heading covering your change.

[stevehipwell]

Suggested change

	{{- if .Values.schedulerName }}
	schedulerName: {{ .Values.schedulerName | quote }}
	{{- end }}
	{{- with .Values.schedulerName }}
	schedulerName: {{ . }}
	{{- end }}

This pattern would be more idiomatic for the chart. How come you added the quote operator, I can't see a reason why it'd be needed?

[senges]

Hey @stevehipwell, thanks for the feedback !

Indeed after further readings, empty values seems to be valid in YAML 1.2 spec (see #72-empty-nodes).

But still we both agree it is a bit unnecessary to have this value set with empty value.

Regarding with / if your suggestion seems reasonable. I tend to use with for mapping value more often that's why I did it this way.

quote is just what I believed was best practice, as we know this field must be a string value. Helm can perform some sketchy type juggling from times to times, that's why some other string values are also explicitly quoted in the file, such as deployment.yaml#L47.

I'll update the changelog then !

[senges]

Well I did some updates according to your suggestions, what do you think of the current version ?

[stevehipwell]

I still don't think quote is necessary here, if you look at the chart as a whole we don't add this layer of protection. The general rule is that if a valid value may be interpreted as another type use quote otherwise 50%+ of the chart characters would be quote. If you want to protect against the wrong type being used just add the field to the JSON schema so the Helm wont even attempt to render with the wrong type.

[senges]

TBH I'm a bit surprised, as explicit quoting seems to be quite standard in the Helm ecosystem.

It is recommended in the official Best Practices as well as the Chart Development guideline.

Indeed I see few other examples, maybe not 50% of the chart though.

For example 1234 would be a totally valid name for a Kubernetes namespace, however with current configuration in service.yaml#L5 the chart would render the namespace value as an integer (namespace: 1234 ) which is invalid syntax.

$ helm -n 1234 template ./charts/metrics-server

---
# Source: metrics-server/templates/service.yaml
apiVersion: v1
kind: Service
metadata:
  name: release-name-metrics-server
  namespace: 1234
[...]

$ kubectl create ns 1234
namespace/1234 created

$ helm -n "1234" install test metrics-server/metrics-server
Error: INSTALLATION FAILED: unable to build kubernetes objects from release manifest: unable to decode "": json: cannot unmarshal number into Go struct field ObjectMeta.metadata.namespace of type string

If you have a look at bitnami's charts (ie. grafana) they add explicit quoting for that very reason.

$ helm -n 1234 template oci://registry-1.docker.io/bitnamicharts/grafana

Pulled: registry-1.docker.io/bitnamicharts/grafana:10.0.9
Digest: sha256:1fd037e9e038230968457f85fb0f19e48c83e0c0f84f8b1fde6398f9372dd689
---
# Source: grafana/templates/networkpolicy.yaml
kind: NetworkPolicy
apiVersion: networking.k8s.io/v1
metadata:
  name: release-name-grafana
  namespace: "1234"
[...]

For the exact same reason, --set schedulerName="1234" would fail where 1234 is a totally valid name for a scheduler. You would have to explicitly use --set-string then.

That is not a big issue though, I just wanted to share my surprise and maybe get your point of view regarding this. Could be the topic of a future PR.

[senges]

Note I did push without quote though

[stevehipwell]

If you want to define chart value types that's what the schema is for, adding extra quote calls adds template complexity and makes the resulting output more complex. So for your example you can use the schema to make sure that "1234" is used instead of 1234.

[stevehipwell]

Thanks @senges.

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: senges, stevehipwell

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:

• charts/OWNERS [stevehipwell]

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

[stevehipwell]

/ok-to-test