Implement `schedule` commands #459

dnr · 2024-02-15T02:23:50Z

What was changed

Implement schedule commands for new CLI.

Why?

Feature parity.

Checklist

Closes
How was this tested:

new tests

Any docs updates needed?

The format of temporal schedule describe -o json changed to the --raw format of the previous cli.

…hedule

cretz

Looks good. Didn't pour over every output detail or anything.

temporalcli/commandsmd/commands.md

temporalcli/commands.schedule.go

cretz · 2024-03-15T15:24:34Z

temporalcli/commands.schedule.go

+		// For json: this doesn't come out as proper protojson, the field names are
+		// day_of_month instead of dayOfMonth.


This is the cost of mixing proto and non-proto JSON but there's not an east solution. What we do in some other commands is separate the JSON representation structs from the text ones, and for JSON, make this be something like []json.Message instead where we pre-JSON it. It became too confusing for complicated models to try to satisfy JSON and non-JSON needs with the same model (but if you must reuse models, consider more explicit JSON models then that meet the JSON field name needs).

I ended up adding a special case for slices in the printer.. I hope that's acceptable. With that, and a couple specialized structs in here, I think the text output for specs is much nicer.

For json, I realized I was making the problem much harder than it had to be: I just made -o json do what --raw did, for both describe and list. There's no reason at all to have two different json outputs.

I ended up adding a special case for slices in the printer.. I hope that's acceptable.

Works for me, not sure we've had a stance on nested slice printing so I don't mind making it friendlier. Looks like it breaks a test, so may have to update that.

For json, I realized I was making the problem much harder than it had to be: I just made -o json do what --raw did, for both describe and list. There's no reason at all to have two different json outputs.

Also works for me, because I find proto to be more stable and documented than CLI-specific structs anyways (but have to resort to specific structs when needing to combine of course)

Sushisource · 2024-03-18T18:22:28Z

temporalcli/commands.schedule.go

+	// TODO: should we include any other fields here, e.g. jitter, time zone, start/end time
+	out := &printableSchedule{


I think so. I'd expect describing to return everything available.

Added most of the data that's in the schedule description. Also rearranged a bunch of stuff and changed some formatting. I'm not sure it's great but it's closer.

temporalcli/commands.schedule.go

Sushisource · 2024-03-18T18:28:46Z

Also appears this will close #433 - if that's true can you update the description to close that when this is merged?

dnr · 2024-03-21T05:07:05Z

Also appears this will close #433 - if that's true can you update the description to close that when this is merged?

Not really, it just prints memo/search attrs in list, but not update them.

…ite-schedule

cretz

Looks good, just want to bring that test time down

temporalcli/commands_test.go

temporalcli/commands.schedule_test.go

cretz · 2024-03-21T12:41:47Z

temporalcli/commands.schedule_test.go

+	res := s.createSchedule("--interval", "2s")
+	s.NoError(res.Err)
+
+	time.Sleep(4 * time.Second) // run at least once


Can we avoid sleeps in tests? Can we do some kind of manual trigger/backfill here?

we can do a manual trigger, yeah. I wanted to keep the tests more orthogonal and use a normal scheduled run here, but I guess it doesn't matter.

temporalcli/commands.schedule_test.go

dnr · 2024-03-22T00:19:13Z

temporalcli/commands.schedule_test.go

+	res := s.createSchedule("--interval", "2s")
+	s.NoError(res.Err)
+
+	time.Sleep(4 * time.Second) // run at least once


we can do a manual trigger, yeah. I wanted to keep the tests more orthogonal and use a normal scheduled run here, but I guess it doesn't matter.

temporalcli/commands.schedule_test.go

temporalcli/commands_test.go

dnr · 2024-03-22T01:23:00Z

temporalcli/commands.schedule.go

+		WorkflowTaskTimeout:      opts.WorkflowTaskTimeout,
+		// RetryPolicy not supported yet
+		// TODO: buildStartOptions doesn't fill in TypedSearchAttributes, this doesn't work yet:
+		TypedSearchAttributes: opts.TypedSearchAttributes,


@cretz @Quinn-With-Two-Ns What do you think is the best way to handle this right now?

The situation appears to be: buildStartOptions builds search attributes in the form map[string]any (very easy to do from json) and puts them in StartWorkflowOptions.SearchAttributes. This is deprecated, but it still works for starting workflows.

For ScheduleWorkflowAction though, apparently untyped support is gone completely? The comment on UntypedSearchAttributes says This should never be used for create. Even if I could use it, it's a map[string]*commonpb.Payload which means I'd have to convert to Payloads (a little more work but not a huge deal).

To set TypedSearchAttributes, though, it looks like I'd have to do a lot of work. Basically it would have to look a lot like convertToTypedSearchAttributes from internal/internal_search_attributes.go, except with reflection?

I understand the goal of typed search attributes is to make them hard to use in a generic way, but the cli kinda needs to use them in a generic way 🫤 (So does "modify schedule search attributes" btw, except that's from workflow context so the code has to be different again.)

Ug, yeah, we've hit this a few times now with generic tools. If the UntypedSearchAttributes does work for now I think it's ok to use here and just assume that warning is for users only. The problem is technically a search attribute key for setting is supposed to be defined by its name and type, but we have no reasonable interface for that in CLI.

Note the starting of workflows sets untyped search attributes too, we were just forced to remove the either-or option on schedules because this same type is used for update and we couldn't tell which was the intended field on update (it was a long discussion internally before we made this backwards compatibility break and we documented it in release notes).

I confirmed UntypedSearchAttributes in the start workflow action works for create schedule so I used that for now (sorry Quinn) and added a test.

cretz

If there are obvious flag incompatibilities with today's CLI schedule command, can you mention those in the PR description? We're going to go across the PRs and collect them when we make our first release.

dnr · 2024-03-22T22:16:25Z

If there are obvious flag incompatibilities with today's CLI schedule command, can you mention those in the PR description? We're going to go across the PRs and collect them when we make our first release.

I'm not aware of any big incompatibilities, though there may be some small ones. The non-raw describe json output is different, but that's why --raw was advised before. Now it's just -o json

cretz · 2024-03-25T14:20:29Z

👍 No concerns about output compatibility

dnr added 7 commits February 14, 2024 18:12

all .md but create/update

c209817

more

2b1a62e

create/update w/shared options

74cc793

skeleton and fix build

97d51f0

add old impl as comments

f23179c

start implementing

28b3930

clean up for PR

47715fc

dnr mentioned this pull request Feb 28, 2024

Expose schedule memo and search attributes in list #434

Closed

Sushisource mentioned this pull request Feb 28, 2024

Updating Schedule with memo option not updating memo #433

Closed

dnr added 12 commits March 13, 2024 19:09

Merge remote-tracking branch 'origin/cli-rewrite' into cli-rewrite-sc…

7e3adf4

…hedule

trigger/pause/unpause

a703ceb

some work on describe

c6d170f

list, refactor

cd12651

more list, -l

5c53907

sa and memo

17b64df

start on start

514d0e9

timestamp type

4c54b6e

calendarspec -> cron string

f380e30

update

daed718

refactor

22951d5

start test

9298c57

dnr changed the title ~~Implement schedule commands [incomplete]~~ Implement schedule commands Mar 15, 2024

dnr marked this pull request as ready for review March 15, 2024 06:47

cretz reviewed Mar 15, 2024

View reviewed changes

Sushisource reviewed Mar 18, 2024

View reviewed changes

dnr added 4 commits March 20, 2024 18:31

get rid of --raw, json output is always raw

5bfa144

fix spec formatting

3c43ed9

use raw grpc for json list

c60f3dd

more output

0d96cf2

dnr added 2 commits March 20, 2024 20:10

fix test, no shorthand paylods

2bb0c2c

add some more tests

a627e21

dnr added 2 commits March 20, 2024 22:10

Merge branch 'cli-rewrite' of github.com:temporalio/cli into cli-rewr…

684aa9d

…ite-schedule

fix

f888d67

dnr force-pushed the cli-rewrite-schedule branch from 595e771 to f888d67 Compare March 21, 2024 05:18

cretz reviewed Mar 21, 2024

View reviewed changes

dnr added 3 commits March 21, 2024 18:02

add todo for shorthand payloads

a44ff13

faster tests

a6e00b9

fix TestTaskQueue_BuildId

d0a9b16

dnr commented Mar 22, 2024

View reviewed changes

dnr added 2 commits March 22, 2024 11:21

fix and test roundtrip search attributes and memo

92f720e

drop testVars

0965de0

cretz approved these changes Mar 22, 2024

View reviewed changes

dnr merged commit 1e26acb into temporalio:cli-rewrite Mar 22, 2024
6 checks passed

dnr deleted the cli-rewrite-schedule branch March 22, 2024 22:19

josh-berry mentioned this pull request Apr 29, 2024

temporal 0.12.0 Homebrew/homebrew-core#170352

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `schedule` commands #459

Implement `schedule` commands #459

dnr commented Feb 15, 2024 •

edited

Loading

cretz left a comment

cretz Mar 15, 2024

dnr Mar 21, 2024

cretz Mar 21, 2024 •

edited

Loading

Sushisource Mar 18, 2024

dnr Mar 21, 2024

Sushisource commented Mar 18, 2024

dnr commented Mar 21, 2024

cretz left a comment •

edited

Loading

cretz Mar 21, 2024

dnr Mar 22, 2024

dnr Mar 22, 2024

dnr Mar 22, 2024

cretz Mar 22, 2024

dnr Mar 22, 2024

cretz left a comment

dnr commented Mar 22, 2024

cretz commented Mar 25, 2024

		// For json: this doesn't come out as proper protojson, the field names are
		// day_of_month instead of dayOfMonth.

		// TODO: should we include any other fields here, e.g. jitter, time zone, start/end time
		out := &printableSchedule{

Implement schedule commands #459

Implement schedule commands #459

Conversation

dnr commented Feb 15, 2024 • edited Loading

What was changed

Why?

Checklist

cretz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz Mar 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sushisource commented Mar 18, 2024

dnr commented Mar 21, 2024

cretz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cretz left a comment

Choose a reason for hiding this comment

dnr commented Mar 22, 2024

cretz commented Mar 25, 2024

Implement `schedule` commands #459

Implement `schedule` commands #459

dnr commented Feb 15, 2024 •

edited

Loading

cretz Mar 21, 2024 •

edited

Loading

cretz left a comment •

edited

Loading