fix!: consolidate cronworkflow variables

Joibel commented 1 month ago

Context

Issues #13474 and #12305 both introduced expressions into CronWorkflows as new features for 3.6. There are some inconsistencies between the two, per https://github.com/argoproj/argo-workflows/pull/13474#issuecomment-2347998579 and https://github.com/argoproj/argo-workflows/pull/12305#issuecomment-2381625468

This PR was discussed at the October 1st 2024 Contributor meeting.

Motivation

The expressions for when include the prefix cronworkflow.. Those for stopStrategy.condition do not.

Unify both of these under cronworkflow. and allow both sets of variables to be used in both expressions. This simplifies the documentation and is less surprising. It also allows some use cases where you could check for previous run failures and take decisions based on that and time of day - so perhaps run every hour in the schedule but then use when to only actually take action if the previous run this day failed.

stopStrategy's condition is mostly not a field name used in workflows, so in agreement with @agilgur5 I've renamed it to expression.

Modifications

Renamed stopStrategy.condition to stopStrategy.expression

Created a single function to generate all the variables for both expressions and use that instead of the two disparate mechanisms.

Modified some code comments to be godoc compliant.

Verification

Existing tests still pass with appropriate amendments to the test cases

Note

This is a breaking change vs 3.6-rc1 and rc2 for users of stopStrategy only. This is not considered properly breaking as those are release candidates and not full releases.

Signed-off-by: Alan Clucas alan@clucas.org

Joibel commented 1 month ago

Summoning

@eduardodbr, the effect of this change is mostly on your implementation, so I'd appreciate feedback
@isubasinghe, this doesn't much affect when, but I'd also appreciate feedback
@agilgur5, as you're the one who pointed this out and I'd like to know if it matches your expectations

Joibel commented 1 month ago

Could you remove the @tag of me in the commit message? I'm getting notified for every single commit right now and uh would prefer not to, including when commits are backported or forked 😅

Oh, ha ha! That's not fun. Done.

Joibel commented 1 month ago

I do apologize but I think this: hack/api/swagger/swaggify.sh is getting in your way.

This is annoying. I'm not going to attempt to address this hack at the moment, so I've reworded it. Probably the correct fix is to understand the json and only replace in the two (I think) places in the structure which actually want it to happen, or at least not in title and description fields.

argoproj / argo-workflows