feat(controller): Use deterministic name for cron workflow children #4638
Conversation
Signed-off-by: Simon Behar <simbeh7@gmail.com>
| } | ||
|
|
||
| func getChildWorkflowName(cronWorkflowName string, scheduledRuntime time.Time) string { | ||
| return fmt.Sprintf("%s-%d", cronWorkflowName, scheduledRuntime.Unix()) |
There was a problem hiding this comment.
I think this function should accept scheduledRuntime as given and not modify it
This reverts commit ef8e3e3. Signed-off-by: Simon Behar <simbeh7@gmail.com>
simster7
force-pushed
the
det-cwf-name
branch
from
2da1fec
to
e1efd93
Compare
Signed-off-by: Simon Behar <simbeh7@gmail.com>
| @@ -8,22 +8,21 @@ import ( | |||
| ) | |||
|
|
|||
| func ConvertCronWorkflowToWorkflow(cronWf *wfv1.CronWorkflow) *wfv1.Workflow { | |||
| wf := toWorkflow(cronWf.TypeMeta, cronWf.ObjectMeta, cronWf.Spec.WorkflowSpec) | |||
There was a problem hiding this comment.
is this covered by existing tests?
There was a problem hiding this comment.
Yes it is. I added an extra test as well
workflow/cron/operator.go
Outdated
| if errors.IsAlreadyExists(err) { | ||
| // The scheduled workflow already exists, likely indicating that there is a corrupted LastScheduledTime field. | ||
| // If the intended scheduledRuntime is later than the present value in LastScheduledTime, then replace it | ||
| if scheduledRuntime.After(woc.cronWf.Status.LastScheduledTime.Time) { |
There was a problem hiding this comment.
If it is a conflict error, then our time should be the same surely? This code does not get run?
There was a problem hiding this comment.
Correct, I think after the overwriting stale LastScheduledRun issue is fixed in #4659 this check will be moot. Not sure I would remove it though as it can only fix a corrupted state if it encounters one. What do you think?
There was a problem hiding this comment.
I think you need to write tests and then bug fix and maintain all new code - so add less code if it achieves the same outcome
There was a problem hiding this comment.
Ok, will remove this then
|
@alexec Addressed comments |
alexec
changed the title
|
Back-ported to v2.12.0-rc5 |
…4638) Signed-off-by: Simon Behar <simbeh7@gmail.com>
Signed-off-by: Simon Behar simbeh7@gmail.com
Checklist: