@sabre1041 can you please re-review it?

Thanks for jumping on this @mattfarina @sabre1041! 🚀

Interestingly with this one I can get a situation where a null appears in the resulting template

This situation has:

Child values.yaml

resources:
  limits:
    cpu: 1000m
    memory: 2Gi
  requests:
    cpu: 1000m
    memory: 2Gi

Parent values.yaml

resources:
  requests:
    cpu: 1000m
    memory: 2Gi
  limits:
    memory: 2Gi
    cpu: null

CLI passed in values:

  resources:
    requests:
      cpu: 100m
      memory: 6Gi
    limits:
      memory: 32Gi

So in this case the null is being persisted rather than trimmed from the final result - is that desired?

Minimal repro is:

Chart.yaml with

dependencies:
  - name: elasticsearch
    version: 7.16.3
    repository: https://helm.elastic.co

chart values.yaml as

elasticsearch:
  resources:
    limits:
      cpu: null

The null will persist through into the template

@jamoflaw You bring up a good point. Thanks for providing a good example. Here are my thoughts...

1. "is that desired?" I don't know that it is desired. But, I was not trying to build the fully desired system for values handling. That system is built and in wide use. We won't want to break anything. Helm places a high priority on backwards compatibility. This PR makes enough changes I'm almost not comfortable and I will widely share this change, if it lands, to get people testing it.
2. Older versions of Helm (I tested multiple) preserve the null you see in your examples.
3. The goal of this PR was to make null delete a key everywhere consistently and to fix the import-values bugs. Those 2 problems were in the same code. Removing all null values was not part of this (and I didn't see a bug on it).

Before removing all null values we would need a lot of backwards compatibility testing. I think that should be a separate change.

@gjenkins8 @sabre1041 can you please re-review

@mattfarina Have you tested unsetting subchart keys with a parent->child->grandchild setup? I've only started to experiment, so maybe I'm doing something wrong, but I'm not seeing keys being unset:

Parent-to-child unset works: helm template . --set parent.child.key=null --> child.key is null
Parent-to-grandchild unset doesn't work: helm template . --set parent.child.grandchild.key=null --> grandchild.key is the non-null default.

@benjaminjb I can reproduce this, but there's an additional nuance that's even more problematic. This doesn't just not fix the issue, it also introduces a new bug. I've attached a test script below, it:

• sets up a minimal parent, child, and grandchild chart
• configures the child chart values.yaml to override a key in the grandchild chart values.yaml
• configures the parent chart values.yaml to override a key in the child chart values.yaml

for clarity, in this comment I'll be using the words "parent", "child", and "grandchild" in the absolute sense, as the names of the charts, not as relative terms to a prior reference.

• ./test.sh helm template .:
  • ✔️ grandchild chart value is correctly overridden by value from child chart's values.yaml
  • ✔️ child chart value is correctly overridden by the parent chart's values.yaml
• ./test.sh helm template --set child.key=null .:
  • ✔️ grandchild chart value is correctly overridden by the child chart's values.yaml
  • ❌ child chart value is not unset (as @benjaminjb observed)
  • ❗ child chart value is reverted to its own values.yaml file's value, despite being overridden by the parent chart's values.yaml
• ./test.sh helm template --set child.grandchild.key=null:
  • ✔️ child chart value is correctly overridden by value from parent chart's values,yaml
  • ❌ grandchild chart value is not unset (as @benjaminjb observed)
  • ❗ grandchild chart value is reverted to its own values.yaml file's value, despite being overridden by the parent chart's values.yaml

#!/bin/bash
# Usage: ./test.sh <helm command>
set -eou pipefail

HELM="$1"
shift

TESTDIR="${TESTDIR:-"/tmp/helmtest"}"

function chartYaml {
    echo -e "apiVersion: v2\nname: $1\nversion: 0.1.0"
}

function makeChart {
    NAME="$(basename "$1")"
    mkdir -p "$1/templates" "$1/charts"
    touch "$1/templates/placeholder.txt"
    touch "$1/values.yaml"
    chartYaml "$NAME" > "$1/Chart.yaml"
}

PARENT_DIR="$TESTDIR/parent"
CHILD_DIR="$PARENT_DIR/charts/child"
GRANDCHILD_DIR="$CHILD_DIR/charts/grandchild"
rm -rf "$PARENT_DIR"
# Parent Chart Stetup
makeChart "$PARENT_DIR"
echo -e "# Parent Chart's view of values:\n{{ toYaml .Values }}" > "$PARENT_DIR/templates/testcase.yaml"
cat <<EOF > "$PARENT_DIR/values.yaml"
key: "Value for parent chart, from parent chart values.yaml"
child:
  key: "Value for child chart, overridden from parent chart values.yaml"
EOF

# Child Chart Setup
makeChart "$CHILD_DIR"
echo -e "# Child Chart's view of values:\n{{ toYaml .Values }}" > "$CHILD_DIR/templates/testcase.yaml"
cat <<EOF > "$CHILD_DIR/values.yaml"
key: "Value for childChart, from childChart values.yaml"
grandchild:
  key: "Value for grandchild chart, overridden from child chart values.yaml"
EOF

# Granchild Chart Setup
makeChart "$GRANDCHILD_DIR"
echo -e "# Grandchild Chart's view of values:\n{{ toYaml .Values }}" > "$GRANDCHILD_DIR/templates/testcase.yaml"
cat <<EOF > "$GRANDCHILD_DIR/values.yaml"
key: "Value for grandchild chart, from grandchild values.yaml"
EOF

set -x; cd "$PARENT_DIR" && "$HELM" template "$@"

@mattfarina should I move my comment to a new issue? For my use case this is a serious regression

Adding to what @alice-sawatzky wrote:
A work around that was known to me was to explicitly pass a values file: helm template . -f values.yaml
This no longer works as of 3.13.0 and now seems to behave the same as helm template . did pre 3.13

So from my tests:
3.12.3
helm template . : doesn't handle null values correctly
helm template . -f values.yaml : does handle null values correctly

3.13.0
helm template . : does handle null values correctly
helm template . -f values.yaml : doesn't handle null values correctly

The fix introduced together with my colleague @ankitabhopatkar13 in the above referenced #9940 is now reverted and broke our charts. I don't think it's wise to change these rather complex things and add it as an (unreferenced!) footnote in a release.

For reference, this is now (again) broken: https://github.com/ankitabhopatkar13/helm-values-issue. Very curious if I am misinterpreting this.

@mattfarina

I am still seeing this issue in 3.15.2. Setting a value to null has no effect, the subchart default still takes priority even though it shouldn't.