-
Notifications
You must be signed in to change notification settings - Fork 107
Support workflow termination. #49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| workflow_execution: Temporal::Api::Common::V1::WorkflowExecution.new( | ||
| workflow_id: workflow_id, | ||
| run_id: run_id, | ||
| request_id: SecureRandom.uuid |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a field on WorkflowExecution but on the higher ResetWorkflowExecutionRequest so I don't think this method would ever have worked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also it's optional, so providing a random string that we don't return anyway doesn't feel useful.
lib/temporal.rb
Outdated
| response.run_id | ||
| end | ||
|
|
||
| def terminate_workflow(workflow, workflow_id:, **options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please make it look like coinbase/cadence-ruby#19?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean the method signature? I think it should line up with start_workflow/schedule_workflow, and they use kwargs. terminate_workflow_execution takes namespace as an argument so I think ExecutionOptions should be used so that the Workflow classes namespace configuration is taken into account (as for start/schedule).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated the method signature, although left namespace as nillable because it has a default value we can use.
…nal-with-start Update our FailWorkflowTask logic's call to ErrorHandler.handle
No description provided.