Skip to content
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

remove check for nested intrinsic functions #422

Merged
merged 1 commit into from
Feb 17, 2025
Merged

Conversation

shadycuz
Copy link
Member

@shadycuz shadycuz commented Feb 14, 2025

This feature was based on the nested function limitations in the AWS CloudFormation documentation. However, in turns out that the AWS documentation is incorrect and nested functions are allowed in many places where the documentation says they are not. Since this feature was doing more harm than good, it has been removed.

This feature was based on the nested function limitations in the AWS
CloudFormation documentation. However, in turns out that the AWS
documentation is incorrect and nested functions are allowed in many
places where the documentation says they are not. Since this feature
was doing more harm than good, it has been removed.
@shadycuz shadycuz requested a review from dhutchison February 14, 2025 14:42
@shadycuz shadycuz added enhancement New feature or request minor A new feature has been added. labels Feb 14, 2025 — with GitHub Codespaces
Copy link

codecov bot commented Feb 14, 2025

Codecov Report

Attention: Patch coverage is 94.73684% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.58%. Comparing base (f1b9d72) to head (5c96f8f).
Report is 122 commits behind head on develop.

Files with missing lines Patch % Lines
src/cloud_radar/cf/unit/_template.py 90.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #422      +/-   ##
===========================================
- Coverage    95.00%   94.58%   -0.42%     
===========================================
  Files           11       12       +1     
  Lines          760      850      +90     
  Branches       155      160       +5     
===========================================
+ Hits           722      804      +82     
- Misses          21       25       +4     
- Partials        17       21       +4     
Flag Coverage Δ
unit 94.58% <94.73%> (-0.42%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@dhutchison
Copy link
Collaborator

Just to confirm what I'd wondered about in the other thread - it does appear cfn-lint has support for checking intrinsic function nesting now - so def believe people should be relying on the linter for this check instead of this testing library. Example definition is here.

Copy link
Collaborator

@dhutchison dhutchison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me 👍🏻

@dhutchison dhutchison linked an issue Feb 17, 2025 that may be closed by this pull request
@shadycuz shadycuz merged commit 042fa5e into develop Feb 17, 2025
11 checks passed
@shadycuz shadycuz deleted the remove_nested_check branch February 17, 2025 18:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request minor A new feature has been added.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow Fn::Split and Fn::Transform to be nested within Fn::If
2 participants