Skip to content
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

Add root_template_path to nested stack #3237

Merged
merged 17 commits into from
Sep 10, 2021
Merged
9 changes: 8 additions & 1 deletion samcli/commands/_utils/template.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
import os
import pathlib

from typing import Optional
import jmespath
import yaml
from botocore.utils import set_value_from_jmespath
Expand All @@ -29,20 +30,26 @@ class TemplateFailedParsingException(UserException):
pass


def get_template_data(template_file):
def get_template_data(template_file, root_template_dir: Optional[str] = None):
"""
Read the template file, parse it as JSON/YAML and return the template as a dictionary.

Parameters
----------
template_file : string
Path to the template to read
root_template_dir: string
Optional directory of the root SAM Template

Returns
-------
Template data as a dictionary
"""

# 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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Discussed elsewhere: normalize_resource_path no longer converts to absolute path only since a bug was discovered. Agreed to add comment explaining the context.


if not pathlib.Path(template_file).exists():
raise TemplateNotFoundException("Template file not found at {}".format(template_file))

Expand Down
2 changes: 2 additions & 0 deletions samcli/commands/local/cli_common/invoke_context.py
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,8 @@ def _get_stacks(self) -> List[Stack]:
self._template_file,
parameter_overrides=self._parameter_overrides,
global_parameter_overrides=self._global_parameter_overrides,
# root_template_dir will be used to convert relative path to absolute path
root_template_dir=os.path.dirname(os.path.abspath(self._template_file)),
)
return stacks
except (TemplateNotFoundException, TemplateFailedParsingException) as ex:
Expand Down
46 changes: 36 additions & 10 deletions samcli/lib/providers/sam_stack_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ def __init__(
template_dict: Dict,
parameter_overrides: Optional[Dict] = None,
global_parameter_overrides: Optional[Dict] = None,
root_template_dir: Optional[str] = None,
):
"""
Initialize the class with SAM template data. The SAM template passed to this provider is assumed
Expand All @@ -43,6 +44,7 @@ def __init__(
to get substituted within the template
:param dict global_parameter_overrides: Optional dictionary of values for SAM template global parameters that
might want to get substituted within the template and all its child templates
:param str root_template_dir: Optional directory of root SAM Template.
"""

self._template_file = template_file
Expand All @@ -53,6 +55,7 @@ def __init__(
)
self._resources = self._template_dict.get("Resources", {})
self._global_parameter_overrides = global_parameter_overrides
self._root_template_dir = root_template_dir

LOG.debug("%d stacks found in the template", len(self._resources))

Expand Down Expand Up @@ -109,11 +112,19 @@ def _extract_stacks(self) -> None:
try:
if resource_type == SamLocalStackProvider.SERVERLESS_APPLICATION:
stack = SamLocalStackProvider._convert_sam_application_resource(
self._template_file, self._stack_path, name, resource_properties
self._template_file,
self._stack_path,
name,
resource_properties,
root_template_dir=self._root_template_dir,
)
if resource_type == SamLocalStackProvider.CLOUDFORMATION_STACK:
stack = SamLocalStackProvider._convert_cfn_stack_resource(
self._template_file, self._stack_path, name, resource_properties
self._template_file,
self._stack_path,
name,
resource_properties,
root_template_dir=self._root_template_dir,
)
except RemoteStackLocationNotSupported:
self.remote_stack_full_paths.append(get_full_path(self._stack_path, name))
Expand All @@ -129,6 +140,7 @@ def _convert_sam_application_resource(
stack_path: str,
name: str,
resource_properties: Dict,
root_template_dir: Optional[str] = None,
global_parameter_overrides: Optional[Dict] = None,
) -> Optional[Stack]:
location = resource_properties.get("Location")
Expand All @@ -142,7 +154,7 @@ def _convert_sam_application_resource(
if location.startswith("file://"):
location = unquote(urlparse(location).path)
else:
location = SamLocalStackProvider.normalize_resource_path(template_file, location)
location = SamLocalStackProvider.normalize_resource_path(template_file, location, root_template_dir)

return Stack(
parent_stack_path=stack_path,
Expand All @@ -151,7 +163,7 @@ def _convert_sam_application_resource(
parameters=SamLocalStackProvider.merge_parameter_overrides(
resource_properties.get("Parameters", {}), global_parameter_overrides
),
template_dict=get_template_data(location),
template_dict=get_template_data(location, root_template_dir),
)

@staticmethod
Expand All @@ -160,6 +172,7 @@ def _convert_cfn_stack_resource(
stack_path: str,
name: str,
resource_properties: Dict,
root_template_dir: Optional[str] = None,
global_parameter_overrides: Optional[Dict] = None,
) -> Optional[Stack]:
template_url = resource_properties.get("TemplateURL")
Expand All @@ -175,7 +188,7 @@ def _convert_cfn_stack_resource(
if template_url.startswith("file://"):
template_url = unquote(urlparse(template_url).path)
else:
template_url = SamLocalStackProvider.normalize_resource_path(template_file, template_url)
template_url = SamLocalStackProvider.normalize_resource_path(template_file, template_url, root_template_dir)

return Stack(
parent_stack_path=stack_path,
Expand All @@ -184,7 +197,7 @@ def _convert_cfn_stack_resource(
parameters=SamLocalStackProvider.merge_parameter_overrides(
resource_properties.get("Parameters", {}), global_parameter_overrides
),
template_dict=get_template_data(template_url),
template_dict=get_template_data(template_url, root_template_dir),
)

@staticmethod
Expand All @@ -194,6 +207,7 @@ def get_stacks(
name: str = "",
parameter_overrides: Optional[Dict] = None,
global_parameter_overrides: Optional[Dict] = None,
root_template_dir: Optional[str] = None,
) -> Tuple[List[Stack], List[str]]:
"""
Recursively extract stacks from a template file.
Expand All @@ -212,15 +226,16 @@ def get_stacks(
global_parameter_overrides: Optional[Dict]
Optional dictionary of values for SAM template global parameters
that might want to get substituted within the template and its child templates

root_template_dir: str
Optional directory of root SAM Template. By default it is None unless we pass in a value.
Returns
-------
stacks: List[Stack]
The list of stacks extracted from template_file
remote_stack_full_paths : List[str]
The list of full paths of detected remote stacks
"""
template_dict = get_template_data(template_file)
template_dict = get_template_data(template_file, root_template_dir)
Comment on lines -223 to +238
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to provide root_template_dir for all usage of get_template_data()? Maybe we can make it not optional

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

stacks = [
Stack(
stack_path,
Expand All @@ -233,7 +248,12 @@ def get_stacks(
remote_stack_full_paths: List[str] = []

current = SamLocalStackProvider(
template_file, stack_path, template_dict, parameter_overrides, global_parameter_overrides
template_file,
stack_path,
template_dict,
parameter_overrides,
global_parameter_overrides,
root_template_dir,
)
remote_stack_full_paths.extend(current.remote_stack_full_paths)

Expand All @@ -244,6 +264,7 @@ def get_stacks(
child_stack.name,
child_stack.parameters,
global_parameter_overrides,
root_template_dir=root_template_dir,
)
stacks.extend(stacks_in_child)
remote_stack_full_paths.extend(remote_stack_full_paths_in_child)
Expand Down Expand Up @@ -290,7 +311,7 @@ def merge_parameter_overrides(
return merged_parameter_overrides

@staticmethod
def normalize_resource_path(stack_file_path: str, path: str) -> str:
def normalize_resource_path(stack_file_path: str, path: str, root_template_dir: Optional[str] = None) -> str:
"""
Convert resource paths found in nested stack to ones resolvable from root stack.
For example,
Expand Down Expand Up @@ -327,6 +348,8 @@ def normalize_resource_path(stack_file_path: str, path: str) -> str:
The file path of the stack containing the resource
path
the raw path read from the template dict
root_template_dir
Optional directory of root SAM Template

Returns
-------
Expand All @@ -344,4 +367,7 @@ def normalize_resource_path(stack_file_path: str, path: str) -> str:
# absolute paths are not robust as relative paths. So here prefer to use relative path.
stack_file_path = os.path.relpath(os.path.realpath(stack_file_path))

if root_template_dir:
return os.path.relpath(os.path.join(os.path.dirname(stack_file_path), path), root_template_dir)

return os.path.normpath(os.path.join(os.path.dirname(stack_file_path), path))
5 changes: 5 additions & 0 deletions tests/integration/local/invoke/invoke_integ_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ def setUpClass(cls):
cls.event_path = str(cls.test_data_path.joinpath("invoke", "event.json"))
cls.event_utf8_path = str(cls.test_data_path.joinpath("invoke", "event_utf8.json"))
cls.env_var_path = str(cls.test_data_path.joinpath("invoke", "vars.json"))
cls.docker_volume_basedir = str(cls.test_data_path.joinpath("invoke", "nested-templates"))

@staticmethod
def get_integ_dir():
Expand All @@ -46,6 +47,7 @@ def get_command_list(
profile=None,
layer_cache=None,
docker_network=None,
docker_volume_basedir=None,
):
command_list = [self.cmd, "local", "invoke", function_to_invoke]

Expand Down Expand Up @@ -79,6 +81,9 @@ def get_command_list(
if region:
command_list = command_list + ["--region", region]

if docker_volume_basedir:
command_list = command_list + ["--docker-volume-basedir", docker_volume_basedir]

return command_list

def get_build_command_list(
Expand Down
19 changes: 19 additions & 0 deletions tests/integration/local/invoke/test_integrations_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,25 @@ def test_invoke_returns_expected_results_from_git_function_with_parameters(self)
process_stdout = stdout.strip()
self.assertEqual(process_stdout.decode("utf-8"), '"git init passed"')

@pytest.mark.flaky(reruns=3)
def test_invoke_with_docker_volume_basedir(self):
command_list = self.get_command_list(
"HelloWorldServerlessFunction",
template_path=self.template_path,
event_path=self.event_path,
docker_volume_basedir=self.docker_volume_basedir,
)

process = Popen(command_list, stdout=PIPE)
try:
stdout, _ = process.communicate(timeout=TIMEOUT)
except TimeoutExpired:
process.kill()
raise

process_stdout = stdout.strip()
self.assertEqual(process_stdout.decode("utf-8"), '"Hello world"')


class TestSamInstrinsicsAndPlugins(InvokeIntegBase):
template = Path("template-pseudo-params.yaml")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1082,5 +1082,8 @@ def test_must_pass_custom_region(self, get_stacks_mock):
invoke_context = InvokeContext("template_file", aws_region="my-custom-region")
invoke_context._get_stacks()
get_stacks_mock.assert_called_with(
"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"},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you put a comment at line 1087 explaining a little bit why it is os.getcwd()?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

)
16 changes: 7 additions & 9 deletions tests/unit/commands/local/lib/test_stack_provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ def test_sam_nested_stack_should_be_extracted(
}
}
}
self.get_template_data_mock.side_effect = lambda t: {
self.get_template_data_mock.side_effect = lambda t, m: {
self.template_file: template,
child_location_path: LEAF_TEMPLATE,
}.get(t)
Expand All @@ -59,6 +59,7 @@ def test_sam_nested_stack_should_be_extracted(
"",
"",
parameter_overrides=None,
root_template_dir=os.path.dirname(self.template_file),
)
self.assertListEqual(
stacks,
Expand Down Expand Up @@ -88,16 +89,13 @@ def test_sam_deep_nested_stack(self):
}
}
}
self.get_template_data_mock.side_effect = lambda t: {
self.get_template_data_mock.side_effect = lambda t, m: {
self.template_file: template,
child_template_file: child_template,
grand_child_template_file: LEAF_TEMPLATE,
}.get(t)
stacks, remote_stack_full_paths = SamLocalStackProvider.get_stacks(
self.template_file,
"",
"",
parameter_overrides=None,
self.template_file, "", "", parameter_overrides=None, root_template_dir=os.path.dirname(self.template_file)
)
self.assertListEqual(
stacks,
Expand All @@ -119,7 +117,7 @@ def test_remote_stack_is_skipped(self, resource_type, location_property_name):
}
}
}
self.get_template_data_mock.side_effect = lambda t: {
self.get_template_data_mock.side_effect = lambda t, m: {
self.template_file: template,
}.get(t)
stacks, remote_stack_full_paths = SamLocalStackProvider.get_stacks(
Expand Down Expand Up @@ -158,7 +156,7 @@ def test_sam_nested_stack_template_path_can_be_resolved_if_root_template_is_not_
}
}
}
self.get_template_data_mock.side_effect = lambda t: {
self.get_template_data_mock.side_effect = lambda t, m: {
template_file: template,
child_location_path: LEAF_TEMPLATE,
}.get(t)
Expand Down Expand Up @@ -199,7 +197,7 @@ def test_global_parameter_overrides_can_be_passed_to_child_stacks(
}
}
}
self.get_template_data_mock.side_effect = lambda t: {
self.get_template_data_mock.side_effect = lambda t, m: {
template_file: template,
child_location_path: LEAF_TEMPLATE,
}.get(t)
Expand Down