-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Fix #5647: Add support for Fn::ForEach intrinsic function #8298
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: develop
Are you sure you want to change the base?
Conversation
Fixes the 2.5-year-old bug where SAM CLI crashed with AttributeError when processing templates using CloudFormation's Fn::ForEach. Following AWS CLI's approach (aws/aws-cli#8096), we now detect and skip Fn::ForEach constructs during local parsing, letting CloudFormation expand them server-side. Changes: - Added Fn::ForEach to unresolvable intrinsics - Updated resource metadata normalizer to skip ForEach blocks - Added informative logging - Updated providers to handle ForEach gracefully - Added integration tests Testing: - All 5,870 unit tests pass - 94.12% code coverage - Verified with real templates Closes aws#5647
678b731 to
9aa1bf7
Compare
DescriptionFixes #5647 This PR fixes the 2.5-year-old bug where SAM CLI crashed with Following AWS CLI's approach (aws/aws-cli#8096), we now detect and skip Changes
ExampleBefore this fix: sam local invoke --template template-with-foreach.yaml
# Error: AttributeError: 'list' object has no attribute 'get'
# CRASH ❌After this fix: sam local invoke --template template-with-foreach.yaml
# INFO: Detected Fn::ForEach construct 'Fn::ForEach::Topics'.
# This will be expanded by CloudFormation during deployment.
# Function runs successfully ✅Which issue(s) does this change fix?Fixes #5647 Why is this change necessary?SAM CLI has been crashing with The crash affects multiple SAM CLI commands:
This blocks users from using
Impact: Users with ForEach in templates cannot use SAM CLI at all (complete blocker). How does it address the issue?This PR follows the same approach used by AWS CLI (aws/aws-cli#8096): 1. Created
2. Updated
3. Technical Approach:
4. Placeholder Logic:
Result: Templates with ForEach no longer crash, local testing works for non-ForEach resources, deployment works as CloudFormation handles ForEach expansion. What side effects does this change have?Breaking Changes: ❌ None Backward Compatibility: ✅ Fully maintained
Behavior Changes:
Performance Impact: ✅ Minimal (just adds a loop to check resource IDs) Deployment Impact: ✅ None (CloudFormation still expands ForEach as normal) Local Testing Impact:
Mandatory Checklist✅ Completed Items
Testing✅ Verification ResultsTest Suite: New File Coverage: Quality Checks:
✅ Test QualityNo Over-Mocking:
Comprehensive Scenarios:
✅ Manual VerificationTest template from Issue #5647: AWSTemplateFormatVersion: '2010-09-09'
Transform:
- AWS::LanguageExtensions
- AWS::Serverless-2016-10-31
Resources:
'Fn::ForEach::Topics':
- TopicName
- [Success, Failure, Timeout, Unknown]
- 'SnsTopic${TopicName}':
Type: AWS::SNS::TopicBefore fix: ❌ Crashes with AttributeError Implementation DetailsNew File:
|
|
Will this PR be getting some attention soon? It would be highly impactful to our organization's use of CFN to unblock use of the |
|
Hello @rhbecker, thanks for the comment! We are currently pausing reviewing and merging most external PRs due to prioritizing Re:Invent launches. We will be able to look at this PR in a few weeks, thank you for your patience |
Appreciate the responsiveness and the transparency; thanks, @seshubaws! |
|
Hi @dcabib, thank you for taking the time to work on this long-standing issue! I appreciate the thorough PR description and the effort to follow the AWS CLI approach. However, after careful review, I have concerns that this approach may not be suitable for SAM CLI due to fundamental differences in how SAM CLI and AWS CLI process templates. Key Concern: SAM CLI needs to build code, AWS CLI doesn't The AWS CLI fix (PR #8096) works for
By filtering out
Example scenario: Fn::ForEach::Functions:
- [Api, Worker]
- ${Id}Function:
Type: AWS::Serverless::Function
CodeUri: src/
Handler: index.handlerWith this PR, Suggestions for a path forward:
I'd be happy to discuss alternative approaches. Thanks again for your contribution! |
|
Thanks for the detailed review, @bnusunny! You're absolutely right about the fundamental difference - I was focused on fixing the crash but didn't fully consider the build/invoke workflow implications. Your point about functions inside Looking at your three suggestions, here's my take: Option 1 (Clear error) seems like the most honest approach right now. If SAM can't properly handle ForEach constructs yet, failing fast with a helpful message is better than silently creating broken builds. Something like: Option 2 (Local expansion) would be the proper solution long-term. Implementing ForEach in Option 3 (Skip with warnings) feels like a middle ground but might be worse than option 1 - partial success that breaks later is confusing for users. What do you think makes most sense as next steps? Should I:
Also curious - do you have a sense of how complex option 2 would be? I'm willing to dig into it if there's appetite for a proper implementation. |
Fixes the 2.5-year-old bug where SAM CLI crashed with AttributeError when processing templates using CloudFormation's Fn::ForEach.
Following AWS CLI's approach (aws/aws-cli#8096), we now detect and skip Fn::ForEach constructs during local parsing, letting CloudFormation expand them server-side.
Changes:
Testing:
Closes #5647
Which issue(s) does this change fix?
Why is this change necessary?
How does it address the issue?
What side effects does this change have?
Mandatory Checklist
PRs will only be reviewed after checklist is complete
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.