-
Notifications
You must be signed in to change notification settings - Fork 146
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
fix(idempotency): pass lambda context remaining time to save inprogress #1540
fix(idempotency): pass lambda context remaining time to save inprogress #1540
Conversation
@@ -32,6 +32,7 @@ const idempotent = function ( | |||
const functionPayloadtoBeHashed = isFunctionOption(options) | |||
? record[(options as IdempotencyFunctionOptions).dataKeywordArgument] | |||
: record; | |||
|
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.
Even with the current signature (& the comment about future refactoring), can we not do any assertion when idempotentLambdaHandler
is used on the second argument like we do for the fn wrapper?
Like, getting the second argument which might or might not be context (example), and then doing an assertion on its content (example).
Is it worth it? Should we not do it?
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.
Good point, adding args to check the context works. I have added a copy of isContext
, it needs a better place. Exporting would mix wrapper and decorator imports. What do you think?
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.
It's fine for now, we should probably move it to commons
together with the other type guards I'm adding in #1528.
Let's leave this for now.
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 fixing this!
Description of your changes
In this PR we fix the issue to pass lambda context remaining execution time to save in progress. This only applies to function wrapper for now. We need to rework the entire decorator signature to extract the context, this will be handled in a separate PR.
Related issues, RFCs
Issue number: closes #1482
Checklist
Breaking change checklist
Is it a breaking change?: NO
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.