-
Notifications
You must be signed in to change notification settings - Fork 4.3k
feat(stepfunctions-tasks): add architecture support to EvaluateExpression #35468
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
base: main
Are you sure you want to change the base?
Conversation
…sion - Add optional architecture parameter to EvaluateExpressionProps - Support ARM_64 and X86_64 Lambda architectures - Add comprehensive unit and integration tests - Update documentation with usage examples Closes aws#34974
- Updated snapshots for evaluate-expression tests (arm64, x86, default) - Added Architectures property to Lambda functions - Updated runtime mapping and CDK metadata
The snapshots have been updated to match the current code state which no longer generates CDKMetadata resources and conditions. This resolves the destructive changes detected in the integration tests.
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.
Thank you for your contribution! Just a quick question on this: I see that the props that you are adding this to already has once optional field runtime
. Are there combinations of architecture
and runtime
that are incompatible? I also notice that you didn't set both in any of the test cases you created. I assume that both can be set at the same time, though? We want to be careful about adding fields that are either mutually exclusive or that have incompatibilities that may result in deployment time failures.
I tried to take a quick look into this and came across this page that mentions that ARM_64 isn't available in some regions so we would definitely want a docstring comment added letting users know that so they don't try to use this option in regions where it's not yet available. Those error messages aren't always super clear so it can cause confusion for customers.
One last note: thank you for adding thorough integ tests here!
…86 and ARM64 EvaluateExpression constructs - Add separate nodeJsArmGuids map with dedicated UUIDs for ARM64 architecture - Add architecture-specific GUID selection logic to prevent resource name clashes - Add ARM64 region support warning to architecture property documentation - Add integ.evaluate-expression-mixed-arch test demonstrating mixed architecture usage - Update ARM64 integration test snapshots to use new ARM-specific UUIDs - Maintain full backward compatibility for existing X86 deployments This resolves the issue where using both X86 and ARM64 EvaluateExpression constructs in the same stack would cause resource name collisions due to identical UUIDs.
Pull request has been modified.
Thanks for the comments. I answered your questions below and I also implemented a new change which I would like you to review if possible.
To resolve this, I created a separate UUID map specifically for ARM functions ( I did this to maintain backward compatibility with existing
Please let me know if you have any thoughts on these additions. Thanks! References |
Issue # (if applicable)
Closes #34974
Reason for this change
Adding an optional architecture parameter to EvaluateExpressionProps which allows users to specify their desired architecture when using the EvaluateExpression construct. This enables support for ARM64 Lambda functions, which can provide better price-performance for certain workloads.
Description of changes
Describe any new or updated permissions being added
No new or updated IAM permissions are required. The existing Lambda invoke permissions remain unchanged as the architecture parameter only affects the Lambda function configuration, not the required permissions.
Description of how you validated changes
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license