-
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
Conversation
| 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: |
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.
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.
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.
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 else: was never reached.
Since line 95 already assigns this value correctly, the best approach is to delete this duplicate code block.
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.
Makes sense 👍
| from iwf.workflow_state_options import WorkflowStateOptions | ||
|
|
||
|
|
||
| class InitState1(WorkflowState[str]): |
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.
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).
|
|
||
| self.assertTrue(isinstance(result.wait_until_api_failure_policy, Unset)) | ||
|
|
||
| def test_proceed_on_failure(self): |
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.
No description provided.