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

feat(appset): Advanced Templating using templatePatch #14893

Merged
merged 5 commits into from
Dec 1, 2023

Conversation

speedfl
Copy link
Contributor

@speedfl speedfl commented Aug 3, 2023

Signed-off-by: gmuselli [email protected]

@crenshaw-dev a small proposal for patchTemplate

Closes #11164
Closes #9177

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Attention: 32 lines in your changes are missing coverage. Please review.

Comparison is base (5c187a1) 49.49% compared to head (47f5743) 49.49%.
Report is 2 commits behind head on master.

Files Patch % Lines
...cationset/controllers/applicationset_controller.go 13.04% 19 Missing and 1 partial ⚠️
applicationset/controllers/templatePatch.go 55.55% 8 Missing and 4 partials ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##           master   #14893   +/-   ##
=======================================
  Coverage   49.49%   49.49%           
=======================================
  Files         269      270    +1     
  Lines       47389    47480   +91     
=======================================
+ Hits        23454    23500   +46     
- Misses      21630    21670   +40     
- Partials     2305     2310    +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@crenshaw-dev
Copy link
Member

For folks watching this issue: I really, really wanted to get it into 2.9 but ran out of time. We did manage to merge this slightly less-powerful tool, which should be sufficient for some use cases: #14743

@speedfl
Copy link
Contributor Author

speedfl commented Oct 3, 2023

@crenshaw-dev #14743 will save a lot of time. Thanks a lot. In parallel I will rebase this one and keep it ready for 2.10

@speedfl speedfl requested a review from a team as a code owner October 4, 2023 12:27
@speedfl speedfl requested a review from a team as a code owner October 4, 2023 17:14
Copy link

@sethgupton-mastery sethgupton-mastery left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with patchTemplate -> templatePatch I'm still having problems getting it working with my test files. But that could be on my end. Will continue on it next week.

docs/operator-manual/applicationset/Template.md Outdated Show resolved Hide resolved
docs/operator-manual/applicationset/Template.md Outdated Show resolved Hide resolved
@speedfl speedfl force-pushed the feature/advanced-templating branch from bc46257 to 39c3ed8 Compare October 23, 2023 12:50
@sethgupton-mastery
Copy link

sethgupton-mastery commented Oct 24, 2023

Hiya, I am having a problem where if goTemplate: true is in there my second generator can't get values from my first.

apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
  name: dev-appset
  namespace: argocd
spec:
  goTemplate: true
  generators:
    - matrix:
        generators:
          - list:
              elements:
                - prefix: default
                  folderName: local
                  projectName: default
          - git:
              repoURL: myUrl
              revision: main
              files:
                - path: "testfiles-argocd/**/{{folderName}}/app.json"

The above works fine without goTemplate: true but with it I get

failed to get params for second generator in the matrix generator: child generator returned an error on parameter generation: failed to replace parameters in generator: failed to parse template testfiles-argocd/**/{{folderName}}/app.json

@speedfl
Copy link
Contributor Author

speedfl commented Oct 24, 2023

@sethgupton-mastery as you are using goTemplate you must use testfiles-argocd/**/{{ .folderName }}/app.json

@sethgupton-mastery
Copy link

Sweet. That fixed me up. I hadn't realized goTemplate was something that already existed and wasn't part of this PR.
With goTemplate set to true and me using goTemplate correctly... drum roll

I was able to load a dynamic list of labels from my app.json file! Very exciting! Can't wait for this to get in.

Thanks speedfl.

@jessebot
Copy link
Contributor

I just read through the docs section and this feature looks really cool and would help out a ton! 🚀 Is this slated for 2.10?

@crenshaw-dev
Copy link
Member

Yep, slated for 2.10. Starting to review in earnest now. :-)

@crenshaw-dev crenshaw-dev changed the title feat(appset): Advanced Templating using patchTemplate feat(appset): Advanced Templating using templatePatch Nov 30, 2023
Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's go 🚀

@crenshaw-dev crenshaw-dev merged commit a08c573 into argoproj:master Dec 1, 2023
25 checks passed
@crenshaw-dev crenshaw-dev deleted the feature/advanced-templating branch December 1, 2023 16:13
@kanor1306 kanor1306 mentioned this pull request Dec 4, 2023
10 tasks
vladfr pushed a commit to vladfr/argo-cd that referenced this pull request Dec 13, 2023
* fix(11164): Advanced templating using patchTemplate

Signed-off-by: gmuselli <[email protected]>

* small changes

Signed-off-by: Michael Crenshaw <[email protected]>

---------

Signed-off-by: gmuselli <[email protected]>
Signed-off-by: Michael Crenshaw <[email protected]>
Co-authored-by: Michael Crenshaw <[email protected]>
tesla59 pushed a commit to tesla59/argo-cd that referenced this pull request Dec 16, 2023
* fix(11164): Advanced templating using patchTemplate

Signed-off-by: gmuselli <[email protected]>

* small changes

Signed-off-by: Michael Crenshaw <[email protected]>

---------

Signed-off-by: gmuselli <[email protected]>
Signed-off-by: Michael Crenshaw <[email protected]>
Co-authored-by: Michael Crenshaw <[email protected]>
@didlawowo
Copy link

i'm trying this feature using this article : https://medium.com/@geoffrey.muselli/argocd-multi-cluster-helm-charts-installation-in-mono-repo-0a406ff7c578

building an image based on 2.10, get for each time get Error from server (BadRequest): error when creating "demo/argo-applicationset-template/applicationset-argo-monocluster.yaml": ApplicationSet in version "v1alpha1" cannot be handled as a ApplicationSet: strict decoding error: unknown field "Generator"

@speedfl
Copy link
Contributor Author

speedfl commented Dec 23, 2023

Can you open a bug ?
Please add as well the ApplicationSet you are trying to create and the logs from the appset controller ? You can tag me. I will try to have a look

@didlawowo
Copy link

Can you open a bug ? Please add as well the ApplicationSet you are trying to create and the logs from the appset controller ? You can tag me. I will try to have a look

made a mistake in my config , sorry

@didlawowo
Copy link

didlawowo commented Dec 23, 2023

it's working for multi cluster, i just make some little adjustement , but enable to use the feature flag enabled.

this is my applicationset, with that, i can get two clone on different cluster

`
apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
name: hello-world-clusters
namespace: kube-infra
spec:
goTemplate: true
generators:

  • matrix:
    generators:
    • clusters: {}

    • git:
      repoURL: [email protected]:didlawowo/pulumi.git
      revision: feat/crossplane-ml
      files:

      • path: 'continuous-delivery/clusters-apps/hello-world/.argocd.json'

template:
metadata:
name: '{{ .name }}-{{ .path.basename }}'
spec:
project: default
sources:
- repoURL: [email protected]:didlawowo/pulumi.git
targetRevision: '{{ dig "clusters" .name "valuesRevision" "feat/crossplane-ml" . }}'
ref: values
- repoURL: '{{ .source.repoURL }}'
targetRevision: '{{ dig "clusters" .name "chartRevision" .source.targetRevision . }}'
chart: '{{ default "" .source.chart }}'
path: '{{ default "" .source.path }}'
helm:
releaseName: '{{ .path.basename }}'
valueFiles:
- $values/{{ .path.path }}/values.yaml
- $values/{{ .path.path }}/values.{{ .name }}.yaml
destination:
server: https://kubernetes.default.svc
namespace: '{{ .destination.namespace }}'
syncPolicy:
automated:
prune: true
syncOptions:
- CreateNamespace={{ dig "syncPolicy" "syncOptions" "CreateNamespace" "true" . }}
`
i have tried many tips, but not working, any help was really usefull

@speedfl
Copy link
Contributor Author

speedfl commented Dec 23, 2023

This is a support question. Could you please ask it on the cncf slack. Section argo-cd-appset. Thanks 🙂

@LS80
Copy link

LS80 commented Jan 8, 2024

Thanks for implementing this.

Was any consideration given to the multiple source use case? i.e. the patching of a source that is a list element under sources? It would be great if it behaved like a Kustomize strategic merge patch and appended the patched source rather than replacing the entire contents of sources. It would be even better if it could merge the list elements on a known key (perhaps ref) so we would only have to patch one field. Then the bulk of the template could be under template as an object rather than a giant string under templatePatch.

An example of a desired ApplicationSet that doesn't currently work:

apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
  name: vpa
  namespace: argocd
spec:
  goTemplate: true
  generators:
    - merge:
        mergeKeys:
          - name
        generators:
          - clusters: {}
          - list:
              elements:
              - name: dev
                kustomizeComponents: 
                  - components/dashboards
  templatePatch: |
    spec:
      sources:
        - ref: custom
          kustomize:
            components: 
            {{- range .kustomizeComponents }}
              - {{ . }}
            {{- end }}
  template:
    metadata:
      name: 'vpa-{{.name}}'
      namespace: argocd
      labels:
        environment: '{{.metadata.labels.environment}}'
    spec:
      sources:
        - repoURL: https://github.com/LS80/vertical-pod-autoscaler.git
          path: manifests/vpa
          targetRevision: '{{ default "main" .targetRevision }}'
          ref: custom
          kustomize:
            commonLabels:
              environment: '{{.metadata.labels.environment}}'
        - chart: vpa
          repoURL: https://charts.fairwinds.com/stable
          targetRevision: 2.1.0
          helm:
            releaseName: vpa
            valuesObject:
              updater:
                enabled: false
      project: default
      destination:
        name: '{{.name}}'
        namespace: kube-system

Here everything under template.spec.sources is lost. If it could work in a similar way to a Kustomize strategic merge patch for a Container in a Pod spec (uses the name as the merge key) then it would allow for a cleaner spec.

@speedfl
Copy link
Contributor Author

speedfl commented Jan 8, 2024

This is a normal behavior but it could be probably improved.
The array in your templatePatch overrides the array in your sources.
The solution for array is to put everything in the templatePatch

@LS80
Copy link

LS80 commented Jan 8, 2024

Yes I am looking for a way to avoid putting everything in a giant string under templatePatch, similar to how Kustomize allows patching of a single field in a Container with a strategic merge patch, using the name as a merge key. How easy would it be to implement a similar merge key for sources?

@speedfl
Copy link
Contributor Author

speedfl commented Jan 8, 2024

We are using a strategic merge patch so that strange

@speedfl
Copy link
Contributor Author

speedfl commented Jan 8, 2024

Could you please open a bug with a minimal case to reproduce. I'll try to have a look

@LS80
Copy link

LS80 commented Jan 9, 2024

I created #16800. Thanks for looking 👍

JulienFuix pushed a commit to JulienFuix/argo-cd that referenced this pull request Feb 6, 2024
* fix(11164): Advanced templating using patchTemplate

Signed-off-by: gmuselli <[email protected]>

* small changes

Signed-off-by: Michael Crenshaw <[email protected]>

---------

Signed-off-by: gmuselli <[email protected]>
Signed-off-by: Michael Crenshaw <[email protected]>
Co-authored-by: Michael Crenshaw <[email protected]>
lyda pushed a commit to lyda/argo-cd that referenced this pull request Mar 28, 2024
* fix(11164): Advanced templating using patchTemplate

Signed-off-by: gmuselli <[email protected]>

* small changes

Signed-off-by: Michael Crenshaw <[email protected]>

---------

Signed-off-by: gmuselli <[email protected]>
Signed-off-by: Michael Crenshaw <[email protected]>
Co-authored-by: Michael Crenshaw <[email protected]>
Signed-off-by: Kevin Lyda <[email protected]>
Hariharasuthan99 pushed a commit to AmadeusITGroup/argo-cd that referenced this pull request Jun 16, 2024
* fix(11164): Advanced templating using patchTemplate

Signed-off-by: gmuselli <[email protected]>

* small changes

Signed-off-by: Michael Crenshaw <[email protected]>

---------

Signed-off-by: gmuselli <[email protected]>
Signed-off-by: Michael Crenshaw <[email protected]>
Co-authored-by: Michael Crenshaw <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants