Fix schedule update removing cron/spec when not provided (fixes #927) #928
Conversation
- Describe current schedule before building update and merge CLI with existing spec - Preserve CronExpressions/Calendars when --cron and --calendar not passed - Preserve Intervals when --interval not passed - Preserve Jitter, TimeZoneName, StartAt, EndAt when corresponding flags not set (fully incremental update using FlagSet.Changed) - Add toScheduleSpecForUpdate(spec, existing); create path still uses toScheduleSpec - Add tests: TestSchedule_Update_PreservesCronWhenNotProvided, TestSchedule_Update_PreservesJitterWhenNotProvided
| // toScheduleSpecForUpdate applies CLI options to spec, preserving existing spec | ||
| // fields when the user did not pass the corresponding flags (fully incremental update). | ||
| // If existing is nil, behaves like toScheduleSpec (no preservation). | ||
| func (c *ScheduleConfigurationOptions) toScheduleSpecForUpdate(spec *client.ScheduleSpec, existing *client.ScheduleSpec) error { |
There was a problem hiding this comment.
I will let CLI reviewers review this, but I wanted to clarify...
The way this CLI works is not a bug, it is very much by intention. It's a backwards incompatible alteration to change this CLI command from being full update to be partial update. For example, users have been able to remove jitter by simply not providing the --jitter flag, but now they can't. Not only is that an incompatible alteration, there are likely a lot of users using it in this declarative form (i.e. they make update calls that are effectively no-ops because they have the same CLI args as what's already on server).
There is an inherent desire for declarative schedules so a user can partial update what they know to be the spec in some separate file. In the meantime, to remain compatible, we need to split off the concept of "partial update" and "full update" (and therefore if the former is needed as a new feature, it should be opted into either via flag or separate command).
There was a problem hiding this comment.
Makes sense, this might make more sense as a separate patch command or something where it is intended to keep everything except for updated fields.
Will wait for feedback from CLI team
There was a problem hiding this comment.
I agree with Chad on not making a backwards incompatible change. I'm not sure off the top of my head how we'd want to address this.
There was a problem hiding this comment.
I don't think the server api supports updating the cron only right now:
https://github.com/temporalio/api/blob/e966760430f98290e83459571f40c91126ece7d8/temporal/api/schedule/v1/message.proto#L300
There was a problem hiding this comment.
You can make whatever "patch" update you want with UpdateSchedule and the conflict token guard. This is built into the SDK as well
There was a problem hiding this comment.
(but keep in mind that cron strings are canonicalized to structured calendar specs)
What was changed
Describeand merge CLI options with existing spec.CronExpressionsandCalendarswhen--cronand--calendarare not passed.Intervalswhen--intervalis not passed.FlagSet.Changedfor fully incremental update).toScheduleSpecForUpdate(spec, existing): New merge logic; create path still usestoScheduleSpec(existing == nil).Why?
temporal schedule updatewas building a new schedule from scratch and sending it as a full replacement, so omitting--cron(or other spec flags) wiped existing values.Checklist
Closes [Bug] schedule update removing cron if not explicitly included #927
How was this tested:
TestSchedule_Update(existing): still passes.TestSchedule_Update_PreservesCronWhenNotProvided: create with--cron "0 9 * * *", update without--cron, assert cron preserved.TestSchedule_Update_PreservesJitterWhenNotProvided: create with--jitter 1m, update without--jitter, assert jitter preserved.I don't think so?
###Note:
I used AI to do most of the grunt work on this one, I reviewed all pushed code, but please call out if you notice anything I missed, or if anything doesn't follow project conventions, etc.