feat(cron): cronworkflows when clause

[isubasinghe]

Motivation

Modifications

This supersedes #13365 allowing for the evaluation of expr-lang expressions before running a CronWorkflow.

The scope of variables in this PR is quite limited, but can be further extended in later PRs when variable scoping is properly defined/formalised.

Verification

Added some tests for the evaluation function.

[Joibel]

This needs documenting, with examples and the variables we can use.

I'd also like to bring across the documentation from #13365 regarding how to do the various things mentioned there but using this new technique. I'm happy to contribute that, but the basic docs need to be in here first I think.

[agilgur5]

This needs documenting, with examples and the variables we can use.

See also the "Variables" page

[agilgur5]

Mainly reviewed the docs and examples below, but that also lends itself to some usage concerns I have about some of the variables

[JPZ13]

Hey @agilgur5 - Hope you had a good holiday weekend. I believe all of the changes you requested have been updated. I might have missed one, so please verify that. Could you remove the change request or request more changes so that we can keep moving forward on this? We'd like to roll out the when clause soon and recommend it to some of our customers

[isubasinghe]

Note: This uses the deprecated govaluate library. This is very intentional and done to keep the logic similiar to the other When fields. Initially I implemented this using purely expr but after a chat with @Joibel we decided it was best to keep the logic as similiar as possible.

This field is intended to be deprecated and replaced with a whenExpression field in the future.

I will create another issue to unify the logic used in the When fields and to also use expr there instead, after this PR is merged in.

[agilgur5]

context, changed direction should be discussed

For context, Isitha followed up with me on Slack and I repeated my concern about using deprecated govaluate above. The examples here do use embedded expr though, which is part of the "soft" deprecation I outlined in #9529 (comment) / #7576 (comment). However, it's still concerning to add a new feature using a deprecated feature / library.
I said that such usage actually makes me more strongly consider whenExpression of #7831, which I left open as an option, but isn't part of the same plan linked above.

Isitha explained that he talked to Alan and decided to use whenExpression instead, to which I said there was no context for in the PR, which Isitha then added above.
With no rationale, adding a field that uses deprecated govaluate goes directly against Isitha, mine, and Alex C's stated intents in #13471 and #9529.

I'm not necessarily opposed to using whenExpression instead (I did leave it open after all), but a change in direction should really be discussed in the open, especially before being implemented in a PR.
Making a PR that goes directly against stated intentions in multiple issues without any discussion is, at best, confusing. (and even more so for users who may have been following along -- "I thought govaluate was planned for deprecation, why does this new field still use it?" is a very valid question)

when vs. whenExpression

This field is intended to be deprecated and replaced with a whenExpression field in the future.

I wouldn't release a new field that uses a deprecated library and itself will be shortly deprecated.

If anything, if we want to keep the logic identical -- we should just release this as whenExpression and skip creating a new when field that will be deprecated for CronWorkflows.
The Workflow resource will then have a new field that does the same and its when field would be the only one to need deprecation. I.e. we would not add more fields to deprecate

expression usage

To be clear, per #9529 (comment), when is the only outlier that uses govaluate. Everything else uses "expressions", which the docs describe as using expr (and which I unified/consolidated in #12617), sometimes with fields explicitly named expression.

There is also fasttemplate and text/template for "simple"/"plain" substitutions (those are terms the docs use), which are also planned to be replaced, but are inherently not as confusing to users as govaluate since they're not expression engines (they are much more confusing internally in the code vs externally to users). #9529 (comment) goes over this as well.

I will create another issue to unify the logic used in the When fields and to also use expr there instead, after this PR is merged in.

This was already described in #9529 and was partly duplicated in #13471. We can continue using #9529 for that. I've renamed both of those for specificity now.

[JPZ13]

Thank you for the feedback @agilgur5. It's a bit hard for me to parse through the entire comment given I haven't been following all of the discussions and am strapped for time. Could you concisely give us a couple bullet points of:

• where you feel there has been an inappropriate closed discussion, and
• examples of a technical solution you would consider to be acceptable?

Our goal is to maintain consensus, keep our customers on mainline Argo Workflows expressions, and move quickly but thoroughly. I think working off of more concrete examples of what a when or whenExpression will look like will help move us forward

Edit: Upon re-reading the comment, another interpretation would be that you're objecting to the plan of matching the implementation of when with hooks that uses govaluate under the hood and then planning to migrate them both away from govaluate in one swoop. That or you feel that there should be more documentation/guidance around it. Either way, could you clarify?

[terrytangyuan]

I think it's okay to use govaluate for new feature even if it will be deprecated in the future. It's already widely adopted in the code base and the syntax is familiar to existing users. I recommend using it for this feature, work on consistency in future versions, and then provide a very detailed migration guideline to users who'd like to upgrade.

This PR has been here for a month and I hope my comment breaks the tie if any and helps unblock adoption. @agilgur5 - could you let us know if this sounds good within the next two days? I'll merge it on Thursday. We can continue discuss migration path and work on consistency in a follow-up issue.

[agilgur5]

This PR has been here for a month

Alan only approved it 2 weeks ago, which was shortly after I noticed the govaluate usage and shortly before your own review comments.

@agilgur5 - could you let us know if this sounds good within the next two days?

I still have another unresolved comment as well

It's already widely adopted in the code base and the syntax is familiar to existing users.

Neither of these are true:

• To repeat, govaluate is used in a single place, when, whereas every other usage of expressions uses expr. In fact, the term "expression" in field names and in the docs always refers to expr.
• Likely in part due to its lack of usage in the codebase, govaluate is actually very poorly documented by Argo: Expand documentation on conditional expressions (govaluate) #7382. You can also embed expr expressions inside of when, to add to the confusion, per Confusingly, when _kind of_ supports expr expressions #7576
  • The very confusing error messages from govaluate and our wrapper of it are one of the single most common questions by users (e.g. Slack question from this week, which I actually don't know how to answer because the error gives no details)
• We in fact generally tell users to use embedded expr expressions within when statements, which is also what this PR's own docs do.

of matching the implementation of when with hooks that uses govaluate under the hood

Since a couple contributors and maintainers in this very thread have said that govaluate is used elsewhere, like in hooks (it is not, to be clear), this also suggests the lack of consistency is poorly understood by maintainers as well.

That is all the more reason to not increase inconsistency and to actually properly discuss this.

We can continue discuss migration path and work on consistency in a follow-up issue.

Again, these exist and they predate me, and Isitha previously expressed agreement with this in the open prior to this PR.

None of the above options mention adding another when field that uses deprecated govaluate.

[agilgur5]

• where you feel there has been an inappropriate closed discussion,

Initially I implemented this using purely expr but after a chat with @Joibel we decided it was best to keep the logic as similiar as possible.

@JPZ13 ^The above was not in the open. Isitha only wrote this after I specifically asked about it and mentioned whenExpression. The open discussions are in #9529, #13471, #7831, and their duplicates, none of which say to further extend usage of govaluate and precedent is actually to use expr for all new fields (that's why when is the odd one out).

that you're objecting to the plan of matching the implementation of when with hooks that uses govaluate

when with when, not hooks, which do not use govaluate, per my above comments.

Otherwise, yes, this is correct. To be clear, this was not previously discussed, but I'd rather add a whenExpression field immediately than introducing another govaluate-based when which is immediately deprecated. In fact, we can add whenExpression to plain workflows simultaneously as well to keep them consistent.

I think adding whenExpression is a superior option to adding a deprecated when and has also been previously discussed and fits in with prior plans and precedent (no further usage of govaluate).
Adding an already confusing, deprecated field -- with docs that embed expr no less -- that users would have to migrate off of sounds like it would add even more confusion.

[agilgur5]

@isubasinghe @Joibel Could any of you create a follow-up issue?

Regarding when -> whenExpression, I would propose we add to the existing #7831.

Should the templating syntax and docs here be unified with stopStrategy of #12305 ?

For instance, the variables there may make sense to prefix with cronworkflow.. and the docs could be moved to a subheading under this PR's docs

This might require a separate issue -- we should get any of these variable name consistency changes in before 3.6.0

[tooptoop4]

@isubasinghe could this be used to run first monday of the month or last friday of the month?

[agilgur5]

You can use arbitrary expressions, so you might be able to workaround it for #8348 with pure date/time manipulation.

For instance, if you use now().Day(), you can get the day of the month (as in today is "the 18th").
So if you have a schedule weekly on Fridays and a when with now().Day() > 23 (day of the month is a bit coarse to use ofc), that might get you close to "last Friday of the month".
expr uses the time package for those functions, so you can potentially find a more accurate manipulation therein

[isubasinghe]

You can also get the weekday it seems in expr-lang: https://expr-lang.org/docs/language-definition#date-functions
So you can probably use @agilgur5 's solution above along with this to get the behaviour @tooptoop4

Probably a little bit hacky, but dates are always at least a little bit hacky :)

[isubasinghe]

Perhaps another feature could be stored functions to help with more complex expressions, that way you could reference these stored functions instead of having to write your expressions inline.

[agilgur5]

Should the templating syntax and docs here be unified with stopStrategy of #12305 ?
For instance, the variables there may make sense to prefix with cronworkflow.. and the docs could be moved to a subheading under this PR's docs

This might require a separate issue -- we should get any of these variable name consistency changes in before 3.6.0

Alan and I discussed this in the Oct 1st Contributor Meeting and agreed on this. See #13702 as a follow-up

[agilgur5]

I'd also like to bring across the documentation from #13365 regarding how to do the various things mentioned there but using this new technique. I'm happy to contribute that, but the basic docs need to be in here first I think.

This has been added in #13723