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

Support for missingkey=error in ApplicationSets with goTemplate: true #13731

Closed
raxod502-plaid opened this issue May 24, 2023 · 3 comments · Fixed by #13733
Closed

Support for missingkey=error in ApplicationSets with goTemplate: true #13731

raxod502-plaid opened this issue May 24, 2023 · 3 comments · Fixed by #13733
Labels
component:applications-set Bulk application management related enhancement New feature or request

Comments

@raxod502-plaid
Copy link
Contributor

Summary

Currently, ArgoCD does not offer any customization of the options used for Go templating:

template, err := template.New("").Funcs(sprigFuncMap).Parse(tmpl)

It would be nice if it were possible to set a key on the ApplicationSet spec that would customize the missingkey option.

Motivation

We find that it is easy to forget to set a key in the cluster generator, or there is a typo that leads to the wrong key being set. This results in the use of that key in the appset manifest being expanded to null or an empty string, which is usually not what we want. If we could tell ArgoCD to abort on a templating error we would catch issues earlier rather than potentially after they are already deployed.

Proposal

I would suggest a new key at the top level of the appset spec, next to goTemplate, which lists options to be passed to the template engine, in the same format as the underlying function. This ensures that work is not needed to keep up with new options that are added to Go.

If there are security concerns, we can add an allowlist for the supported options.

The existing behavior would remain the same as the default behavior would be to pass no extra options, which is what happens today.

@raxod502-plaid raxod502-plaid added the enhancement New feature or request label May 24, 2023
@crenshaw-dev
Copy link
Collaborator

This makes a lot of sense, I'd gladly review a PR to make this change.

@raxod502-plaid
Copy link
Contributor Author

@crenshaw-dev I've created #13733 to implement this feature. I've added passing unit tests although I haven't deployed the fork to a cluster to test in practice. The only thing missing from the contribution checklist is the feature status, should this field be marked as alpha?

@crenshaw-dev
Copy link
Collaborator

Thanks for the PR! I think this feature is simple enough to consider stable immediately.

crenshaw-dev added a commit that referenced this issue May 30, 2023
…3731) (#13733)

* Add support for missingkey=error in ApplicationSets

Signed-off-by: Radon Rosborough <rrosborough@plaid.com>

* options for cluster generator too

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

---------

Signed-off-by: Radon Rosborough <rrosborough@plaid.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
yyzxw pushed a commit to yyzxw/argo-cd that referenced this issue Aug 9, 2023
…goproj#13731) (argoproj#13733)

* Add support for missingkey=error in ApplicationSets

Signed-off-by: Radon Rosborough <rrosborough@plaid.com>

* options for cluster generator too

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

---------

Signed-off-by: Radon Rosborough <rrosborough@plaid.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
tesla59 pushed a commit to tesla59/argo-cd that referenced this issue Dec 16, 2023
…goproj#13731) (argoproj#13733)

* Add support for missingkey=error in ApplicationSets

Signed-off-by: Radon Rosborough <rrosborough@plaid.com>

* options for cluster generator too

Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>

---------

Signed-off-by: Radon Rosborough <rrosborough@plaid.com>
Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Co-authored-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:applications-set Bulk application management related enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants