argo-cd icon indicating copy to clipboard operation
argo-cd copied to clipboard

Support nested objects in merge generator

Open girstenbrei opened this issue 1 year ago • 8 comments

Summary

Currently, the merge generator relies on the mergeKeys list to select which child generators to merge. This works for goTemplate: false with nested keys, but using Golang templates it only works for root keys.

Motivation

Templating functionality in the values object of a generator allows a user to dynamically assemble and format a specific merge key, especially using Golang templates. It decouples the shape and value of fields generated by a generator into a user-definable one. But, without being able to select a key within the values object, a merge can only be performed with the whole values object. Additionally, this is a feature already working with the non-golang-template engine, making this an inconsistency between the two.

This was discovered as part of implementing #10754 .

Proposal

This issue exists for ApplicationSets with goTemplate: true only. This means, the solution could rely on Golang templates to be available. One option therefore would be to enable Golang templating inside the mergeKeys list values.

Example

# […]
goTemplate: true
spec:
  generators:
    - merge:
        mergeKeys:
          - "{{ .values.merge }}"
        generators:
          - clusters:
            values:
              merge: "{{.metadata.labels.availabilityZone}}/{{.metadata.labels.datacenter}}"
          - git:
            repoURL: https://github.com/argoproj/argo-cd.git
            revision: HEAD
            directories:
              - path: "*"
            values:
              merge: "{{ index .path.segments 0 }}/{{ index .path.segments 1 }}"
# […]

First, the child generators would be evaluated, as they would now. This resolves the templating inside the .values.merge fields into concrete values. Then, the merge generator would evaluate each merge key for each child generator separately. In this example, the merge key {{ .values.merge }} would evaluate to the already expanded values for the git and cluster generator. If the values match, the generated elements merge normally.

Pros

  • No additional dependencies, re-use the existing templating functionality
  • No fiddling with a new variant of the "what if my key contains a dot" problem
  • Keeps the existing mergeKeys as the single point to set the merge criteria
  • Integrates into existing documentation (Golang templating already has it's own page)
  • Similar functionality already known to the user

Cons

  • This might break in interesting ways for a user. Golang templates are a pretty huge hammer for this nail and there might be interesting ways to use this which in tern lead to interesting problems. At the same time this is what makes it interesting to me: Just give me as a user the seemingly already present functionality and good parts of Golang templates in this field.

Additional Info

I think this doesn't break any existing merge keys. The behavior only changes, when goTemplate: true is set. In this case, currently, no nested merging is possible, meaning every valid key in mergeKeys must be a top level key in a generator. As there is no generator with top-level keys which are valid Golang templates, there is no valid merge key now in existence which might be a valid Golang template (and therefore evaluate differently after the change).

There might be other variants of doing this. One option would be to use something like jsonpath. But, this would be an additional, different mechanism which looks visually similar to the existing templating functionality, maybe confusing users when to use what. Another option would be to have something like a mergeObject, which contains the structure of what to merge, but not it's values (which would be in the generators). "Walking" this merge object and the generator values would then merge if the values exist. But this would mean implementing the object walking, which would probably be more work than "template this field with this context". Additionally it would introduce another field next to mergeKeys.

Feel free to let me know about any concerns or better ideas you have regarding this proposal. If you could see this as a viable route, I'd be interested in trying to get a PR ready.

girstenbrei avatar Mar 13 '23 12:03 girstenbrei

Just hit this same problem after migrating to Go templates as we wanted to use the default function to guard against missing config in the git files generator. Not being able to use the path.basename with Go templates is breaking things for us so it would be great if we can use this with Go templates in the future.

m13t avatar Apr 18 '23 21:04 m13t

Unless I'm missing something, this is preventing all usage of the merge generator with git generators when go templating is enabled, because there is no top level object that can be used as a merge key (what used to be path is now path.path). https://argo-cd.readthedocs.io/en/stable/operator-manual/applicationset/GoTemplate/#git-generators

amanfredi avatar Jun 30 '23 22:06 amanfredi

Yep. I think this other PR serves as a good basis for fixing the problem: https://github.com/argoproj/argo-cd/pull/13584#issuecomment-1546967445

crenshaw-dev avatar Jun 30 '23 22:06 crenshaw-dev

@crenshaw-dev slightly more complicated for the keys if I remember rightly. Need to find some time to have a proper deep dive and see if I can come up with a working solution.

m13t avatar Jun 30 '23 22:06 m13t

Just wanted to note that I was able to work around this issue and satisfy my use case by reverting to fasttemplate. The behavior I wanted was to infer most of my parameters from the directory structure (git directory generator), but allow overrides on a per-application basis by using values from a config.yaml (git files generator) without requiring all values to be specified in every config.yaml (provide default values).

The generator structure I ended up with to get this to work is:

merge:

  • matrix:
    • git directory
    • list with single element containing default values
  • git files

amanfredi avatar Jun 30 '23 23:06 amanfredi

As mentioned, this prevents any usage of GoTemplate when used in a manifest with merge generator that includes different types of generators. In my case no workaround would work and I had to revert back to simple-template.

Is there any estimation when this may be fixed?

Thanks!

MohammedNoureldin avatar Jan 27 '24 00:01 MohammedNoureldin

Greets everyone, I've mentioned the PR #17145 in Slack but haven't heard back.

If you have some capacity, @crenshaw-dev , maybe you can have a look at it or point me in the right direction towards someone to get some feedback. I'd still like to get this feature, unblocking the whole GoTemplate usage in my AppSet-Generators.

Thanks everyone!

girstenbrei avatar Apr 17 '24 14:04 girstenbrei

I would be interested in this feature to. Are there any updates on this?

Dominic1DL avatar Apr 24 '24 13:04 Dominic1DL