feat(invoke): reading Metadata adding to Resources in a template #907
Conversation
|
As an FYI: @sooddhruv @eladb @fulghum |
|
|
||
| ```python | ||
|
|
||
| class CdkTemplateNormalizer(object): |
There was a problem hiding this comment.
Thinking about this more. I think this should be a metadata normalized instead of tying it to CDK. CDK happens to be the first to add this type of metadata but nothing here forces it to be from CDK.
I will work on removing references to that directly and center the design around the Metadata instead. I will also add a snippet of the Metadata on a Resource (slipped my mind when initially writing)
There was a problem hiding this comment.
+1 we designed the metadata keys (aws:asset:*) to not be tied directly to cdk with the purpose of allowing other frameworks to utilize the same approach.
| Below algorithm to do this Metadata Normalization on the template. | ||
|
|
||
| ```python | ||
| class TemplateMetadataNormalizer(object): |
There was a problem hiding this comment.
Is this normalizer going to be injected? can we have more than one normalizer such that we can chain them?
There was a problem hiding this comment.
This will not be injected. The Method is static. I wanted to keep this out of the SamBaseProvider class, so it can make the decision on when/where to run this in the flow of getting the template_dict. It is possible to have more than one normalizer but they could happen at different times. For now, I elected to keep it simple and just make the method call to this class where we needed.
| ``` | ||
|
|
||
| The two keys we will recognize are `aws:asset:path` and `aws:asset:property`. `aws:asset:path`'s value will be the path | ||
| to the code, files, etc that are help on the machine, while `aws:asset:property` is the Property of the Resource that |
There was a problem hiding this comment.
Not sure how 'help' got into that sentence.
jfuss
changed the title
| } | ||
| } | ||
|
|
||
| ResourceMetadataNormalizer.normalize(template_data) |
There was a problem hiding this comment.
Shouldn’t that be an error? Or at least a warning
There was a problem hiding this comment.
This is done on the full template, which means a resource not being invoked could fail the invoke of another and don't think that is the correct UX. Maybe there are cases like start-api or start-lambda that could require this to be a failure but the CLI doesn't have any distinctions about which command is run.
A warning seems reasonable.
| if property_key and property_value: | ||
| resource.get(PROPERTIES_KEY, {})[property_key] = property_value | ||
| elif property_key or property_value: | ||
| LOG.info("WARNING: Ignoring Metadata for Resource %s. Metadata contains only aws:asset:path or " |
There was a problem hiding this comment.
That will not get logged to console by default (due to how we currently have logging setup) and the pattern we have followed thus far is using LOG.info instead. There is a bigger story here on our logging (including adding colors to make reading easier) but that is for another day.
|
I dont see any major concerns, looks good to me. 👍 |
💻 Rendered Design Doc is here
🗓 RFC Open for feedback until: 1/10/2018
Support Templates written through CDK through reading Resource Level Metadata #893
Description of changes:
This design is meant to capture how we can directly support templates that are embedded with Metadata CDK appended into the template. This will allow customers to synthesize templates written in CDK and locally debug/invoke their functions through SAM CLI. While understanding CDK Metadata is the only thing in scope for this design, it is left open to allow further support and expansion (most likely through another command) that can generate the AWS CloudFormation templates directly within SAM CLI.
Checklist:
make prpassesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.