update optional labels

[jiaquan1]

Update the PR after revert #173 (review)

[triarius]

Given that the default in the JSON schema is {}, I think we should do that here as well.

Suggested change

	labels:
	user: "user"
	labels: {}

Also, this does not fit the JSON schema you've specified. Attempting to deploy it will give:

Error: UPGRADE FAILED: values don't meet the specifications of the schema(s) in the following chart(s):
agent-stack-k8s:
- labels.user: Invalid type. Expected: object, given: string

But as per my comment above, I don't think the JSON schema is valid for Kubernetes.

[triarius]

Are these valid labels? My understanding is that labels are key-value pairs where both the key and value are strings. But here it seems you're asking for the values to be objects.

I tried to deploy this from your branch with

labels:
  user:
    user: "name"
  project:
    project: "project"

which is the example given and passes the JSON schema, but Kubernetes does not accept it with the error message:

json: cannot unmarshal object into Go struct field ObjectMeta.spec.template.metadata.labels of type string

I think it needs to be something like:

Suggested change

	"additionalProperties": {
	"type": "object",
	"default": {}
	},
	"example": [
	{
	"user": {
	"user": "name"
	},
	"project": {
	"project": "project"
	}
	}
	]
	"additionalProperties": {
	"type": "string",
	"default": ""
	},
	"example": [
	{
	"user": "name",
	"project": "project"
	}
	]

[triarius]

If a user sets

labels:
  app: foo

then the selector will no longer match the label. I think this is a footgun we should not allow.

I recommend you merge app: {{ .Release.Name }} back into $.Values.labels to prevent this, and document that the app label will be overwritten if it is specified by the user.

To implement this, you most likely will need to create a _helpers.tpl file and make some helpers in it.

Have a look at what the helm chart that is generated when you run

helm create agent-stack-k8s

does.

[triarius]

Thanks for working on this @jiaquan1, we're getting there, but I have some more suggestions.

[triarius]

Suggested change

	{{- define "mychart.labels" }}
	{{- define "agent-stack-k8s.labels" }}

Let's give a more meaningful name to the helpers

[triarius]

It's still going to be the case that if the user specifies, e.g., --set labels.app=orcas that the labels don't match the selectors. This is the error you get when you try to upgrade an existing release

Error: UPGRADE FAILED: cannot patch "agent-stack-k8s" with kind Deployment: Deployment.apps "agent-stack-k8s" is invalid: spec.template.metadata.labels: Invalid value: map[string]string{"app":"orcas"}: `selector` does not match template `labels`

And this is what you get when you try to install a new release:

Release "agent-stack-k8s" does not exist. Installing it now.
Error: 1 error occurred:
	* Deployment.apps "agent-stack-k8s" is invalid: spec.template.metadata.labels: Invalid value: map[string]string{"app":"orcas"}: `selector` does not match template `labels`

There are few ways to approach this.

You can simply reverse the order of the app: ... and {{- toYaml ... lines. This will give the later one preference. You don't need a separate _heplers.tpl file to do this. But this approach also has a footgun as it silently overwrites the app value that the user supplied. Furthermore, it leads to the output of helm template containing duplicate keys, which is technically invalid yaml. Some users might process the output of helm template with yaml parsers that would reject it for this reason, so I don't think this is acceptable for us in the long term.

A better approach is to use helper functions to detect when the user is specifying an app label and return an error to the user letting them know that app is reserved. That's where helper functions come in. You should be able to use something like this: https://masterminds.github.io/sprig/flow_control.html to achieve this.

[triarius]

Also, typically, you want there to be no indentation in the helper function so that all the indentation is managed in the template with nIndent.

[triarius]

I think include is what's needed here

Suggested change

	{{- template "mychart.labels" }}
	{{- include "mychart.labels" }}

[triarius]

Thanks for working with us on the feedback @jiaquan1. This looks really close!

[triarius]

Suggested change

	labels:
	{{- toYaml $.Values.labels }}
	app: {{ .Release.Name }}
	{{- toYaml $.Values.labels }}
	app: {{ .Release.Name }}

[triarius]

Otherwise, labels appears twice and there will be another error:

Error: YAML parse error on agent-stack-k8s/templates/deployment.yaml.tpl: error converting YAML to JSON: yaml: line 14: mapping values are not allowed in this context

This is what the template renders as:

# Source: agent-stack-k8s/templates/deployment.yaml.tpl
apiVersion: apps/v1
kind: Deployment
metadata:
  name: agent-stack-k8s-clusters
  namespace: buildkite-clusters
spec:
  selector:
    matchLabels:
      app: agent-stack-k8s-clusters
  template:
    metadata:
      labels:
          labels:user: user
            app: agent-stack-k8s

if you run it with --set labels.user=user. Notice that the indentation is also not consistent. I will repeat my recommendation that you DO NOT do any indentation in the helper, and use nindent (not indent) in the yaml file to control the indentation.

I recommend you attempt to render the template with help template --debug during your development, so avoid having to go back and forth like this.

[jiaquan1]

Thank you. I am not very familiar with helm cmd. help template --debug command did not work in my git repo or I missed some pre-requisition step

help template --debug 
bash: help: no help topics match `--debug'.  Try `help help' or `man -k --debug' or `info --debug'

[triarius]

Sorry, that was a typo, its helm, not help.

[triarius]

We did it! 🎉

[jiaquan1]

@triarius maybe main...jiaquan1:agent-stack-k8s:jiaquan1/add_optional_labels is needed to fix the error https://buildkite.com/buildkite-kubernetes-stack/kubernetes-agent-stack/builds/745