Update with start and signal with start #201
Conversation
cretz
changed the title
| end | ||
|
|
||
| # Start an update, possibly starting the workflow at the same time if it doesn't exist (depending upon ID conflict | ||
| # policy). Note that in some cases this may fail but the workflow will still be started, and the handle can then be |
There was a problem hiding this comment.
this is unclear in this sentence.
| # policy). Note that in some cases this may fail but the workflow will still be started, and the handle can then be | |
| # policy). Note that in some cases, the Update message may fail though the Workflow was sucessfully started; the handle can then be |
There was a problem hiding this comment.
The method doc saying "this will fail" means "this method will fail". I don't want to say the update message may fail because that may be a different meaning. I am telling the user to watch out, an exception may still mean partial success due to the unfortunate lack of atomicness of this API (unlike signal with start which will all-fail or all-succeed).
| start_workflow_operation:, | ||
| rpc_options: nil | ||
| ) | ||
| @impl.signal_with_start_workflow( |
There was a problem hiding this comment.
From a user's POV, is there any important difference between the classic, dedicated SignalWithStart API and this one, based on the MultiOps API? I mean, is there anything that could really surprise users coming from other SDKs and could be worth mentioning in docs?
There was a problem hiding this comment.
There is no difference. This does not use multi-ops, this uses the classic. This is just us trying to have a consistent user-facing API even though the gRPC API is inconsistent.
| l.include?('"UpdateInfoWorkflow"') && l.include?('update_id') && l.include?('"update-2"') | ||
| end) | ||
| end | ||
|
|
There was a problem hiding this comment.
Let's add coverage for start_update_with_start_workflow.
There was a problem hiding this comment.
It is covered, execute_update_with_start_workflow calls that (and basically only that + .result by intention)
There was a problem hiding this comment.
What I mean is let's add coverage demonstrating that a user can obtain an update handle, and test that their update handle behaves as we expect (yields result on update success, yields exception on update fail/cancel, etc).
There was a problem hiding this comment.
I think covering code in a test and demonstrating a user can do something are a bit different. We do have plans to add samples at temporalio/samples-ruby#22 demonstrating that a user can do those things. But it doesn't have value as a separate test here since the same code is already tested.
There was a problem hiding this comment.
(merging, but if we do feel that the coverage could be improved or something is missing from test, we can always add it)
3 participants
What was changed
Temporalio::Client::WithStartWorkflowOperationclassstart_update_with_start_workflowandexecute_update_with_start_workflowto client and all necessary plumbingsignal_with_start_workflowto client and all necessary plumbingChecklist