Add root_template_path to nested stack #3237
Conversation
Co-authored-by: _sam <3804518+aahung@users.noreply.github.com>
samcli/commands/_utils/template.py
Outdated
| @@ -29,20 +29,26 @@ class TemplateFailedParsingException(UserException): | |||
| pass | |||
|
|
|||
|
|
|||
| def get_template_data(template_file): | |||
| def get_template_data(template_file, root_template_path: str = ""): | |||
There was a problem hiding this comment.
do we expect root_template_path to be None or ""?
There was a problem hiding this comment.
I think either is fine. Just need to keep it consistent in other places. My understanding is if it is string, we prefer using "".
There was a problem hiding this comment.
Updated it to None since it makes more sense.
samcli/commands/_utils/template.py
Outdated
|
|
||
| Returns | ||
| ------- | ||
| Template data as a dictionary | ||
| """ | ||
|
|
||
| # if template_file is a relative path , we use root_template_path to convert to absolute path | ||
| if not os.path.isabs(template_file) and root_template_path: | ||
| template_file = os.path.join(root_template_path, template_file) |
There was a problem hiding this comment.
It looks to me that we need to get dirname from root_template_path before joining
There was a problem hiding this comment.
Actually the name is misleading. I just updated it to root_template_dir
|
Added an integration test involving using |
samcli/commands/_utils/template.py
Outdated
| # if template_file is a relative path , we use root_template_dir to convert to absolute path | ||
| if not os.path.isabs(template_file) and root_template_dir: | ||
| template_file = os.path.join(root_template_dir, template_file) |
There was a problem hiding this comment.
Is there any way to do this without requiring multiple variables to specify a single path?
e.g. in one location make the path absolute, and pass that around, so that callees just read from the path without caring whether it's absolute or relative
There was a problem hiding this comment.
I think that is possible and useful. It needs a relative larger refactoring since currently we have a few places doing path checking and converting. I'd say it's out of scope for this bug and we can do this in a separate PR.
There was a problem hiding this comment.
Any way to reduce the number of places where these path modifications are done?
It's already somewhat confusing as for example normalize_resource_path already does some path changes, but then further changes are done here.
There was a problem hiding this comment.
Discussed elsewhere: normalize_resource_path no longer converts to absolute path only since a bug was discovered. Agreed to add comment explaining the context.
| template_dict = get_template_data(template_file) | ||
| template_dict = get_template_data(template_file, root_template_dir) |
There was a problem hiding this comment.
Do we want to provide root_template_dir for all usage of get_template_data()? Maybe we can make it not optional
There was a problem hiding this comment.
Currently it is used in a few other places including intrinsics resolver. I prefer not changing that part. Ideally as Chris said, we can get rid of all root_template_dir and convert all paths to absolute in a single place later.
| "template_file", parameter_overrides=None, global_parameter_overrides={"AWS::Region": "my-custom-region"} | ||
| "template_file", | ||
| parameter_overrides=None, | ||
| root_template_dir=os.getcwd(), | ||
| global_parameter_overrides={"AWS::Region": "my-custom-region"}, |
There was a problem hiding this comment.
Can you put a comment at line 1087 explaining a little bit why it is os.getcwd()?
Which issue(s) does this change fix?
#2837
Why is this change necessary?
The current behavior is if
CodeUriis a relative path, we convert it to absolute path when we normalize resource path for nested stacks. We will also try to resolve relative code path to absolute code path when we get invoke config using--docker-volume-basediroption. There is a bug here. If a customer wants to resolve the path using that option, it won't work since CodeUri is already an absolute path so it won't trying to resolve again.To fix this issue, we need to make sure if CodeUri is relative path, we keep it relative path so that it can be resolved using
--docker-volume-basedirproperly.root_template_pathis introduced to make sure relative path is based on root template.How does it address the issue?
Keep relative path when we normalize resource path.
What side effects does this change have?
Checklist
make prpassesmake update-reproducible-reqsif dependencies were changedBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.