feat: additional validation and bin/sam-translate.py update #873
Conversation
brettstack
commented
Mar 26, 2019
•
brettstack
commented
- Add additional commands
- Update parameters to be consistent with SAM CLI
- Add additional parameters
- Improve logging
- Fixes
* Add additional commands * Update parameters to be consistent with SAM CLI * Add additional parameters * Improve logging * Fixes
Codecov Report
@@ Coverage Diff @@
## develop #873 +/- ##
===========================================
- Coverage 94.66% 94.62% -0.04%
===========================================
Files 69 69
Lines 2978 2997 +19
Branches 547 554 +7
===========================================
+ Hits 2819 2836 +17
- Misses 84 85 +1
- Partials 75 76 +1
Continue to review full report at Codecov.
|
Previously this was raising ValueError
There was a problem hiding this comment.
Key question that comes up looking at this is why are we writing this when SAM CLI exists? Had a discussion on it and determined that installing SAM CLI and pointing it towards the samtranslator repo adds yet more overhead for contributors to get setup. So this lightweight CLI tool adds value.
One minor comment to confirm you've tested the latest changes, but otherwise, looks good!
brettstack
requested review from
jlhood and
keetonian
and removed request for
jlhood
| if not app_id: | ||
| raise InvalidResourceException(logical_id, "Property 'ApplicationId' cannot be blank.") | ||
|
|
||
| if isinstance(app_id, dict): | ||
| raise InvalidResourceException(logical_id, "Property 'ApplicationId' cannot be resolved. Only FindInMap " |
There was a problem hiding this comment.
The error messages for both of these instances are practically the same, any way we could re-use the verbage and insert the property name?
| semver = self._replace_value(app.properties[self.LOCATION_KEY], | ||
| self.SEMANTIC_VERSION_KEY, intrinsic_resolvers) | ||
|
|
||
| if isinstance(app_id, dict) or isinstance(semver, dict): | ||
| key = (json.dumps(app_id), json.dumps(semver)) | ||
| self._applications[key] = False |
There was a problem hiding this comment.
Why did you choose to set this to False?
There was a problem hiding this comment.
Just needed a value here. Wanted to get any other recommendations from you.
There was a problem hiding this comment.
Ok. We're getting a value here because we don't resolve intrinsics until later, right? Could we resolve them earlier so that the flow in this area remains the same?
There was a problem hiding this comment.
I think you can actually stick an error instead of False into self._applications[key]. If the app_id or semver is still a dict, we won't resolve it any further so we'll just have to return an error at some point.
tests/translator/input/error_function_invalid_autopublishalias.yaml
Outdated
Show resolved
Hide resolved
keetonian
dismissed
jlhood’s stale review
Dismissing review- there are new changes that need to be reviewed since this review was done.
brettstack
changed the title
