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

Adds support for using Fn::If in function policies #988

Merged
merged 7 commits into from
Aug 13, 2019

Conversation

parimaldeshmukh
Copy link
Contributor

Issue #, if available:
#874

Description of changes:
Adds support for using Fn::If in function policies.

Description of how you validated changes:
Unit testing

Checklist:

  • [y] Write/update tests
  • [y] make pr passes
  • [y] Update documentation
  • [n] Verify transformed template deploys and application functions as expected
  • [n] Add/update example to examples/2016-10-31

Implemented according to jlhood's suggestions on Issue 874.

bin/sam-translate.py fails when the !If function resolves to the policy template type.

This seems to be because the PolicyTemplatesForFunctionPlugin class expects a policy template object to be passed to it, but in this case we pass the entire If object. How should I proceed?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov-io
Copy link

codecov-io commented Jun 21, 2019

Codecov Report

Merging #988 into develop will increase coverage by 0.15%.
The diff coverage is 98.55%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #988      +/-   ##
===========================================
+ Coverage    94.71%   94.86%   +0.15%     
===========================================
  Files           69       69              
  Lines         3160     3333     +173     
  Branches       606      651      +45     
===========================================
+ Hits          2993     3162     +169     
- Misses          85       89       +4     
  Partials        82       82
Impacted Files Coverage Δ
...slator/plugins/policies/policy_templates_plugin.py 100% <100%> (ø) ⬆️
samtranslator/model/function_policies.py 100% <100%> (ø) ⬆️
samtranslator/model/intrinsics.py 95.31% <100%> (+0.86%) ⬆️
samtranslator/model/sam_resources.py 95.49% <92.3%> (-0.1%) ⬇️
samtranslator/swagger/swagger.py 95.68% <0%> (-1.98%) ⬇️
samtranslator/model/eventsources/push.py 88.6% <0%> (-0.25%) ⬇️
...lator/plugins/application/serverless_app_plugin.py 83.44% <0%> (ø) ⬆️
samtranslator/model/apigateway.py 97.79% <0%> (+0.06%) ⬆️
samtranslator/model/api/api_generator.py 96.66% <0%> (+3.08%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88df20c...f4101cf. Read the comment docs.

@keetonian keetonian changed the base branch from master to develop June 21, 2019 17:49
Copy link
Contributor

@jlhood jlhood left a comment

Choose a reason for hiding this comment

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

So sorry for the delay on getting back to you on this. Really appreciate the work you've put into this PR so far!

  1. You'll need to add end to end unit tests of the translator (input and expected output templates). Here's an example PR showing what it looks like to add that type of unit test.
  2. Great catch on PolicyTemplatesForFunctionPlugin throwing an error. That class will need to be updated such that it can support the case where the policy passed to it is an Fn::If dictionary. You'll have to recursively traverse that dictionary and find any policy template statements within it and call self._policy_template_processor.convert() on each one of them.

Again, really appreciate the work you've put into this so far! Looking forward to your updates. 😊

samtranslator/model/function_policies.py Outdated Show resolved Hide resolved
samtranslator/model/function_policies.py Outdated Show resolved Hide resolved
samtranslator/model/function_policies.py Outdated Show resolved Hide resolved
samtranslator/model/function_policies.py Outdated Show resolved Hide resolved
samtranslator/model/function_policies.py Outdated Show resolved Hide resolved
@parimaldeshmukh
Copy link
Contributor Author

Added support for intrinsic if to the policy plugin and sam resource. Made suggested changes. Added end to end test. Was able to verify stack launch with the end to end test outputs.

Copy link
Contributor

@keetonian keetonian left a comment

Choose a reason for hiding this comment

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

This looks great. Thank you for the test coverage as well as the feature, this will be very useful!

if is_intrinsic_no_value(else_data):
return if_data_type

raise InvalidTemplateException("Different policy types within the same Fn::If statement is unsupported. "
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great limitation, makes this feature easier and cuts down on edge cases. Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great! Can you merge it in? Looks like its still blocked for me on some "code changes requested".

Copy link
Contributor

@jlhood jlhood left a comment

Choose a reason for hiding this comment

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

Terrific work on this PR! Really appreciate you adding the updates. I called out two super minor variable renames, but I'll go ahead and commit those changes and get this merged in. Thanks so much for this great contribution!

samtranslator/model/intrinsics.py Outdated Show resolved Hide resolved
samtranslator/model/intrinsics.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants