search

argoproj / argo-workflows

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

Fix `{{..}}` templating -- standardize on `expr` #9529

Open alexec opened 2 years ago

alexec commented 2 years ago

The current templating system appears to be the primary cause of confusion and bugs. These are caused by the following:

  1. Template evaluation is unpredicable. By which I mean, you don't know when it'll evaluate and what the output will be. This can result in a workflow that passes just some of the time
  2. We use several evaluation technologies: Govaluate, fasttemplate, Expr+Sprig.

Minor issues:

  1. {{..}} does not play nicely with Helm templating.
  2. podSpecPatch is arguably a work-around rather than a solution.

It would be great to remove this entires class of issue.

To fix this we should:

agilgur5 commented 7 months ago

I've written about my plans to standardize on expr in a few other places (https://github.com/argoproj/argo-workflows/issues/7576#issuecomment-1728725234, https://github.com/argoproj/argo-workflows/issues/12694, https://github.com/argoproj/argo-workflows/issues/12693#issuecomment-1974873029, etc), figured this issue should really be the central source of truth for that and so will document here.

  1. Close all related/duplicate issues (did this some months ago)
  2. "Soft deprecate" govaluate by removing it from the docs and examples and replacing it with expr everywhere, per https://github.com/argoproj/argo-workflows/issues/7576#issuecomment-1728725234
  3. "Hard" deprecate govaluate in 3.7 by detecting its usage (or non-expr usage) and logging out a deprecation warning saying to switch to expr
    • 3.7 is an example matching the current status: 3.6 is the next minor, so if the docs are updated in 3.5's time, give it one more minor to settle in
  4. Fully remove govaluate in 3.8 and error out when its usage is detected, saying to switch to expr
    • 3.8 is an example, should be at least one minor after the previous step

Per #7576, as when already supports expr (albeit with some confusing error messages), we can do this without a breaking spec change to the CRs

This covers govaluate but not quite fasttemplate (#7810) or text/template -- the same process could be followed for those. govaluate is arguably the most confusing one (based on all the issues around it etc) to focus initial efforts on. The other, non-expression templating systems are also documented in the same place as expr, so govaluate is kind of the outlier in that sense as it is only used in when.

agilgur5 commented 4 weeks ago
  1. {{..}} does not play nicely with Helm templating.

Syntax replacement has been spilt to #13471.

To be specific, this issue focuses on the expr standardization.