-
Notifications
You must be signed in to change notification settings - Fork 3
IWF-936: fix FAIL_WORKFLOW_ON_FAILURE state option not working #113
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,78 @@ | ||
| from iwf.command_request import CommandRequest | ||
| from iwf.command_results import CommandResults | ||
| from iwf.communication import Communication | ||
|
|
||
| from iwf.iwf_api.models import RetryPolicy, WaitUntilApiFailurePolicy | ||
| from iwf.persistence import Persistence | ||
| from iwf.state_decision import StateDecision | ||
| from iwf.state_schema import StateSchema | ||
| from iwf.workflow import ObjectWorkflow | ||
| from iwf.workflow_context import WorkflowContext | ||
| from iwf.workflow_state import T, WorkflowState | ||
| from iwf.workflow_state_options import WorkflowStateOptions | ||
|
|
||
|
|
||
| class InitState1(WorkflowState[str]): | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added two workflows with a single state. One tests that upon waitUntil fails, it proceeds to execute() and completes the workflow. The other, tests that upon waitUntil fails, it fails the workflow (does not proceed to execute). |
||
| def get_state_options(self) -> WorkflowStateOptions: | ||
| return WorkflowStateOptions( | ||
| wait_until_api_retry_policy=RetryPolicy(maximum_attempts=1), | ||
| proceed_to_execute_when_wait_until_retry_exhausted=WaitUntilApiFailurePolicy.PROCEED_ON_FAILURE, | ||
| ) | ||
|
|
||
| def wait_until( | ||
| self, | ||
| ctx: WorkflowContext, | ||
| input: T, | ||
| persistence: Persistence, | ||
| communication: Communication, | ||
| ) -> CommandRequest: | ||
| raise RuntimeError("test failure") | ||
|
|
||
| def execute( | ||
| self, | ||
| ctx: WorkflowContext, | ||
| input: T, | ||
| command_results: CommandResults, | ||
| persistence: Persistence, | ||
| communication: Communication, | ||
| ) -> StateDecision: | ||
| data = "InitState1_execute_completed" | ||
| return StateDecision.graceful_complete_workflow(data) | ||
|
|
||
|
|
||
| class InitState2(WorkflowState[str]): | ||
| def get_state_options(self) -> WorkflowStateOptions: | ||
| return WorkflowStateOptions( | ||
| wait_until_api_retry_policy=RetryPolicy(maximum_attempts=1), | ||
| proceed_to_execute_when_wait_until_retry_exhausted=WaitUntilApiFailurePolicy.FAIL_WORKFLOW_ON_FAILURE, | ||
| ) | ||
|
|
||
| def wait_until( | ||
| self, | ||
| ctx: WorkflowContext, | ||
| input: T, | ||
| persistence: Persistence, | ||
| communication: Communication, | ||
| ) -> CommandRequest: | ||
| raise RuntimeError("test failure") | ||
|
|
||
| def execute( | ||
| self, | ||
| ctx: WorkflowContext, | ||
| input: T, | ||
| command_results: CommandResults, | ||
| persistence: Persistence, | ||
| communication: Communication, | ||
| ) -> StateDecision: | ||
| data = "InitState2_execute_completed" | ||
| return StateDecision.graceful_complete_workflow(data) | ||
|
|
||
|
|
||
| class StateOptionsWorkflow1(ObjectWorkflow): | ||
| def get_workflow_states(self) -> StateSchema: | ||
| return StateSchema.with_starting_state(InitState1()) | ||
|
|
||
|
|
||
| class StateOptionsWorkflow2(ObjectWorkflow): | ||
| def get_workflow_states(self) -> StateSchema: | ||
| return StateSchema.with_starting_state(InitState2()) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -108,17 +108,6 @@ def _to_idl_state_options( | |
| ) | ||
| if options.wait_until_api_retry_policy is not None: | ||
| res.wait_until_api_retry_policy = options.wait_until_api_retry_policy | ||
| if options.proceed_to_execute_when_wait_until_retry_exhausted is not None: | ||
| if options.proceed_to_execute_when_wait_until_retry_exhausted: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if this is the correct fix here. In the Java SDKs, the property passed in this option is boolean. I believe we should instead change the specification to take a bool value, and then use it here. I'll PM you with a couple links of some of our internal examples, so that you can see how it is currently working in the Java SDK.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know why in the python sdk Long decided to use enum instead of boolean, but the result should be the same. Let me explain why I am removing this code block. First, the correct behavior is already implemented in line 95, so this code block was unnecessarily reassigning the value again. In addition to that, this duplicate code block had a bug. It was using the enum as conditional which was always evaluating to true, so the Since line 95 already assigns this value correctly, the best approach is to delete this duplicate code block.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense 👍 |
||
| res.wait_until_api_failure_policy = ( | ||
| WaitUntilApiFailurePolicy.PROCEED_ON_FAILURE | ||
| ) | ||
| else: | ||
| res.wait_until_api_failure_policy = ( | ||
| WaitUntilApiFailurePolicy.FAIL_WORKFLOW_ON_FAILURE | ||
| ) | ||
|
|
||
| pass | ||
| if options.wait_until_api_timeout_seconds is not None: | ||
| res.wait_until_api_timeout_seconds = options.wait_until_api_timeout_seconds | ||
| if options.execute_api_retry_policy is not None: | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Below are integration tests, actually testing the behavior with running workflow.