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 for ApplicationSet #11567

Closed
wants to merge 28 commits into from

Conversation

speedfl
Copy link
Contributor

@speedfl speedfl commented Dec 5, 2022

Signed-off-by: gmuselli geoffrey.muselli@gmail.com

@crenshaw-dev I think you were interested by this one

Closes #11164
Closes #9177

Main purpose is to have an ApplicationSet Template as map[string]interface{} which can be fully templatable

apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
  name: guestbook
spec:
  goTemplate: true
  generators:
  - list:
      elements:
      - cluster: engineering-dev
        url: https://kubernetes.default.svc
        automated: true
        prune: true
      - cluster: engineering-prod
        url: https://kubernetes.default.svc
        automated: true
        prune: false
      - cluster: engineering-debug
        url: https://kubernetes.default.svc
        automated: false
        prune: false
  template:
    metadata:
      name: '{{.cluster}}'
    spec:
      project: default
      source:
        repoURL: https://github.com/argoproj/argo-cd.git
        targetRevision: HEAD
        path: applicationset/examples/list-generator/guestbook/{{.cluster}}
      destination:
        server: '{{.url}}'
        namespace: guestbook
      syncPolicy:
        # If automated == true, it will generate a key 'automated' which is part of the Application Spec model. It will then be retained
        # If automated == false, it will generate a key 'noAuto' which is not part of the Application Spec model. It will then be ignored
        '{{ ternary "automated" "noAuto" .automated }}':
          # If prune == true, it will generate a key 'prune' which is part of the Application Spec model. It will then be retained
          # If prune == false, it will generate a key 'noprune' which is not part of the Application Spec model. It will then be ignored
          '{{ ternary "prune" "noprune" .prune }}': true

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 Dec 5, 2022

Codecov Report

Patch coverage: 40.40% and project coverage change: -0.16 ⚠️

Comparison is base (a695aa8) 49.15% compared to head (e94488e) 49.00%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #11567      +/-   ##
==========================================
- Coverage   49.15%   49.00%   -0.16%     
==========================================
  Files         248      248              
  Lines       42891    43002     +111     
==========================================
- Hits        21082    21071      -11     
- Misses      19691    19807     +116     
- Partials     2118     2124       +6     
Impacted Files Coverage Δ
applicationset/generators/duck_type.go 70.18% <0.00%> (ø)
applicationset/generators/pull_request.go 52.23% <0.00%> (ø)
applicationset/generators/scm_provider.go 34.19% <0.00%> (ø)
.../apis/application/v1alpha1/applicationset_types.go 29.26% <ø> (+0.69%) ⬆️
util/argo/argo.go 64.40% <0.00%> (-2.06%) ⬇️
cmd/argocd/commands/applicationset.go 18.25% <33.66%> (+1.19%) ⬆️
...licationset/generators/generator_spec_processor.go 64.00% <60.00%> (-2.30%) ⬇️
applicationset/utils/utils.go 61.99% <60.60%> (-13.60%) ⬇️
...cationset/controllers/applicationset_controller.go 63.15% <100.00%> (-0.29%) ⬇️
applicationset/generators/cluster.go 80.27% <100.00%> (ø)
... and 4 more

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@crenshaw-dev
Copy link
Collaborator

@speedfl you're a force of nature.

I doubt I'll personally have time to review this in time for 2.6. But if the community likes the approach, I'd love to get it into 2.7.

@crenshaw-dev crenshaw-dev self-assigned this Dec 5, 2022
@speedfl speedfl force-pushed the feature/11164 branch 4 times, most recently from 64e73b8 to 26b7ea6 Compare December 7, 2022 13:27
@michalschott
Copy link

@crenshaw-dev can you explain how can we as community motivate you more to "merge it asap as this is what we need"? 😄

@crenshaw-dev crenshaw-dev added this to the v2.7 milestone Dec 12, 2022
@boedy
Copy link

boedy commented Dec 26, 2022

Interesting approach. I think this is exactly what I need. Would we be to toggle automated by using only a single variable as in the example bellow?

syncPolicy:
  '{{ ternary "automated" "noAuto" .automated }}':
    prune: true
    selfHeal: true

@speedfl
Copy link
Contributor Author

speedfl commented Dec 26, 2022

@boedy yes. You Can take a look to examples and e2e

@mubarak-j
Copy link
Contributor

apologies if this repeating the same question, but can parameter values provided by various generators be used as a condition here, e.g labels provided by cluster generator?

kind: ApplicationSet
spec:
  goTemplate: true
  template:
    spec:
      syncPolicy:
{{- if eq (index .metadata.labels "env") "staging" }}
        automated: {}
{{- end }}

@crenshaw-dev
Copy link
Collaborator

@crenshaw-dev can you explain how can we as community motivate you more to "merge it asap as this is what we need"? 😄

@michalschott pay Intuit to clone me. ;-) But seriously, I'll be advocating with my PM to get my time assigned to this before 2.7.

@mubarak-j templates outside of keys or string-type values are not part of this PR. For that, you'll need this: #11183

@crenshaw-dev
Copy link
Collaborator

Oh! But @mubarak-j, you could accomplish the same thing by doing this:

kind: ApplicationSet
spec:
  goTemplate: true
  template:
    spec:
      syncPolicy:
        '{{ ternary "automated" "noAuto" (eq (index .metadata.labels "env") "staging")}}': {}

My bad, I was looking at the exact text of your example instead of the intention. :-)

Signed-off-by: gmuselli <geoffrey.muselli@gmail.com>
@speedfl
Copy link
Contributor Author

speedfl commented Feb 8, 2023

@crenshaw-dev Hope you are doing well. A little present :), I rebased from master with small improvments compared to previous version.

Initially only the spec in the template was fully templatable. Now the entire template is a raw Json so that we can as well template metadata (ie: conditional finalizers)

Added documentation + e2e

@crenshaw-dev
Copy link
Collaborator

@speedfl that's awesome, thanks!

I just got approval to spend time reviewing this for 2.7. It'll be a couple weeks before I can dive in.

A couple notes:

  1. it would be nice to still build a "full CRD" for folks to use in their IDEs - but I'd consider that nice-to-have
  2. with the whole Application being templateable, I think we'll need to add code to protect the status field (and maybe one or two other top-level fields that might exist - I need to look at the Application CRD).

@speedfl
Copy link
Contributor Author

speedfl commented Feb 14, 2023

@crenshaw-dev

Thanks a lot for the suggestion. I will add the status field protection in applicationset_controller.validateGeneratedApplications + unit tests

Let me know if you have other fields in mind

Concerning the CRD I will try to think about it. If you have suggestion they are welcome.
The main problem is that map[string]interface{} is not working with kubebuilder. That's why I had to use the JSON with Raw String kubernetes-sigs/controller-tools#636. And I don't think it is possible to declare fields inside the Json

Signed-off-by: gmuselli <geoffrey.muselli@gmail.com>
@crenshaw-dev
Copy link
Collaborator

@speedfl would you mind resolving conflicts?

Makes sense w.r.t. the CRD. I think it's fine to leave it plain-json.

@crenshaw-dev
Copy link
Collaborator

Probably a version difference. Try running make install-go-tools-local && make install-codegen-tools-local.

@speedfl
Copy link
Contributor Author

speedfl commented Mar 17, 2023

All done @crenshaw-dev Have a good weekend :)

@vl-kp
Copy link

vl-kp commented Apr 15, 2023

any update?

@speedfl
Copy link
Contributor Author

speedfl commented Apr 15, 2023

@vl-kp I will take it back soon. I think it could be interesting to merge it just after the official release 2.7 so that we have time to test it for 2.8. it is a quite big PR. what do you think @crenshaw-dev

@crenshaw-dev
Copy link
Collaborator

Yep, that makes sense. I also wanna make some quick effort to see if we can still use the ApplicationSpec struct for CRD generation but use preserve-fields to make sure the code can access all fields. I just haven't had time to try it.

Signed-off-by: gmuselli <geoffrey.muselli@gmail.com>
@FCosta999
Copy link

Any idea when this is going to be merged?

@speedfl
Copy link
Contributor Author

speedfl commented May 29, 2023

@FCosta999 I am sorry but I am not sure I will be able to work on this one for 2.8 but I can prioritize.
@crenshaw-dev did you have time to check the crd generation ?

@speedfl speedfl closed this Aug 18, 2023
@blakepettersson
Copy link
Member

This has been superceded by #14893 right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants