dict merge doesn't create a deep copy about sprig HOT 12 CLOSED

@msonnabaum
This does not work for nested dictionaries/lists. New dictionary retains references to the children of the source dictionary.
Both merge and mergeOverwrite are perfect foot guns.

from sprig.

I also meet this problem. If we use merge in a range loop, it will be annoying.

from sprig.

Just got bit by this as well. I definitely did not expect merge to mutate the first argument.

from sprig.

@multi-io The workaround I've used is to always pass dict as the first argument to merge. The semantics appear similar to javascript's Object.assign.

The docs hint at this behavior by calling the first argument $dest, but it could be much clearer.

from sprig.

For example, this code unintentionally modifies $src2:

{{- $src1 := dict "a" "b" -}}	
{{- $src2 := dict "foo" (dict "c" "d") -}}
{{- $src3 := dict "foo" (dict "e" "f") -}}
{{- $dest := merge dict $src1 $src2 $src3 }}

Go playground: https://play.golang.org/p/Kx5z_8Z7uHH

from sprig.

Having the same issue currently, and it seems there's no workaround in a Helm context (since deepCopy() is also not available there). This is one more proof that mutable state is evil.

from sprig.

I agree with InvictusMB. This is a royal pain in the... feet. I'm trying to merge helm values for a set of resources, and keep accumulating values inside of replacing them.

from sprig.

Does this comment indicate that changing the merge behavior would break existing helm charts?
helm/helm#3486 (comment)

from sprig.

No, I don't think so. That one is about merging vs overriding behavior. This thread is about taking parameters by value rather than by reference and not introducing unexpected side effects.

from sprig.

My current thinking is that we need a deepCopy function (see #149). This way people who rely on the current behavior can have that while people who want a deep copy can have that as well.

from sprig.

We recently added a deepCopy function and with the merger of #194 the docs now cover deep copying with merging.

While this is targeted at the upcoming v3 release it will also be cherry picked into a v2 release this week.

The reason we didn't alter the merge behavior is because...

1. merging has a fair amount of use in production environments. We are going to be cautious on changing behavior like this.
2. the template functions tend to follow the patterns of the underlying functions that implement the logic. In this case the underlying function does not perform a deep copy.

I understand that some want the merge behavior changed while others do not. There have been a few threads on this. There is no consensus. So, we are taking the conservative approach for now.

from sprig.

@mattfarina
I am OK with preserving the mutation of the first parameter which is a destination dictionary. But I still insist that all the source dictionaries must be guarded by deepCopy.
The current behavior introduces unexpected mutations that depend on the shape and order of objects and the bugs it makes possible are extremely hard to track down.
Moreover, I cannot think of any code that will depend on that behavior intentionally.

Troubleshooting things like #120 (comment) is no fun at all. It happened to me on a project with a huge spread out Helm configuration and challenged my sanity a lot. Only because I added another nested dictionary into one chart that depended on a piece of a shared boilerplate, a completely unrelated chart stopped working.

from sprig.