Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Fix values merge in helm-operator #2292

Merged
merged 1 commit into from
Jul 26, 2019
Merged

Fix values merge in helm-operator #2292

merged 1 commit into from
Jul 26, 2019

Conversation

obiesmans
Copy link
Contributor

@obiesmans obiesmans commented Jul 24, 2019

Fix values merge in helm-operator.

Values merges in helm-operator will overwrite instead of merge a dict that has it's keys
defined in more than two sources (e.g : Values and multiple ValuesFrom).

This happens because when merging,

destMap, isMap := dest[k].(map[string]interface{})
// If the source map has a map for this key, prefer it
if !isMap {
dest[k] = v
continue
}
// If we got to this point, it is a map in both, so merge them
dest[k] = mergeValues(destMap, nextMap)

first iteration makes dest[k] a chartutil.Values and then at the next iteration will see isMap evaluate to False, overwriting instead of merging the two maps.

type Values map[string]interface{}

func main() {
    var v Values
    _, ok := interface{}(v).(map[string]interface{})
    fmt.Print(ok) // ok is false
}

I wrote a unit test that feed Values from a HelmRelease, a Configmap and a
Secret to make sure it fails, then changed mergValues signature to make it pass.

go test -v ./integrations/helm/release/
=== RUN   TestValues
--- FAIL: TestValues (0.00s)
    release_test.go:75:
        	Error Trace:	release_test.go:75
        	Error:      	Expected value not to be nil.
        	Test:       	TestValues
    release_test.go:76:
        	Error Trace:	release_test.go:76
        	Error:      	Expected value not to be nil.
        	Test:       	TestValues
FAIL
FAIL	github.com/weaveworks/flux/integrations/helm/release	0.498s

After the signature change :

go test -v ./integrations/helm/release/
=== RUN   TestValues
--- PASS: TestValues (0.00s)
PASS
ok  	github.com/weaveworks/flux/integrations/helm/release	0.242s

Fixes #1696.

@obiesmans obiesmans changed the title Fix values merge in Helm integration Fix values merge in helm-operator Jul 24, 2019
@hiddeco
Copy link
Member

hiddeco commented Jul 24, 2019

This PR fixes #1696.

@hiddeco hiddeco added the helm label Jul 24, 2019
Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @obiesmans

Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also looks good to me, thanks for getting to the bottom of this @obiesmans. 🌷

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mergeValues doesn't merge values inside of maps
3 participants