Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ApplicationSet templatePatch list merge with template #16800

Open
3 tasks done
LS80 opened this issue Jan 9, 2024 · 3 comments
Open
3 tasks done

ApplicationSet templatePatch list merge with template #16800

LS80 opened this issue Jan 9, 2024 · 3 comments
Labels
bug Something isn't working component:application-sets Bulk application management related version:EOL Latest confirmed affected version has reached EOL

Comments

@LS80
Copy link

LS80 commented Jan 9, 2024

Checklist:

  • I've searched in the docs and FAQ for my answer: https://bit.ly/argocd-faq.
  • I've included steps to reproduce the bug.
  • I've pasted the output of argocd version.

Describe the bug

The new ApplicationSet templatePatch introduced in #14893 does not merge lists from template. It will override a list field entirely. An example is when using sources to list multiple sources. Consider the following example which produces the error

InvalidSpecError: spec.source.repoURL and either source.path, source.chart, or source.ref are required for source...

To Reproduce

apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
  name: guestbook
  namespace: argocd
spec:
  goTemplate: true
  generators:
    - list:
        elements:
        - cluster: ops-dev
          kustomizeComponents:
            - components/pod-disruption-budget
  templatePatch: |
    spec:
      sources:
        - repoURL: https://github.com/LS80/kustomize-examples.git
          kustomize:
            components:
            {{- range .kustomizeComponents }}
              - {{ . }}
            {{- end }}
  template:
    metadata:
      name: 'guestbook-{{.cluster}}'
      namespace: argocd
    spec:
      sources:
        - repoURL: https://github.com/argoproj/argocd-example-apps.git
          path: helm-guestbook
          helm:
            valuesObject:
              nameOverride: guestbook
              replicaCount: 2
        - repoURL: https://github.com/LS80/kustomize-examples.git
          path: guestbook
          kustomize:
            commonLabels:
              app: guestbook
      project: default
      destination:
        name: '{{.cluster}}'
        namespace: default

Putting all sources in templatePatch does work, but is not ideal as it could be a very large string that doesn't work as nicely in IDEs as an object would. Here is the equivalent example that works:

apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
  name: guestbook
  namespace: argocd
spec:
  goTemplate: true
  generators:
    - list:
        elements:
        - cluster: ops-dev
          kustomizeComponents:
            - components/pod-disruption-budget
  templatePatch: |
    spec:
      sources:
        - repoURL: https://github.com/argoproj/argocd-example-apps.git
          path: helm-guestbook
          helm:
            valuesObject:
              nameOverride: guestbook
              replicaCount: 2
        - repoURL: https://github.com/LS80/kustomize-examples.git
          path: guestbook
          kustomize:
            components:
            {{- range .kustomizeComponents }}
              - {{ . }}
            {{- end }}
            commonLabels:
              app: guestbook
  template:
    metadata:
      name: 'guestbook-{{.cluster}}'
      namespace: argocd
    spec:
      project: default
      destination:
        name: '{{.cluster}}'
        namespace: default

Expected behavior

The sources in the templatePatch will be deep merged with the sources in template, using repoURL or perhaps ref as a merge key.

That might not be feasible because while repoURL is required, it is not necessarily unique. ref is unique but not required. Perhaps ref could be required in the case where a source merge is desired.

This behaviour would be similar to how a Kustomize strategic merge patch patches Container fields within a Pod spec, using the Container name as a unique merge key.

If not then at least additional sources in templatePatch could be appended to the existing list of sources in template?

Version

argocd: v2.9.3+6eba5be.dirty
  BuildDate: 2023-12-02T00:37:14Z
  GitCommit: 6eba5be864b7e031871ed7698f5233336dfe75c7
  GitTreeState: dirty
  GoVersion: go1.21.4
  Compiler: gc
  Platform: darwin/arm64
argocd-server: v2.10.0-rc1+d919606
  BuildDate: 2023-12-18T20:45:12Z
  GitCommit: d9196060c2d8ea3eadafe278900e776760c5fcc6
  GitTreeState: clean
  GoVersion: go1.21.3
  Compiler: gc
  Platform: linux/arm64
  Kustomize Version: v5.2.1 2023-10-19T20:13:51Z
  Helm Version: v3.13.2+g2a2fb3b
  Kubectl Version: v0.26.11
  Jsonnet Version: v0.20.0
@LS80 LS80 added the bug Something isn't working label Jan 9, 2024
@crenshaw-dev
Copy link
Member

Yeah templatePatch does use strategicMergePatch under the hood, but it's not actually super helpful since the sources array doesn't specify merge keys. I do think that ref is a good candidate to be a merge key, but that's maybe a bigger API change than we have an appetite for without bumping the Application apiVersion.

@joebowbeer
Copy link
Contributor

joebowbeer commented Apr 11, 2024

I suspect ref would need to be a required field in order to use it as a merge key.

But currently ref cannot be used for non-git sources.

More discussion: #17731

@reggie-k reggie-k added the component:application-sets Bulk application management related label Oct 1, 2024
@andrii-korotkov-verkada
Copy link
Contributor

ArgoCD versions 2.10 and below have reached EOL. Can you upgrade and let us know if the issue is still present, please?

@andrii-korotkov-verkada andrii-korotkov-verkada added the version:EOL Latest confirmed affected version has reached EOL label Nov 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working component:application-sets Bulk application management related version:EOL Latest confirmed affected version has reached EOL
Projects
None yet
Development

No branches or pull requests

5 participants