-
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: Skip missed executions if CronWorkflow schedule is changed. Fixes #7182 #7353
Conversation
Signed-off-by: Simon Behar <simbeh7@gmail.com>
Signed-off-by: Simon Behar <simbeh7@gmail.com>
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.
this seems smart, more tests?
Signed-off-by: Simon Behar <simbeh7@gmail.com>
Signed-off-by: Simon Behar <simbeh7@gmail.com>
@alexec enhanced tests, they should no cover all expected behaviors |
Signed-off-by: Simon Behar <simbeh7@gmail.com>
@@ -34,6 +36,8 @@ const ( | |||
ReplaceConcurrent ConcurrencyPolicy = "Replace" | |||
) | |||
|
|||
const annotationKeyLatestSchedule = workflow.CronWorkflowFullName + "/last-used-schedule" |
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.
Moving the label here was necessary to avoid import cycles. I believe this is okay because we have full accessor methods and the label is private... this way implementation of the "last used schedule" concept is completely abstracted.
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.
maybe previous-schedule
?
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 think last-used-schedule
is more accurate. We're interested in the schedule that was last used to execute a workflow. This may or may not be the previous schedule.
Fixes #7182
Signed-off-by: Simon Behar simbeh7@gmail.com
Don't bother creating a PR until you've done this:
make pre-commit -B
to fix codegen, lint, and commit message problems.Create your PR as a draft.
does not need to pass.
Tips: