Misleading statements in mergeOptions

[fernandezcuesta]

URL

https://docs.crossplane.io/latest/concepts/patch-and-transform/#merge-options

What's Wrong?

According to the documentation:

With an object, use keepMapValues: true to leave existing object keys in tact. The patch updates any matching keys between the input and destination data.

The second sentence suggests that a patch changing existing keys (or updating them!) will update them, while apparently that's not the case as discussed with Upbound support.

Given this param for a composition:

spec:
  params:
    clusterName: main
    tags:
      component: cluster
      country: us
      foo: baz

An MR containing:

spec:
  forProvider:
    tags:
      foo: bar

and being applied a patch like:

    - type: FromCompositeFieldPath
      fromFieldPath: spec.parameters.clusterName
      toFieldPath: spec.forProvider.tags.cluster-name
    - type: FromCompositeFieldPath
      fromFieldPath: spec.parameters.tags
      toFieldPath: spec.forProvider.tags
      policy:
        mergeOptions:
          keepMapValues: true

will update the tags for the MR and get:

tags:
  cluster-name: main
  component: eks
  country: us
  foo: bar

which is correct, according to documentation.

However updating the parameters to:

spec:
  params:
    clusterName: main
    tags:
      component: network # updated
      country: es # updated
      foo: baz

does not reflect into any changes in the MR, while according to the documentation:

[...] The patch updates any matching keys between the input and destination data.

and the following was expected in the MR:

tags:
  cluster-name: main
  component: network
  country: es
  foo: bar

[ytsarev]

@plumbis it looks like we need to extend the documentation exposing the information from the associated merge tests

• https://github.com/crossplane/crossplane/blob/master/internal/controller/apiextensions/composite/merge_test.go#L118-L119

Default behavior if no merge options are supplied is to replace dst with src

• https://github.com/crossplane/crossplane/blob/master/internal/controller/apiextensions/composite/merge_test.go#L139-L140

When KeepMapValues is set but AppendSlice is not, dst should preserve its values at the merge path

• https://github.com/crossplane/crossplane/blob/master/internal/controller/apiextensions/composite/merge_test.go#L162-L163

When both KeepMapValues and AppendSlice are ser, dst should preserve map values but arrays being appended

[plumbis]

@ytsarev it sounds like you can't update any existing keys, on add new ones, right?

If the user wants different values for the MR they would have to delete and recreate it, is that correct?

[ytsarev]

@plumbis it's more like KeepMapValues is set but AppendSlice is not the need of mixing both KeepMapValues and KeepMapValues is not clear to the users