-
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
fix!: consolidate cronworkflow variables #13702
Conversation
Summoning
|
84afcd9
to
d4e3e02
Compare
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.
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 😅
d4e3e02
to
08d15c2
Compare
Oh, ha ha! That's not fun. |
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 do apologize but I think this:
hack/api/swagger/swaggify.sh
is getting in your way.
Removed @ from commit message as requested
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 |
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.
There are some misses here and a semantic error in the docs. Do keep in mind the surrounding context when making docs changes, as they do reference and build upon each other.
The version annotations are also intentionally at the beginning.
See in-line comments below
docs/cron-workflows.md
Outdated
| `stopStrategy` | `nil` | v3.6 and after: defines if the CronWorkflow should stop scheduling based on an expression | | ||
| `stopStrategy.expression` | None | v3.6 and after: an expression which if present must evaluate to false for the workflow to be created | |
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 don't think it's necessary to explicitly explain both in the table as they are identical right now. You can instead link to the usage section below
| `stopStrategy` | `nil` | v3.6 and after: defines if the CronWorkflow should stop scheduling based on an expression | | |
| `stopStrategy.expression` | None | v3.6 and after: an expression which if present must evaluate to false for the workflow to be created | | |
| `stopStrategy` | `nil` | v3.6 and after: defines if the CronWorkflow [should stop scheduling](#automatically-stopping-a-cronworkflow) based on an expression | |
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've left in the .expression part as I feel it should be possible to use this table to understand what to do, and expression
is unguessable.
@@ -69,11 +69,12 @@ type CronWorkflowSpec struct { | |||
When string `json:"when,omitempty" protobuf:"bytes,12,opt,name=when"` | |||
} | |||
|
|||
// v3.6 and after: StopStrategy defines if the CronWorkflow should stop scheduling based on a condition | |||
// StopStrategy defines if the CronWorkflow should stop scheduling based on an expression. v3.6 and after |
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.
The version annotation being consistently the first thing is intentional so that it is the first thing you see. This change makes this inconsistent with every other version annotation in the codebase, like lines 64-68 above, as well as harder to notice (and we already have trouble with users not noticing versions and so should not increase that).
I believe you made this change for GoDoc purposes, which does raise a conundrum since that apparently requires it to start the same way? If it's just a convention, the consistency and visibility should supersede that.
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 did that to make it GoDoc compliant.
Issues #13474 and #12305 both introduced expressions into CronWorkflows as new features for 3.6. 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. stopExpression's `condition` is mostly not a field name used, 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. Signed-off-by: Alan Clucas <[email protected]>
Signed-off-by: Alan Clucas <[email protected]>
f93cd60
to
5878b21
Compare
Signed-off-by: Alan Clucas <[email protected]>
5878b21
to
1c20373
Compare
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.
LGTM
Requested changes have been addressed
Signed-off-by: Alan Clucas <[email protected]>
Context
Issues #13474 and #12305 both introduced expressions into CronWorkflows as new features for 3.6.
There are some inconsistencies between the two, per #13474 (comment) and #12305 (comment)
This PR was discussed at the October 1st 2024 Contributor meeting.
Motivation
The expressions for
when
include the prefixcronworkflow.
. Those forstopStrategy.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 theschedule
but then usewhen
to only actually take action if the previous run this day failed.stopStrategy
'scondition
is mostly not a field name used in workflows, so in agreement with @agilgur5 I've renamed it toexpression
.Modifications
Renamed
stopStrategy.condition
tostopStrategy.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 [email protected]