Add CLI call for UpdateActivityOptions API #727
Conversation
|
|
| google.golang.org/protobuf v1.34.2 | ||
| go.temporal.io/api v1.43.0 | ||
| go.temporal.io/sdk v1.30.1 | ||
| go.temporal.io/server v1.26.2-125.1 |
There was a problem hiding this comment.
We cannot update main to a non-stable tag. But there is the next-server branch you can change this PR to target
| @@ -106,6 +106,32 @@ func (s *SharedServerSuite) TestActivity_Fail_InvalidDetail() { | |||
| s.Nil(failed) | |||
| } | |||
|
|
|||
| func (s *SharedServerSuite) TestActivityOptionsUpdate_Accept() { | |||
There was a problem hiding this comment.
Can we add one more test confirming a partial update doesn't mutate the existing value?
There was a problem hiding this comment.
what are we testing with that? If server implementation - it is already tested.
There was a problem hiding this comment.
Just an extra confirmation. Many (most?) of our CLI tests are end to end tests with the server implementation. We don't have to test everything, but I would like to confirm that only setting some CLI flags leaves unset values intact. Often we find we think we are building the proto message correctly but we don't test the behavior of it so we aren't sure.
There was a problem hiding this comment.
ok. I add TestActivityOptionsUpdate_Partial
| "--retry-backoff-coefficient", "2", | ||
| "--retry-maximum-attempts", "5", | ||
| ) | ||
| // atm, the command is not enabled |
There was a problem hiding this comment.
Should have PR wait to be merged until test can confirm behavior
There was a problem hiding this comment.
ok looks like we can change dynamic config values, so the output can be tested
| google.golang.org/protobuf v1.34.2 | ||
| go.temporal.io/api v1.43.0 | ||
| go.temporal.io/sdk v1.30.1 | ||
| go.temporal.io/server v1.26.2-125.1 |
There was a problem hiding this comment.
We can't merge this in main without a released open-source server version. Please target your PR to next-server instead.
| // atm, the command is not enabled | ||
| s.Error(res.Err) |
There was a problem hiding this comment.
We'll need to be able to actually verify the behavior here before merge.
temporalcli/commandsgen/commands.yml
Outdated
| Update an Activity's options or update an Activity's state to completed or failed. | ||
| Updating activity state marks an Activity as successfully finished or as having encountered an error. |
There was a problem hiding this comment.
Please wrap to 80 columns per the "Wrapping" section of the style guide at the top of this file. Same goes for the options and command descriptions.
temporalcli/commandsgen/commands.yml
Outdated
| temporal activity update \ | ||
| --activity-id YourActivityId \ | ||
| --workflow-id YourWorkflowId \ | ||
| --task-queue NewTaskQueueName \ | ||
| --schedule-to-close-timeout DURATION\ | ||
| --schedule-to-start-timeout DURATION\ | ||
| --start-to-close-timeout DURATION\ | ||
| --heartbeat-timeout DURATION\ | ||
| --retry-initial-interval DURATION\ | ||
| --retry-maximum-interval DURATION\ | ||
| --retry-backoff-coefficient NewBackoffCoefficient\ | ||
| --retry-maximum-attempts NewMaximumAttempts" |
There was a problem hiding this comment.
Inconsistent use of whitespace in your \s, and you have a stray "
temporalcli/commandsgen/commands.yml
Outdated
| Indicates how long the caller is willing to wait for an activity completion. | ||
| Limits how long retries will be attempted. |
There was a problem hiding this comment.
For your option descriptions, please follow the grammar recommendations in the style guide at the top of the file. You can look at other options (e.g. the workflow-start timeout options) if you need examples.
Co-authored-by: Chad Retz <[email protected]>
Co-authored-by: Josh Berry <[email protected]>
|
create new PR for next-server |
What was changed
Why?
Checklist
Closes
How was this tested: