Allow deletion of a previous values file key #2648

scottrigby · 2017-07-06T00:45:45Z

Since #1656 Helm deep-merges multiple values files. However, this also means there is currently no way to delete an existing key that may be incompatible with a newly added one.

This PR addresses option 2 in #1966. My Golang fu is not top notch yet, but that said let me know if this is wrong so I can fix it (PS Thanks to @rabellamy for sending me Go map documentation).

k8s-reviewable · 2017-07-06T00:45:50Z

This change is

thomastaylor312 · 2017-07-06T04:39:06Z

@scottrigby I think that you added the right logic in the wrong place. When I specify foo: "null", I get this:

NAME:   awesome-panther
REVISION: 1
RELEASED: Wed Jul  5 21:30:57 2017
CHART: nginx-0.1.0
USER-SUPPLIED VALUES:
{}

COMPUTED VALUES:
foo: bar
image:
  pullPolicy: IfNotPresent
  repository: nginx
  tag: 1.11.0
index: <h1>Hello</h1> <p>This is a test</p>
nodeSelector: {}
podAnnotations: {}
replicaCount: 1
resources: {}
restartPolicy: Never
service:
  annotations: {}
  clusterIP: ""
  externalIPs: []
  loadBalancerIP: ""
  loadBalancerSourceRanges: []
  nodePort: ""
  port: 8888
  type: ClusterIP
sleepyTime: "10"

The function you modified only is merging user supplied values, not the values.yaml file. That merging is done on the tiller side (confusing I know) right here. So I think if you move the logic to there it will work

seh · 2017-07-06T15:17:31Z

cmd/helm/install.go

+		// If the new value is literally the string "null", remove the value's key.
+		// This allows helm's multiple value sources (value files or --set) to
+		// remove incompatible keys from the previous values.
+		if v == "null" {


Note that "null" is only one of the values that the YAML specification projects to null.
There is also "Null," "NULL," and "~."

seh · 2017-07-06T15:18:29Z

cmd/helm/install.go

@@ -273,6 +273,14 @@ func (i *installCmd) run() error {
 // Merges source and destination map, preferring values from the source map
 func mergeValues(dest map[string]interface{}, src map[string]interface{}) map[string]interface{} {
 	for k, v := range src {
+		// If the new value is literally the string "null", remove the value's key.
+		// This allows helm's multiple value sources (value files or --set) to


s/helm's/Helm's/

s/multiple value sources/various sources of values/

scottrigby · 2017-07-06T16:41:36Z

@thomastaylor312 @seh I just updated this to cover earlier feedback.

Thanks @rabellamy for pairing on Golang syntax.

seh · 2017-07-06T18:17:07Z

I'm trusting @thomastaylor312 that the accommodation is in the right place now.

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

pkg/chartutil/values.go, line 281 at r2 (raw file):

is YAML null

This reads strangely. How about "is the YAML null value?"

pkg/chartutil/values.go, line 286 at r2 (raw file):

		// ref: http://www.yaml.org/spec/1.2/spec.html#id2803362
		switch val {
		case "null":

You can instead write the following:

case "null", "Null", "NULL", "~":

Alternately, use a regular expression.

Comments from Reviewable

seh · 2017-07-06T19:30:58Z

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

pkg/chartutil/values.go, line 286 at r2 (raw file):

Previously, seh (Steven E. Harris) wrote…

You can instead write the following:
case "null", "Null", "NULL", "~":
Alternately, use a regular expression.

I like the regular expression approach because it allows you to write the following:

if stringIsYAMLNull(val) {
  delete(v, key)
  continue
}

The single-case switch looks a little weird to me, but I concede that the regular expression version also either requires or at least could involve a separate line to compile the regular expression pattern. If you don't want to save the compiled pattern, you can use regexp.Match or regexp.MatchString.

Comments from Reviewable

scottrigby · 2017-07-06T19:32:18Z

@seh I'd be OK with adding the regex check. Honestly that does feel cleaner (and more reusable) to me too. I'll ping you on Slack to make sure I'm 100% clear on how and where you'd like organized in the codebase.

scottrigby · 2017-07-06T23:53:24Z

@seh I changed the switch case to a simple in-place version of regexp.MatchString rather than adding a new complied stringIsYAMLNull. Mainly because I'm not sure whether this has any other use value besides here. Either way, I'd love more feedback on this as this as-is… and then if compiling is preferable I can convert to that.

pkg/chartutil/values.go

scottrigby · 2017-07-07T01:10:12Z

@thomastaylor312 OK, I changed to nil comparison. Thanks for checking that this is the case 💯

If this needs a test, can you point me in the right direction of where to look for the existing tests here? Also, could a test be a follow-up PR, or should it be the same one?

scottrigby · 2017-07-07T04:21:24Z

@thomastaylor312 How do these tests look?

thomastaylor312 · 2017-07-07T04:44:55Z

pkg/chartutil/values_test.go

@@ -316,6 +322,8 @@ func TestCoalesceValues(t *testing.T) {
 		expect string
 	}{
 		{"{{.top}}", "yup"},
+		// The keys bottom, right, left, front, and back should not be present.
+		{"{{.side}}", ""},


These look great, we do need to test that all of the other keys are unset. This was a learning experience for me too, but I found out it should look like this: {"{{.bottom}}", "<no value>"},

I saw that, but doesn't that signify the key is present but the value is empty? Is there currently a test method available that checks if a key is not present?

Definitely a good point. We are talking about this now in Slack to get some ideas

thomastaylor312 · 2017-07-07T04:48:39Z

I manually tested this and it is good to go as soon as we have those last unit test cases. Thanks for all the hard work @scottrigby!

scottrigby · 2017-07-07T17:08:17Z

@thomastaylor312 OK I updated the PR with the check to ensure keys are not present in the Values map. I just defined an array of the keys that should have null value properly recognized (and converted to nil by the yaml parser), and looped over them to check that they are not in the map.

Also, thanks for helping me find what type Values is (from /pkg/chartutil/values.go: type Values map[string]interface{}. From the documentation of CoalesceValues it was not clear to me that Values referred to a type declaration).

scottrigby · 2017-07-07T18:21:34Z

Note that the documentation is the only clear example I can think of now, because it's from a PR I have worked on helm/charts#1375, but that PR is not yet merged. However, it clearly explains the functionality, and I tried to make it match the rhetorical style of the rest of the doc (except I added a header for clarity). If there is another, better example, we can use that. But it does feel like an example helps. Thoughts?

thomastaylor312 · 2017-07-07T18:25:17Z

The docs look good to me. Because circle isn't running the unit tests, I am running those locally and validating that they pass

…is nil. - Note that this covers all YAML null syntax options: ref: http://yaml.org/type/null.html - Note that we do a nil comparison because the encoding/yaml package parses YAML properly and any variation of null, Null, NULL, or ~ is converted to nil by the time we get here.

…tax options, but not for empty string

scottrigby · 2017-07-07T18:33:47Z

Rebased against kubernetes/master now.

thomastaylor312 · 2017-07-07T18:44:29Z

Ok, I checked that the tests are passing, so this should be good to go. Thanks for the contribution @scottrigby!

He LGTM'd it in Reviewable

seh

I missed the merge here, but thought it was still worth noting these suggestions.

seh · 2017-07-07T20:32:07Z

pkg/chartutil/values.go

@@ -281,6 +281,13 @@ func coalesceValues(c *chart.Chart, v map[string]interface{}) (map[string]interf
 		if _, ok := v[key]; !ok {
 			// If the key is not in v, copy it from nv.
 			v[key] = val
+		} else if ok && v[key] == nil {


Here, we could have avoided a second map lookup by binding the mapped value to a variable at line 281 above:

if value, ok := v[key]; ! ok { // ... } else if ok && value == nil { // When the YAML ... }

I'd consider then turning the conditional around:

if value, ok := v[key]; ok { if value == nil { // When the YAML ... } else if dest, ok := value.(map[string]interface{}); ok { // If v[key] is a table ... } } else { // If the key is not in v ... }

@seh If you create an issue and assign it to me, I can make these small changes

That became #2663.

seh · 2017-07-07T20:36:00Z

pkg/chartutil/values.go

+			// When the YAML value is null, we remove the value's key.
+			// This allows Helm's various sources of values (value files or --set) to
+			// remove incompatible keys from any previous chart, file, or set values.
+			// ref: http://www.yaml.org/spec/1.2/spec.html#id2803362


I'd prefer to move this reference over into the pertinent test function in file values_test.go (around line 280 there).

seh · 2017-07-07T21:04:03Z

By the way, the go-yaml/yaml package uses a map of strings to resolve special identifiers to YAML tags. Here we find the set that maps to the "null" tag. Note that the empty string is one of the keys, per the specification.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 6, 2017

thomastaylor312 added awaiting review feature size/small labels Jul 6, 2017

thomastaylor312 self-requested a review July 6, 2017 03:46

thomastaylor312 added this to the 2.6.0 - Features milestone Jul 6, 2017

thomastaylor312 added in progress and removed awaiting review labels Jul 6, 2017

seh assigned thomastaylor312 Jul 6, 2017

seh previously requested changes Jul 6, 2017

View reviewed changes

thomastaylor312 suggested changes Jul 7, 2017

View reviewed changes

pkg/chartutil/values.go Outdated Show resolved Hide resolved

thomastaylor312 reviewed Jul 7, 2017

View reviewed changes

scottrigby force-pushed the allow-delete-key branch 2 times, most recently from 7a09ebc to 2918e0c Compare July 7, 2017 17:42

scottrigby added 3 commits July 7, 2017 14:32

Test that the coalesced key is properly removed for all YAML null syn…

6dc31f8

…tax options, but not for empty string

Update documentation for deleting a default key from a values file

8be9bb5

scottrigby force-pushed the allow-delete-key branch from 0b2161c to 8be9bb5 Compare July 7, 2017 18:33

thomastaylor312 approved these changes Jul 7, 2017

View reviewed changes

thomastaylor312 merged commit 97818b1 into helm:master Jul 7, 2017

thomastaylor312 mentioned this pull request Jul 7, 2017

Can't remove keys defined in values.yml #1966

Closed

seh reviewed Jul 7, 2017

View reviewed changes

seh mentioned this pull request Jul 10, 2017

Several map lookups in chart value coalescing procedure can be eliminated #2663

Closed

seh mentioned this pull request Jul 20, 2017

ref(pkg/chartutil): decrease map lookups #2704

Merged

so0k mentioned this pull request May 11, 2018

[vault] drop default file storage banzaicloud/banzai-charts#214

Closed

royjs mentioned this pull request May 25, 2018

fix(pkg/strvals): evaluate "null" values #3912

Merged

aeijdenberg mentioned this pull request Jan 18, 2019

Nested null values don't remove keys as expected #5184

Closed

nuwang mentioned this pull request Sep 7, 2019

Change galaxy-helm chart to work with Galaxy image built with libyaml uwsgi galaxyproject/galaxy-helm#55

Merged

This was referenced Nov 7, 2019

Helm 3 Proposal: Replace null override with a flag #5834

Closed

Using null to delete a key sets it if it was not already set #6900

Closed

chancez mentioned this pull request Jan 13, 2020

Cannot unset default values fluxcd/helm-operator#204

Closed

sumo-drosiek mentioned this pull request Oct 14, 2020

Can't use maxUnavailable in PDB because chart is probably taking both default minAvailable and custom maxUnavailable values SumoLogic/sumologic-kubernetes-collection#1007

Closed

elliotkennedy mentioned this pull request Dec 17, 2020

Nested null values do not remove keys from sub-charts #9136

Closed

muvaf mentioned this pull request Sep 15, 2021

olm: remove runAsGroup and runAsUser from pods upbound/universal-crossplane#185

Merged

3 tasks

enrique-ayala mentioned this pull request Jan 2, 2024

[kube-state-metrics] Can't disable seccompProfile prometheus-community/helm-charts#4123

Closed

Allow deletion of a previous values file key #2648

Allow deletion of a previous values file key #2648

Uh oh!

Conversation

scottrigby commented Jul 6, 2017

Uh oh!

k8s-reviewable commented Jul 6, 2017

Uh oh!

thomastaylor312 commented Jul 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scottrigby commented Jul 6, 2017

Uh oh!

seh commented Jul 6, 2017

Uh oh!

seh commented Jul 6, 2017

Uh oh!

scottrigby commented Jul 6, 2017

Uh oh!

scottrigby commented Jul 6, 2017

Uh oh!

Uh oh!

scottrigby commented Jul 7, 2017

Uh oh!

scottrigby commented Jul 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thomastaylor312 commented Jul 7, 2017

Uh oh!

scottrigby commented Jul 7, 2017

Uh oh!

scottrigby commented Jul 7, 2017

Uh oh!

thomastaylor312 commented Jul 7, 2017

Uh oh!

scottrigby commented Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomastaylor312 commented Jul 7, 2017

Uh oh!

seh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seh commented Jul 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

scottrigby commented Jul 7, 2017 •

edited

Loading

seh commented Jul 7, 2017 •

edited

Loading