Merge options are declared per patch, but are actually scoped to the entire MR being composed about crossplane HOT 3 OPEN

I'm not super familiar with the code base, please let me know if this analysis is not correct.

I think this raises a few questions:

1. Do we actually want the current behavior?
   a. If so, will we change the interface to make it clear merge options are not patch-scoped, but composed-resource-scoped?
   b. If not, many compositions out there might already depend on this behavior to work correctly, how to tackle this?
2. How can we improve the merge options documentation to illustrate its nuances?
3. How does this affect, if at all, server-side applies?

I'm willing to contribute with whatever is necessary, but at this point I think we need to further discuss what direction we should move to solve this.

from crossplane.

Great details to explain this behavior, how it can be reproduced, and some analysis of the code base - really impressive effort here @LCaparelli, that accelerates folks understanding of the issue!

I don't have the full deep context of the codebase, but I didn't catch anything obviously wrong with your analysis yet. One thing to note is that the behavior to consider the mergeOptions for all patches on the composed resource goes back at least 3 years, if not longer:
https://github.com/crossplane/crossplane/blame/master/internal/controller/apiextensions/composite/merge.go#L75

So this behavior has indeed been present for awhile - I'm anticipating there's a good reason for it, perhaps to avoid conflicting behavior across patches, but I don't have the depth of context to really know why.

That being said, a couple questions for you:

• Have you happened to try this scenario with function-patch-and-transform yet? The mergeOptions in classic patch & transform have been replaced with a policy.toFieldPath approach. I don't fully expect the behavior to differ here, but it may be useful to try.
• Have you considered a design change to your XRD/claim schema? Instead of separate spec.image and spec.additionalContainers fields, would it be possible to just make spec.image an array that can support multiple entries? e.g.:

image:
- name: argoproj/rollouts-demo
  tag: blue
# claim user can optionally add additional entries to this array
- name: container2
  tag: some-image:v0.0.1

from crossplane.

Thanks for taking the time to have a look at this @jbw976!

Have you considered a design change to your XRD/claim schema? Instead of separate spec.image and spec.additionalContainers fields, would it be possible to just make spec.image an array that can support multiple entries? e.g.:

We ended up setting a static limit to the number of additional containers (usually 5 sidecars is already quite a lot), so that we can have patches for specific indexes while still keeping the abstraction level we wanted for the main container (sidecars can be something we don't maintain ourselves, so we want that to be flexible and allow users to set whatever container fields they want).

        # the static patches solve temporary issues regarding containers duplication
      - type: FromCompositeFieldPath
        fromFieldPath: spec.additionalContainers[0]
        toFieldPath: spec.forProvider.manifest.spec.template.spec.containers[1]
      - type: FromCompositeFieldPath
        fromFieldPath: spec.additionalContainers[1]
        toFieldPath: spec.forProvider.manifest.spec.template.spec.containers[2]
      - type: FromCompositeFieldPath
        fromFieldPath: spec.additionalContainers[2]
        toFieldPath: spec.forProvider.manifest.spec.template.spec.containers[3]
      - type: FromCompositeFieldPath
        fromFieldPath: spec.additionalContainers[3]
        toFieldPath: spec.forProvider.manifest.spec.template.spec.containers[4]
      - type: FromCompositeFieldPath
        fromFieldPath: spec.additionalContainers[4]
        toFieldPath: spec.forProvider.manifest.spec.template.spec.containers[5]

Will probably solve this more elegantly with function-go-templating (or similar) so we can iterate indefinetely.

Have you happened to try this scenario with function-patch-and-transform yet? The mergeOptions in classic patch & transform have been replaced with a policy.toFieldPath approach. I don't fully expect the behavior to differ here, but it may be useful to try.

I'll give that a swing (hopefully) soon, still hadn't had time to do it. 😅

Replying right now just to share the workaround we found.

from crossplane.