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

Confusingly, when _kind of_ supports expr expressions #7576

Open
crenshaw-dev opened this issue Jan 18, 2022 · 15 comments
Open

Confusingly, when _kind of_ supports expr expressions #7576

crenshaw-dev opened this issue Jan 18, 2022 · 15 comments
Assignees
Labels
area/spec Changes to the workflow specification. area/templating Templating with `{{...}}` solution/suggested A solution to the bug has been suggested. Someone needs to implement it. solution/workaround There's a workaround, might not be great, but exists type/tech-debt

Comments

@crenshaw-dev
Copy link
Member

Summary

My workflow worked. I expected it to not work. I used the following when expression:

when: "{{=jsonpath(inputs.parameters.json, '$.a') == 'b'}}"

I expected to get an error like this: Failed to submit workflow: templates.top when expression doesn't support 'expr' format '{{='. 'When' expression is only support govaluate format {{ .

But instead, the workflow ran! However, if I introduce a typo to the expression:

when: "{{=jsonpath(inputs.parameters.json, '.a') == 'b'}}"

I get the above error message as expected. Presumably because the failed expression was not substituted, so the when validation logic actually caught its presence.

What version of Argo Workflows are you running?

Controller: 3.2.6
CLI: 3.2.3

Diagnostics

This workflow runs without error, even though it seems like it should fail with a validation error.

apiVersion: argoproj.io/v1alpha1
kind: Workflow
metadata:
  generateName: should-not-work-
spec:
  arguments:
    parameters:
      - name: json
        value: >-
          {"a": "b"}
  entrypoint: top
  templates:
  - name: top
    inputs:
      parameters:
        - name: json
    steps:
      - - name: whalesay
          when: "{{=jsonpath(inputs.parameters.json, '$.a') == 'b'}}"
          template: whalesay
  - name: whalesay
    container:
      image: docker/whalesay:latest
      command: [cowsay]
      args: ["hello world"]

I'm using docker-desktop k8s with the default executor.

Here's my guess: when the expr expression is valid, it gets executed, and the result is dropped into the when field. If the output is a valid govaluate expression, the when logic just treats it as a hard-coded govaluate expression.

I'm not really sure how this should be fixed. You could just drop the warning. But an expr expression that doesn't produce a govaluate expression is still invalid. You could document the behavior, but it's really weird to explain. You could add first-class expr support for when conditions, but that's a lot of work.

Or nothing, and this ticket could serve as documentation of this weird edge case. :-P


Message from the maintainers:

Impacted by this bug? Give it a 👍. We prioritise the issues with the most 👍.

@sarabala1979
Copy link
Member

sarabala1979 commented Jan 18, 2022

Currently When doesn't support expr format.

@crenshaw-dev
Copy link
Member Author

But it does a little bit. If I write an expression tag template which 1) executes successfully and 2) produces a valid govaluate expression, then the when will execute successfully. For example, I could do when: "{{='1 == 1'}}".

@crenshaw-dev
Copy link
Member Author

This means I can write any expr template which evaluates to true or false, and it will act as a valid when expression.

For example, when: "{{=jsonpath(inputs.parameters.json, '$.pull_request.head.ref') == 'master'}}" should work fine.

@sarabala1979
Copy link
Member

@crenshaw-dev go-evaluate template format is {{ condition }} Eg.: {{1==1}}

@crenshaw-dev
Copy link
Member Author

crenshaw-dev commented Jan 19, 2022

Kind of, but not precisely. If you used exactly that when value, you'd get this error:

Invalid 'when' expression '{{1 == 1}}': Invalid token: '{{' (hint: try wrapping the affected expression in quotes ("))

{{}} designates a variable substitution. The output of that substitution is what actually gets evaluated by govaluate. That's why I can get away with using {{=}} expressions as a when condition. The output of that expression is a valid govaluate expression.

@alexec
Copy link
Contributor

alexec commented Feb 4, 2022

We should have a new when that uses expression syntax.

@ericmeadows
Copy link

Coincidentally, our build infrastructure for Plumbr.ai is trying to use this as well! I have a workaround, but what we open-source won't be too elegant until this - looking forward to it coming soon :)

@crenshaw-dev
Copy link
Member Author

@ericmeadows I'm pretty sure the build system I put together at my last job relies on this feature. At the time I assumed it had first-class support. So really hope it doesn't break. 😆

@alexec alexec removed the v3.2 label Feb 6, 2022
@alexec
Copy link
Contributor

alexec commented Feb 6, 2022

@ericmeadows would you like to submit a PR to introduced an alternative to when, e.g. whenExpression?

@alexec alexec added the area/controller Controller issues, panics label Feb 7, 2022
@crenshaw-dev
Copy link
Member Author

@alexec I don't think it's actually necessary to add a new field.

Despite what the error message says, when does support expressions. If your expression throws an error, you get a misleading error message saying "when does not support expressions." But I think the error message is the bug, not an actual lack of support for expressions.

For any valid expression producing true or false, when will correctly handle the expression's output. What benefit would whenExpression provide that can't simply be applied to the when field? Improving error handling does not, as far as I can tell, break any contract.

@stale
Copy link

stale bot commented Mar 2, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the problem/stale This has not had a response in some time label Mar 2, 2022
@crenshaw-dev
Copy link
Member Author

Just a reminder that when does support expressions, and that removing support would break Workflows (like ones I wrote at my last job). It's fine to add a whenExpression field, but it should not break the (accidental) support for expressions in when.

@stale stale bot removed the problem/stale This has not had a response in some time label Mar 2, 2022
@katylava
Copy link

katylava commented Aug 14, 2023

Looks like as of May 2022 the docs indicate expressions are supported for when: https://github.com/argoproj/argo-workflows/blame/fa09afce11f1c87279b1dc4a4b18c6b83990a146/docs/walk-through/conditionals.md#L77, so I feel safe relying on this behavior.

I think the real issue is just that it's a confusing error message.

@agilgur5 agilgur5 added solution/workaround There's a workaround, might not be great, but exists area/templating Templating with `{{...}}` labels Sep 5, 2023
@agilgur5
Copy link
Contributor

It looks like this was explicitly implemented in #9761, which should have also removed the error message entirely

@agilgur5
Copy link
Contributor

agilgur5 commented Sep 21, 2023

As such, I'm considering just updating the Conditionals doc to explicitly use expr and remove all usage and references of govaluate in the docs as a form of soft deprecation.
There are potentially some advantages to whenExpression from #7831 (as it is explicit and could be typed as such), but as we're kind of stuck with when + govaluate unless a breaking change to the Workflow spec is made, docs changes for a soft deprecation could potentially ease all of the user confusion in the interim (which could be/has been quite long) at least, as well as better prepare users for a true deprecation and removal in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/spec Changes to the workflow specification. area/templating Templating with `{{...}}` solution/suggested A solution to the bug has been suggested. Someone needs to implement it. solution/workaround There's a workaround, might not be great, but exists type/tech-debt
Projects
None yet
Development

No branches or pull requests

6 participants