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(controller): Expression template tags. Resolves #4548 & #1293 #5115

Merged
merged 15 commits into from
Feb 27, 2021

Conversation

alexec
Copy link
Contributor

@alexec alexec commented Feb 16, 2021

Closes #4585
Closes #1293

image

@alexec alexec linked an issue Feb 16, 2021 that may be closed by this pull request
util/template/template.go Outdated Show resolved Hide resolved
workflow/json/fix.go Outdated Show resolved Hide resolved
go.mod Show resolved Hide resolved
go.mod Show resolved Hide resolved
@alexec alexec changed the title feat(controller): Enhanced templating. Resolves #4548 feat(controller): Enhanced templating. Resolves #4548 & #1293 Feb 22, 2021
@alexec alexec changed the title feat(controller): Enhanced templating. Resolves #4548 & #1293 feat(controller): Expression templates. Resolves #4548 & #1293 Feb 22, 2021
@crenshaw-dev
Copy link
Member

I feel like this would justify a new docs page or maybe an example or two. Looks like the only reference to expr so far is in the events docs.

@alexec alexec marked this pull request as ready for review February 24, 2021 16:47
@alexec
Copy link
Contributor Author

alexec commented Feb 24, 2021

@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?

@alexec alexec changed the title feat(controller): Expression templates. Resolves #4548 & #1293 feat(controller): Expression template tags. Resolves #4548 & #1293 Feb 24, 2021
@max-sixty
Copy link
Contributor

max-sixty commented Feb 24, 2021

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 {{item}} return here, without the equals?

              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}))}}"

examples/expression-template-workflow.yaml Outdated Show resolved Hide resolved
cmd/argo/commands/verify.go Outdated Show resolved Hide resolved
util/template/expression_template.go Show resolved Hide resolved
@alexec
Copy link
Contributor Author

alexec commented Feb 24, 2021

One brief question, what would {{item}} return here, without the equals?

That would be {{item}} which is the item as usual. In this case withParam: "{{=sprig.toJson(filter([1, 3], {# > 1}))}}" would evaluate to withParam: "[3]" which mean one item that is 3 (int).

Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
@max-sixty
Copy link
Contributor

One brief question, what would {{item}} return here, without the equals?

That would be {{item}} which is the item as usual. In this case withParam: "{{=sprig.toJson(filter([1, 3], {# > 1}))}}" would evaluate to withParam: "[3]" which mean one item that is 3 (int).

Right — if we had withParam: "[3]", then {{item}} would be OK in the value field IIUC?

Is the difference here that if you use an expression in the withParam field, then the value field needs to be {{=item}}?

@alexec
Copy link
Contributor Author

alexec commented Feb 24, 2021

One brief question, what would {{item}} return here, without the equals?

That would be {{item}} which is the item as usual. In this case withParam: "{{=sprig.toJson(filter([1, 3], {# > 1}))}}" would evaluate to withParam: "[3]" which mean one item that is 3 (int).

Right — if we had withParam: "[3]", then {{item}} would be OK in the value field IIUC?

Is the difference here that if you use an expression in the withParam field, then the value field needs to be {{=item}}?

yes, that works too

Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
@alexec
Copy link
Contributor Author

alexec commented Feb 25, 2021

@sarabala1979 can I ask you to be the reviewer on this and give me your feedback today or tomorrow please?

@simster7
Copy link
Member

Really good use case for this:

    podSpecPatch: '{"containers":[{"name":"main", "resources":{"requests":{"cpu":
      {{=retries * 100}} }}}]}'

docs/environment-variables.md Show resolved Hide resolved
util/template/expression_template.go Outdated Show resolved Hide resolved
util/template/expression_template.go Show resolved Hide resolved
util/template/kind.go Show resolved Hide resolved
util/template/replace.go Outdated Show resolved Hide resolved
util/template/template.go Outdated Show resolved Hide resolved
util/template/template.go Show resolved Hide resolved
util/template/validate.go Outdated Show resolved Hide resolved
workflow/common/util.go Show resolved Hide resolved
Signed-off-by: Alex Collins <[email protected]>
@alexec
Copy link
Contributor Author

alexec commented Feb 26, 2021

100% coverage:

image

@sarabala1979
Copy link
Member

sarabala1979 commented Feb 26, 2021

@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.
After reviewing this PR. I kind of thinking to refactor conditional artifact for use this template instead of direct expr. It will be the same template language across spec.

@alexec
Copy link
Contributor Author

alexec commented Feb 26, 2021

@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.
After reviewing this PR. I kind of thinking to refactor conditional artifact for use this template instead of direct expr. It will be the same template language across spec.

I'm not a fan of {{ and }}, due to the Helm conflict. I still think it would be nice to have fromExpression, I don't think you need to refactor. Unless, could fromExpression be redundant? It's hard for me to make a call on that. We still want number and string env funcs.

@lucastheisen
Copy link
Contributor

@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).

@alexec
Copy link
Contributor Author

alexec commented Feb 26, 2021

@alexec , my primary wish list item was the addition of control structures

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

@lucastheisen
Copy link
Contributor

@alexec , templating templated templates is never easy (we deploy our WorkflowTemplate's using ansible so thats another layer on top of this), but your suggestion:

{{= workflow.parameters['"dry-run"] ? '
      - "DRY RUN: "
'}}

seems to indicate possible. Right now, that's probably enough.

@sarabala1979
Copy link
Member

I'm not a fan of {{ and }}, due to the Helm conflict. I still think it would be nice to have fromExpression, I don't think you need to refactor. Unless, could fromExpression be redundant? It's hard for me to make a call on that. We still want number and string env funcs.

I kind of thinking on the user side to handle the two different formats. yes I am going to add number, string and jsonpath env function on env. This template is using expr underneath so, we can include those function into env.

current. expression: number(input.parameters.num) ==1? true: false

if we use template here the format
{{=number(input.parameters.num) ==1? true: false}}

@crenshaw-dev
Copy link
Member

@lucastheisen everything inside of source is just a string, as far as Kubernetes' YAML parser is concerned. So the possibilities within that string are endless.

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.

Copy link
Member

@sarabala1979 sarabala1979 left a 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

util/exrp/env/env.go Outdated Show resolved Hide resolved
@@ -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
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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

}
if s.tmpl != nil {
for _, a := range s.tmpl.Inputs.Artifacts {
m["inputs."+a.Name] = a // special case for artifacts
Copy link
Member

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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 {
Copy link
Member

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?

Copy link
Contributor Author

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

Signed-off-by: Alex Collins <[email protected]>
Copy link
Member

@sarabala1979 sarabala1979 left a 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 <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
Signed-off-by: Alex Collins <[email protected]>
@alexec alexec merged commit e6fa41a into argoproj:master Feb 27, 2021
@alexec alexec deleted the template branch February 27, 2021 22:00
@therc
Copy link

therc commented Feb 28, 2021

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.

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