.yaml refresher

[consideRatio]

About

The PR's size

Making many separate PRs would have been good for the ability to review them. I had that ambition initially but realized that making many PRs each affecting a lot of files and the same files over and over was hard and I gave up on that, I'm hoping it won't cause too much trouble - sorry in advance!

In this PR

I have aimed make the .yaml consistent in various ways.

• Labels (using helper)
• Selectors (using helper)
• Block indentation (nindent introduced)
• List element indentation
• Whitespace chomp left ({{- }})
• Template function (include replace template)
• Yaml and Helm comments (# vs {{/* */}})
• Passing scope (with merge (dict "key1" "val1") .)
• Utilize helm lint
• Utilize yamllint like kubernetes/charts
• Setup values for yamllint (lint-chart-values.yaml)
• Utilize kubeval
• Linting / Validation tooling (tools/lint.py)
• Update guidelines (CONTRIBUTING.md)
• Consider dynamic .yaml in chartpress
• Consider dynamic .yaml in pod culler
• Consider dynamic .yaml in kubespawner (kubespawner#159, kubespawner#161)
• Test run cluster upgrades
• Merge kubespawner#161 then bump to utilize it.

Related

Issues and PR's

#538 #622 #624 helm#3811 helm#3470 helm#3837 helm#3854 kubespawner#159 kubespawner#161

Documentation

• Helm
• Sprig
• Go text/template

Refactored from PR

• Update Chart.yaml (Chart.yaml - icon + kube & helm versions #627)
• Update values.yaml ([MRG] values.yaml: extraEnv and extraImages #635)
• Name field suffixing (using helper)
• Optional name field prefixing
• Automate static validation (.yaml templates tests #624)

[betatim]

I am a sucker for consistent style, so + 💯 for this! :)

Do you want to split this into (many) smaller PRs, roughly one per bullet point maybe? This might reduce the amount of rebasing and trying to keep up with changes to the charts made by others.

From experience if there isn't a linter nudging people to stick to a particular style, things drift apart over time. So I'd support an effort to add a linter or somehow automate fixing up the style.

[consideRatio]

@betatim thanks for the encouragement! =D

I've mostly considered breaking the PR up to make it easier for code review for others. I'm currently trying to figure out how to do things properly for a small amount of .yaml files, when that is done I'll know better how to proceed I think.

It might become quite easy to make several PR's for various bullet points if I manage to setup some linter to automate various parts for me. I'll look into linter usage in the kubernetes/charts!

[consideRatio]

Research

helm lint and yamllint

The kubernetes/charts repo contained contribution guidelines that suggested the use of helm lint and yamllint in conjunction with a yamllint config.

template and include

Using the include command instead of template simply adds benefits without drawbacks as far as I know. include allows for piping while template doesn't.

Sprig's nindent

Sprig extends Helms go templating, and Sprig's nindent function is available to us in Helm 2.7+. It first adds a new line, then indents. It allows us to create more readable Yaml files.

# with nindent
metadata:
  labels:
    {{- include "jupyterhub.labels" . | nindent 4 }}
    extraLabel: fun

# without nindent
metadata:
  labels:
{{ include "jupyterhub.labels" . | indent 4 }}
    extraLabel: fun

# output
metadata:
  labels:
    component: "continuous-image-puller"
    app: jupyterhub
    chart: jupyterhub-v0.7-dev
    release: RELEASE-NAME
    heritage: Tiller
    extraLabel: fun

Passing merged scopes to templates

We can pass a dictionary to a template with things, but if we do we might loose track of our previous scope. It was possible to use $. to access that, but it led to issues if you had a nested chart which I have. A nice solution was to pass an augmented scope to the template instead.

# Don't miss the . in the end, it is merged into the first dictionary
# The first variable will be written to based on the second, put the . last!
{{- $_ := merge (dict "key1" "value1" "key2" "value2") . -}}
{{ include "jupyterhub.templatename" $_ }}

Template comments

The details matters a lot when it comes to comments within Helm templates, especially if combined with whitespace chomping. In short, you should comment like {{/* or {{- /* and not {{ /* or {{-/*.

# OK
{{/* A helm comment */}}
# Helm template rendering error: unexpected "/" in command
{{ /* Causes error */ }}

# OK
{{- /* A helm comment with whitespace chomping */ -}}
# Helm template rendering error: illegal number syntax: "-"
{{- /* Causes error */ -}}

# OK
# A Yaml comment

The toYaml function

Apparently it comes with a new line that is hard to chomp away as well using {{- tricks. It can be removed with | trimSuffix "\n" but it only messes up somewhat in the yamllinter with the trailing-spaces flag which we could disable in the yamllint config.

• Helm issue #3470
• go-yaml/yaml issue #355
• Helm PR #3837 - I've attempted a fix
  Also, the toYaml functions default behavior when it comes to list indenting is not to do it, this will cause the yamllinter to fail if it is setup with indent-sequences: true

Whitespace chomping

I established a system for whitespace chomping to follow in the .yaml files. I'm of the opinion that it is easier to understand chomping ({{-) backwards/up as compared to chomping ahead (-\}}). If you are to use either or to solve a task, aim for backwards/up chomping perhaps. It will almost always turn out great if you chomp backwards/up and not ahead/down all the time.

1. Always chomp left and not right.

ports:
  {{- if $manualHTTPS }}
  - containerPort: 8443
    name: proxy-https
  {{- end }}

2. Except when no content is available above (beginning of a file), then chomp right as well once before the first content.

{{- $manualHTTPS := (and .Values.proxy.https.enabled (eq .Values.proxy.https.type "manual")) }}
{{- if $manualHTTPS -}}
apiVersion: v1
# ...
{{- end }}

3. And don't chomp if you are inline with other content

data:
  tls.crt: {{ .Values.proxy.https.manual.cert | b64enc }}
  tls.key: {{ .Values.proxy.https.manual.key | b64enc }}

Future stuff - Sprig's ternary

A new feature was added 28 March 2018 to Sprig. It is my understanding that this will only influence Helm on the client side when generating the chart, allowing us to utilize this quickly. It can resolve some challenges discussed here.

[consideRatio]

I'm thinking I should document preliminary code conventions for this repo, but not confident on where it would be suitable to document it.

Within CONTRIBUTING.md? Suggestions?
/cc @betatim @yuvipanda

[choldgraf]

I think it's a good idea! +1 to CONTRIBUTING.md

[yuvipanda]

I like this very much! Left some minor comments. Needs testing to make sure we don't break people upgrading from last released version of the chart.

Thank you very much for doing this work! It's important for the long term health of the project!

[yuvipanda]

I like this addition!

[yuvipanda]

Can you expand a bit more on the A / B /C and what they mean?

[yuvipanda]

We should make sure that upgrading from an older version of the chart continues to work after this change!

[consideRatio]

• I wonder how Helm behaves, I figure the old deployment will be deleted fully and then recreated, but it should not be an issue I think.
• Would you suggest we simply test on some deployment as a way to make sure this works before merging?

[yuvipanda]

Can you expand a bit on what this is doing?

[consideRatio]

Yes check out the text I added to #625 (comment) under "Passing merged scopes to templates", it might sometimes be overkill, but often it is a nice practice.

For example, when we had the daemonset template and passed a dictionary with two keys, one key was called "name" and one was called "top", with this technique, you could pass a scope that merges the name key/value into the current scope/dict and removing the need to utilize .top.Release.Name instead of simply .Release.Name within the helper

[consideRatio]

Thank you for reviewing @yuvipanda I find it really motivating ❤️

[gsemet]

what about using pipenv for that, at least to garentee the reproductibility in ci?

pipenv install yamllint
wget https://github.com/garethr/kubeval/releases/download/0.6.0/kubeval-darwin-amd64.tar.gz
tar xf kubeval-darwin-amd64.tar.gz
cp kubeval $(pipenv --venv)/bin

So that, pipenv run yamllint or pipenv run kubeval will always run, inside the ci or in the user's env.

[consideRatio]

Ah thanks @gsemet for taking time to review! I've a lot to learn regarding execution environments, I'll start by considering this!

My TODO list

• Read up on pipenv and figure out differences with venv and virtualenv.
• Ensure a robust execution environment~

[minrk]

The shebang here is already correct. While individuals are free to use pipenv, on CI since we already have an env, pip install is all that should be used.

[minrk]

@consideRatio I'm excited to get this landed, since it will inevitably conflict with many new PRs and I don't want you to have to spend your whole life rebasing and resolving conflicts! What are the next steps and how can we help get this finished?

[consideRatio]

@minrk thanks for the encouragement! I will try to split out some details related to the kubespawners labels that is keeping me from removing WIP. Then, some pre-merge testing would be great since i influence so much code with this! I test this on my cluster but would appreciate if someone else also tried it.

It might take a week until that though, im on a climbing vacation without easy access to my linux-computer at home where i code.

[willingc]

Beautiful!!! Thanks @consideRatio

[choldgraf]

What's left to do on this PR? Just have a few pairs of eyeballs take a look at it?

[gsemet]

this looks great :)

[minrk]

This is really great! I was able to do a deploy of master and upgrade to this and it all went smoothly (I did have to do --force due to certain changes, but users were running during the upgrade and the user was still running and accessible, and no volumes were disrupted).

My notes:

2018-06-06

Starting from v0.7-f7fa978

helm install --name test jupyterhub/jupyterhub --version=v0.7-f7fa978 -f config.yaml

config.yaml:

hub:
  cookieSecret: "..."
proxy:
  secretToken: "..."
  https:
    hosts:
      - test-k8s.jovyan.org
    letsencrypt:
      contactEmail: my@email.com

Login and start a user, so that we are doing the upgrade with running users.

Upgrade, in zero-to-jupyterhub:

• checkout pull request 625
• edit chartpress.yaml, set imagePrefix to minrk/k8s- to avoid pushing to official repo
• chartpress --push

Run the upgrade:

helm upgrade test /path/to/zero-to-jupyterhub/jupyterhub -f config.yaml

This fails (as expected) because not everything can upgrade with a patch:

Error: UPGRADE FAILED: Deployment.apps "hub" is invalid: spec.template.metadata.labels: Invalid value: map[string]string{"hub.jupyter.org/network-access-singleuser":"true", "heritage":"Tiller", "release":"test", "hub.jupyter.org/network-access-proxy-api":"true", "hub.jupyter.org/network-access-proxy-http":"true", "app":"jupyterhub", "component":"hub", "chart":"jupyterhub-v0.7-4a728c5"}: `selector` does not match template `labels` && Deployment.apps "proxy" is invalid: [spec.template.metadata.labels: Invalid value: map[string]string{"component":"proxy", "heritage":"Tiller", "hub.jupyter.org/network-access-hub":"true", "hub.jupyter.org/network-access-singleuser":"true", "app":"jupyterhub", "chart":"jupyterhub-v0.7-4a728c5", "release":"test"}: `selector` does not match template `labels`, spec.selector: Invalid value: v1.LabelSelector{MatchLabels:map[string]string{"hub.jupyter.org/network-access-singleuser":"true", "name":"proxy", "release":"test", "app":"jupyterhub", "component":"proxy", "heritage":"Tiller", "hub.jupyter.org/network-access-hub":"true"}, MatchExpressions:[]v1.LabelSelectorRequirement(nil)}: field is immutable] && PodDisruptionBudget.policy "hub" is invalid: spec: Forbidden: updates to poddisruptionbudget spec are forbidden. && PodDisruptionBudget.policy "proxy" is invalid: spec: Forbidden: updates to poddisruptionbudget spec are forbidden.

Now, re-run upgrade with --force:

Success!

Verify:

• hub and proxy pods are restarted
• running from before the upgrade users are still running and accessible
• volumes/database are preserved
• yay!

[willingc]

Thanks @consideRatio 🎉 ☀️

[consideRatio]

Wieee! Thank you all for the attention and the help with this PR! Great that you also tested it @minrk !

I'm looking into the chart label issue causing restarts on hub/proxy right now @minrk !

[choldgraf]

super awesome! TBH, given how many things this PR touched, I'm surprised that it only took 2 months!