Feature/use s3 uploader for cf package with include transforms #3454
Conversation
…age template by adding a global export function for artifacts that are not specific to a resource type
…steps, rename includes to include
|
#3440 will be closed without merging if this is approved |
Codecov Report
@@ Coverage Diff @@
## develop #3454 +/- ##
===========================================
+ Coverage 94.43% 94.88% +0.45%
===========================================
Files 173 180 +7
Lines 13346 13716 +370
===========================================
+ Hits 12603 13015 +412
+ Misses 743 701 -42
Continue to review full report at Codecov.
|
|
@stealthycoin @sanathkr @jamesls @joguSD would appreciate a review from someone on your end when you get a chance, I'm going on vacation soon and would like to make any updates before then :) this implements the feature requested here |
|
Any update on this work? It would be fantastic to have this working 👍 |
|
@sanathkr Just updated, thank you for reviewing! |
There was a problem hiding this comment.
Looks good to me. Thanks for putting this together! Looks like it will be really helpful. My main comments were related to Python styling. One suggestion I have in general is to run the code that you added (I would not worry about the preexisting code) under flake8 it will help catch issues with Python styling.
| here we iterate through the template dict and export params with a | ||
| handler defined in GLOBAL_EXPORT_DICT | ||
| """ | ||
| for key, val in iter(template_dict.items()): |
There was a problem hiding this comment.
Just a python thing. We can simplify this to:
| for key, val in iter(template_dict.items()): | |
| for key, val in template_dict.items(): |
| template_dict[key] = GLOBAL_EXPORT_DICT[key](val, self.uploader) | ||
| elif type(val) is dict: | ||
| self.export_global_artifacts(val) | ||
| return self.template_dict |
There was a problem hiding this comment.
Is it intentional that we are returning self.template_dict? I would assume it would instead be return template_dict. We probably we do not even need to return anything here because Python dictionaries are mutable so any updates made to the dictionary will reflect in the original dictionary passed in.
There was a problem hiding this comment.
I meant to return just template_dict good catch. I'm aware it's not necessary to return and reassign in export but I decided to since I generally dislike mutating values passed into a method so I added an assignment for a brighter future where this is refactored :)
| for key, val in iter(template_dict.items()): | ||
| if key in GLOBAL_EXPORT_DICT: | ||
| template_dict[key] = GLOBAL_EXPORT_DICT[key](val, self.uploader) | ||
| elif type(val) is dict: |
There was a problem hiding this comment.
We should be able to simplify this to:
| elif type(val) is dict: | |
| elif isinstance(val, dict): |
| if key in GLOBAL_EXPORT_DICT: | ||
| template_dict[key] = GLOBAL_EXPORT_DICT[key](val, self.uploader) | ||
| elif type(val) is dict: | ||
| self.export_global_artifacts(val) |
There was a problem hiding this comment.
Is it possible to have Fn::Transforms in lists as well? We may want to traverse through lists as well if that is the case.
|
@kyleknap thank you for the review! I just updated take a look when you get a chance |
ekcrisp
deleted the
feature/use-s3-uploader-for-CF-package-with-include-transforms
branch
I recently opened a PR which added exports for include transform of inline swagger AWS::Serverless::Api resource. I've reconsidered and decided to add an implementation that will upload AWS::Include from anywhere in a cloud formation template for cloudformation package command. I have seen several discussions where users have asked for this feature, we would like to manage templates for various resources in isolation and also potentially reuse them in different stacks, this feature will help a lot towards this end.
A few notes, I originally considered doing this on yaml parse or dump to avoid a pass through the entire template dictionary, however since JSON is also supported this would have been clunky, since it would be difficult to reuse parse hooks across pyyaml and json. It was impossible to add this without bypassing the existing "resource" export pattern since these can appear anywhere in the document, adding a handler that matched some key seemed reasonable to me, and I don't think we have to worry about cyclic references for yaml_parse return val. Lastly my python is not very strong and I've never used this unit testing framework, open to any suggestions you have
If this PR is approved I will reject and close the other one