argoproj / argo-workflows

Workflow Engine for Kubernetes
https://argo-workflows.readthedocs.io/
Apache License 2.0
15.11k stars 3.21k forks source link

fix(cron): allow unresolved variables outside of `when` #13680

Closed isubasinghe closed 1 month ago

isubasinghe commented 1 month ago

This PR allows for unresolved variables to be present in cronworkflows during the evaluation of the when clause. This is needed because workflow variables are not accessible/unresolved when the clause is evaluated.

isubasinghe commented 1 month ago

This is needed because workflow variables are not accessible/unresolved when the clause is evaluated.

I've provided an example in the test. The variables that a workflow may have, for example: {{workflow.name}} will not be available during the call to evalWhen, this means we must allow for unresolved variables when calling the templating engine.

agilgur5 commented 1 month ago

Oh you mean inside the workflowSpec? Can you limit the eval to just the when field instead?

isubasinghe commented 1 month ago

Yup, unfortunately, this isn't really possible. Otherwise, that would have been my preferred solution.

agilgur5 commented 1 month ago

Can you elaborate why?

If I'm not mistaken, this would also end up allowing invalid variables in when, which is a source of constant confusion in Argo templating (i.e. resulting in unresolved variables with no error messages).

isubasinghe commented 1 month ago

The current templating engine doesn't care, it looks for any string with the "{{" pattern and attempts to replace, and it does this at the entire workflow/cronworkflow level.

We absolutely need a refactor of how all this works.

agilgur5 commented 1 month ago

The functions used here, NewTemplate and Replace, take an arbitrary string. So if I'm not mistaken, you can make that work on a single field without any refactoring of the templating logic internals.

i.e. rather than doing a replace on the entire CronWorkflow JSON string, you can do it on the when string specifically.

isubasinghe commented 1 month ago

Yeah you are correct, I just wanted to keep the logic consistent across wherever we use templating but I suppose this is better.

isubasinghe commented 1 month ago

@agilgur5 alright, just did that.

agilgur5 commented 1 month ago

LGTM now, just one stylistic comment