feat: merge new extra resources into existing context

[tenstad]

Description of your changes

Loads the potentially existing context and inserts the new (into) keys in it. Instead of replacing the whole context.

Allows for running the function multiple times, without loosing the results of previous runs.

Was not sure if I should add to an existing test or create a new one. Let me know if I should create a new one instead.

Fixes #92

I have:

• Read and followed Crossplane's contribution process.
• Added or updated unit tests for my change.

[phisco]

This would be a breaking change, what do you think about having it as a policy, spec.context.Policy: Replace (default) or Merge for this behavior?

[tenstad]

This would be a breaking change, what do you think about having it as a policy, spec.context.Policy: Replace (default) or Merge for this behavior?

Good idea! Can then also potentially have a DeepMerge if supporting merging like function-environment-configs does.

If so, should we also make sure it's possible to add an option to merge all resource results, instead of returning a list?

And is policy specifig enough? mergePolicy is also an option. Maybe take some inspiration from *xpv1.MergeOptions?

[tenstad]

Would any of these options make sense here? Could allow for appending new results onto existing keys when there are duplicates? (not in this PR, just trying to plan ahead)

https://github.com/crossplane-contrib/function-patch-and-transform/blob/b12238e354e01df2db426f6347b7850029279773/input/v1beta1/resources_patches.go#L38-L44

[tenstad]

If so, should we also make sure it's possible to add an option to merge all resource results, instead of returning a list?

I think I would put this outside of context, as it relates more to how the resources are gathered than how the data is returned into the context. Meaning we might not need to concider it too much in this PR.

However, if the result of each element in extraResources can be either an object or an array, the context.policy would need to handle that. I think the following could make sense (not in this PR, but trying to thinking ahead).

# initial context
context:
  apiextensions.crossplane.io/environment:
    config:
      # ...
      data:
        labels:
          example.crossplane.io/env-type: prod
    clusters:
      - apiVersion: example.crossplane.io/v1
        kind: Cluster
        metadata:
          name: first
        # ...

# run extra-resources function
context:
  key: apiextensions.crossplane.io/environment
  policy: MergeObjectsAppendArrays # merge into 'config', append to 'clusters'
  # policy: Replace # replace the entire key with the new results (current behaviour)
extraResources:
  - into: config
    kind: EnvironmentConfig
    mode: Merge # return single object
    mergePolicy: MergeObjects # could add policy field if wanting to configure how to handle arrays
    # ...
  - into: clusters
    kind: Cluster
    mode: Array # return list of resources
    # ...

# resulting context
context:
  apiextensions.crossplane.io/environment:
    config:
      # ...
      data:
        labels:
          example.crossplane.io/env-type: prod
          example.crossplane.io/env-source: github.com/org/team # merged in
    clusters:
      - apiVersion: example.crossplane.io/v1
        kind: Cluster
        metadata:
          name: first
        # ...
      - apiVersion: example.crossplane.io/v1 # appended
        kind: Cluster
        metadata:
          name: second
        # ...

Though, if the initital context looked like this, it wouldn't work.

# initial context
context:
  apiextensions.crossplane.io/environment:
    config: # cannot merge object into array, would then replace or fail?
      - labels:
          example.crossplane.io/env-type: prod
    clusters:
      first: # cannot merge array into object, would then replace or fail?
        apiVersion: example.crossplane.io/v1
        kind: Cluster
        # ...

[tenstad]

Suggested change

	// +kubebuilder:validation:Enum=Replace;Merge
	// +kubebuilder:validation:Enum=Replace;MergeObjects

So that it matches patch and trasform?
https://github.com/crossplane-contrib/function-patch-and-transform/blob/b12238e354e01df2db426f6347b7850029279773/input/v1beta1/resources_patches.go#L41C70-L42

And potentially add MergeObjectsAppendArrays in a future PR.

[phisco]

I agree matching function-patch-and-transform behavior and naming makes a lot of sense 👍

[phisco]

mmm wait, maybe not. "Replace" makes sense, as we are saying replace the whole entry in the context. "MergeObjects" gives the impression we are going to deep merge resources in some way, which is not the case. What we want is to "add new keys or append to existing ones", right? so maybe "Add" or "Append" would be more fitting?

[tenstad]

The the type of the value associated with the given context key is map[string][]any, right? E.g. {"clusters": [{"kind": "Cluster", ...}]}. So I think it makes sense to call it merge when combining resource keys (e.g. "cluster") from the existing context value with the new one. The arrays of resources are one level deeper in the structure, meaning they would just be overwritten if a resource key already existed.

MergeObjects
{"foo": [], "clusters": [{"metadata": {"name": "foo"}}]}
{"clusters": [{"metadata": {"name": "bar"}}]}
=> {"foo": [], "clusters": [{"metadata": {"name": "bar"}}]}

And if wanting to combine the existing resource elements with the new ones, MergeObjectsAppendArrays makes sense, as it merges in the new resource keys, and appends to existing arrays whenever resource keys already exists.

MergeObjectsAppendArrays
{"foo": [], "clusters": [{"metadata": {"name": "foo"}}]}
{"clusters": [{"metadata": {"name": "bar"}}]}
=> {"foo": [], "clusters": [{"metadata": {"name": "foo"}}, {"metadata": {"name": "bar"}}]}

Might not be very user friendly though, even though it is technically correct. And it doesn't have to be the same naming as in patch and trasform.

[phisco]

Agree, let's start with Merge and Replace on the context level then

[tenstad]

As now implemented, and not MergeObjects as in patch and transform?