-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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(controller): Expression template tags. Resolves #4548 & #1293 #5115
Conversation
I feel like this would justify a new docs page or maybe an example or two. Looks like the only reference to |
@saranyaeu2987 @chermed @max-sixty @discordianfish @Ark-kun @iven @lucastheisen you've all shown strong interest in this feature - can I please ask to take a look at the syntax of expression tags and tell me what you think? Will it meet your use case? Can you see how to write your workflow using the syntax? Can you even demonstrate that? |
Having sprig in there is perfect for the use cases I had. That does everything I need. The argo syntax seems great. One brief question, what would parameters:
- name: foo
value: "{{=item}}"
# withParam must be a JSON list encoded as a string, so you use `toJson` to perform the conversion
withParam: "{{=sprig.toJson(filter([1, 3], {# > 1}))}}" |
That would be |
Right — if we had Is the difference here that if you use an expression in the |
yes, that works too |
@alexec Thanks. I really like this custom wrapper on the template and expression language. It gives us the control to extend the expression function and includes a new template language in the future. |
I'm not a fan of |
@alexec , my primary wish list item was the addition of control structures (or something else capable of dynamic argument list based on workflow parameter). This is not intended to enable that, correct? Its possible i missed some way of leveraging sprig to generate an argument list with one of the items being optional, but i am not seeing it. Otherwise, the addition of sprig expressions is definitely a benefit (we do date calculations frequently). |
No. The example you have is not valid YAML and cannot be implemented except by a pre-processor like Helm. I think you could achieve the same goal like this: apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
generateName: argo-test-echo-
spec:
entrypoint: echo
arguments:
parameters:
- name: dry-run
value: "false"
templates:
- name: echo
container:
command:
- sh
- -c
- |
echo {{= workflow.parameters['dry-run'] ? ' "DRY RUN: "' : ''}} hello world
image: argoproj/argosay:v1 |
@alexec , templating templated templates is never easy (we deploy our {{= workflow.parameters['"dry-run"] ? '
- "DRY RUN: "
'}} seems to indicate possible. Right now, that's probably enough. |
I kind of thinking on the user side to handle the two different formats. yes I am going to add current. expression: number(input.parameters.num) ==1? true: false if we use |
@lucastheisen everything inside of But argument lists aren't strings - they're lists. Lists of Arguments (represented as lists of objects in YAML and then deserialized to Go data structures in Argo code). Argo can't provide a templating tool to create dynamic argument lists, because the argument lists have to be valid YAML before they get to Argo in the first place. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial Review. I am cloned the PR and trying in my local
workflow/controller/operator.go
Outdated
@@ -2934,7 +2925,7 @@ func (woc *wfOperationCtx) computeMetrics(metricList []*wfv1.Prometheus, localSc | |||
woc.reportMetricEmissionError("real time metrics can only be used with metric variables") | |||
continue | |||
} | |||
value = strings.TrimSuffix(strings.TrimPrefix(value, "{{"), "}}") | |||
value = strings.TrimSuffix(strings.TrimPrefix(value, "{{"), "}}") // TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you missing any TODO here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should use ResolveVar, yes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, not really feasible, code here is very bespoke to metrics
workflow/controller/scope.go
Outdated
} | ||
if s.tmpl != nil { | ||
for _, a := range s.tmpl.Inputs.Artifacts { | ||
m["inputs."+a.Name] = a // special case for artifacts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the key should be `inputs.artifacts '+a.Name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
correct. i misunderstnand the old value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are correct
if art != nil { | ||
return *art, nil | ||
m := make(map[string]interface{}) | ||
for k, v := range s.scope { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s.scope is already map[string]interface{}
. can we just add new keys to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would modify it, want to copy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGMT
Signed-off-by: Alex Collins <alex_collins@intuit.com>
Thanks for adding this feature. I assume that it will be in 3.0, but not cherry-picked in 2.12 or any other 2.x releases, correct? I can't wait to try it. |
Closes #4585
Closes #1293