-
Notifications
You must be signed in to change notification settings - Fork 55
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
Allow for legacy lambda step function steps #1504
Conversation
// not default lambda api | ||
if (step.Resource !== 'arn:aws:states:::lambda:invoke') { | ||
// not default lambda api or legacy lambda definition | ||
if (step.Resource !== 'arn:aws:states:::lambda:invoke' && !step.Resource?.startsWith('arn:aws:lambda')) { |
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.
There's also the callback invocation pattern for invoking the lambda which probably still won't work (If you're thinking of handling that in a separate PR, that's fine too). This is a case where the value for step.Resource
will be arn:aws:states:::lambda:invoke.waitForTaskToken
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.
That's good to know, I hadn't heard of that before. That won't work with this, I'll adjust this PR
Allow context injection for legacy step function steps using lambda. Parameterized many of the existing unit tests to use a legacy and standard definition of the step to assert that the behavior for each of them is the same. JIRA: https://datadoghq.atlassian.net/browse/SVLS-5638
f4c3cba
to
a4e2cc9
Compare
Datadog ReportBranch report: ✅ 0 Failed, 664 Passed, 0 Skipped, 1m 58.07s Total duration (1m 55.8s time saved) |
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 good! 👍
|
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
What and why?
From the JIRA:
Previously, legacy definitions of a lambda step were explicitly denied. This PR allows context injection to happen on step functions using the legacy definitions for lambdas.
How?
There was already a check to not add context when the step function step was using the legacy definition. I removed that check and adjusted the validation that the step was for a lambda.
Review checklist
Commit
Allow context injection for legacy step function steps using lambda. Parameterized many of the existing unit tests to use a legacy and standard definition of the step to assert that the behavior for each of them is the same.
JIRA: https://datadoghq.atlassian.net/browse/SVLS-5638
Open questions I have