-
Notifications
You must be signed in to change notification settings - Fork 171
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
worflows: activity retry policy #644
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #644 +/- ##
==========================================
- Coverage 62.52% 62.38% -0.14%
==========================================
Files 56 56
Lines 4139 4201 +62
==========================================
+ Hits 2588 2621 +33
- Misses 1425 1453 +28
- Partials 126 127 +1 ☔ View full report in Codecov by Sentry. |
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.
lgtm
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.
Looks like a solid implementation of retry policies for activities, just one potential signature refactor to accommodate the future addition of workflow retry policies. Also an ask - would it be possible to implement a simple activity that does continually fail - to test the retry logic?
Signed-off-by: Fabian Martinez <[email protected]>
Signed-off-by: Fabian Martinez <[email protected]>
9539d35
to
006d587
Compare
good to go |
Signed-off-by: Fabian Martinez <[email protected]>
can we merge? |
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 add a test for the activity to retry - I think it's crucial to validate this so we have some visibility if it breaks
Signed-off-by: Fabian Martinez <[email protected]>
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.
Thanks for adding tests, just need to register the new activity
Signed-off-by: Fabian Martinez <[email protected]>
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.
lgtm
Description
first part of #541
also updated one of the existing examples to show how activity retry policies can be used
Issue reference
Please reference the issue this PR will close: #[issue number]
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: