-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Smoke test using several templates from SAR #1357
Conversation
if isinstance(intrinsic, list): | ||
return [self.intrinsic_property_resolver(item) for item in intrinsic] | ||
if not isinstance(intrinsic, dict) or intrinsic == {}: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice change.
@@ -720,7 +722,7 @@ def resolve_sub_attribute(intrinsic_item, symbol_resolver): | |||
) | |||
result = resolve_sub_attribute(sanitized_item, self._symbol_resolver) | |||
sub_str = re.sub( | |||
pattern=r"\$\{" + sub_item + r"\}", string=sub_str, repl=result | |||
pattern=r"\$\{" + sub_item + r"\}", string=sub_str, repl=str(result) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why the str? is the resolved result not a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because Python expects the replacement to be a string. In some cases customers provide an integer value via say a CFN parameter like az number or something.
|
||
with open(template_file_path, "wb") as fp: | ||
r = requests.get(template_url, stream=True) | ||
for chunk in r.iter_content(chunk_size=128): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: could probably do bigger chunks, since I'm assuming the templates are not megabytes in size.
Issue #, if available:
Some of the smoke tests failed due to minor bugs in intrinsic resolution. This PR also fixes those bugs.
Description of changes:
Checklist:
make pr
passesBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.