Fix/adjust function nesting #417
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #417 +/- ##
===========================================
- Coverage 95.00% 94.85% -0.15%
===========================================
Files 11 12 +1
Lines 760 856 +96
Branches 155 161 +6
===========================================
+ Hits 722 812 +90
- Misses 21 24 +3
- Partials 17 20 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| if key == "Fn::If": | ||
| # Need a special case for this one as the first parameter uses | ||
| # different allowed function rules than the other two parameters. | ||
| # There is probably a cleaner way to do this! | ||
| value = [ | ||
| # the condition function | ||
| self.resolve_values(value[0], functions.ALLOWED_FUNCTIONS[key]), | ||
| self.resolve_values(value[1], functions.INTRINSICS), | ||
| self.resolve_values(value[2], functions.INTRINSICS), | ||
| ] | ||
| else: | ||
| value = self.resolve_values( | ||
| value, | ||
| functions.ALLOWED_FUNCTIONS[key], | ||
| ) |
There was a problem hiding this comment.
@dhutchison Do we need a code change for this? It seems like just adding transform and split to be allowed nested under If would be the better solution?
The allowed functions list came from the AWS docs and unfortunately they are VERY incorrect. People have opened up plenty of issues over the years with working AWS provided templates that go against what AWS docs state is allowed.
There was a problem hiding this comment.
Ah - I was going on the assumption that the docs were actually correct and only applied to the condition part.
If that's not right then yeah that would be a simpler solution. I think ideally we wouldn't be checking functions at all and saying to rely on a linter but don't know off the top of my head if cfn-lint would catch it (I'll check when I'm back).
There was a problem hiding this comment.
I mean its a pretty good point. The Allowed functions was hard to implement and costly to maintain. I think 90% of issues opened are about allowed function error on a template that was provided by AWS and deploys fine.
I guess the worse case is you implement a nested function that doesn't work? Unit test it but it fails to deploy. I have done a lot of Cloudformation but not sure I ever ran into that issue. Maybe once or twice.
I think I will open an MR that tears out the allowed functions feature and we can look it over.
There was a problem hiding this comment.
Yeah sounds like a plan. I can't say I remember seeing any cases myself either
|
Closing this as superseded by removing the checks entirely in #422 |
Fixes #406
This changes specifically how
Fn::Ifis handled so the nested function rule is applied to the condition only.