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

feat: Added module output resolving #4259

Merged
merged 8 commits into from
Sep 30, 2022
Merged
53 changes: 51 additions & 2 deletions samcli/hook_packages/terraform/hooks/prepare/resource_linking.py
Original file line number Diff line number Diff line change
Expand Up @@ -99,13 +99,62 @@ def _get_configuration_address(address: str) -> str:
Cleans all addresses of indices and returns a clean address

Parameters
==========
----------
address : str
The address to clean

Returns
=======
-------
str
The address clean of indices
"""
return re.sub(r"\[[^\[\]]*\]", "", address)


def _resolve_module_output(module: TFModule, output_name: str) -> List[Union[ConstantValue, ResolvedReference]]:
"""
Resolves any references in the output section of the module

Parameters
----------
module : Module
The module with outputs to search
output_name : str
The value to resolve

Returns
-------
List[Union[ConstantValue, ResolvedReference]]
A list of resolved values
"""
results: List[Union[ConstantValue, ResolvedReference]] = []

output = module.outputs[output_name]
output_value = output.value

if isinstance(output, ConstantValue):
results.append(output)
elif isinstance(output, References):
cleaned_references = _clean_references_list(output_value)

for reference in cleaned_references:
if reference.startswith("var."):
stripped_reference = _get_configuration_address(reference[4:])
Copy link
Contributor

Choose a reason for hiding this comment

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

anytime absolute numbers are used one has to be careful, would len("var.") be better?

Also if we are to use multiple if else (in this case its 3 (if, elif, else)), it might be better to use a dispatcher pattern. eg: https://www.oreilly.com/library/view/python-cookbook/0596001673/ch01s07.html

Copy link
Contributor Author

@lucashuy lucashuy Sep 28, 2022

Choose a reason for hiding this comment

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

Yup, changing the magic number to len("var.") will be nicer. Might also consider using reference[reference.find("."):] to be inline with the other if case that uses rfind()

Will look into the dispatcher pattern.

Copy link
Contributor Author

@lucashuy lucashuy Sep 28, 2022

Choose a reason for hiding this comment

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

So if we were to go with a dispatching design, we would still need some way to handle the else case. Since Python doesn't have a switch-like statement until 3.10, we would need to do some checking if var or module was in the dictionary and throw all the rest of the cases into a default function/branch. From what I can make out, it would involve if or try/except to catch a KeyError for "default" cases.

results += _resolve_module_variable(module, stripped_reference)
elif reference.startswith("module."):
# aaa.bbb.ccc => bbb
module_name = reference[7 : reference.rfind(".")]
# aaa.bbb.ccc => ccc
output_name = reference[reference.rfind(".") + 1 :]

stripped_reference = _get_configuration_address(module_name)

results += _resolve_module_output(module.child_modules[stripped_reference], output_name)
else:
results.append(ResolvedReference(reference, module.full_address or ""))

return results


def _resolve_module_variable(module: TFModule, variable: str):
pass
Original file line number Diff line number Diff line change
@@ -1,16 +1,17 @@
from copy import deepcopy
from unittest import TestCase
from unittest.mock import Mock
from unittest.mock import Mock, patch

from parameterized import parameterized

from samcli.hook_packages.terraform.hooks.prepare.resource_linking import (
_clean_references_list,
_get_configuration_address,
_resolve_module_output,
TFModule,
TFResource,
ConstantValue,
References,
ConstantValue,
)


Expand Down Expand Up @@ -131,3 +132,41 @@ def test_resource_full_address_root_module(self):
module = TFModule(None, None, {}, {}, {}, {})
resource = TFResource("resource_address", "type", module, {})
self.assertEqual(resource.full_address, "resource_address")

@patch("samcli.hook_packages.terraform.hooks.prepare.resource_linking._resolve_module_variable")
@patch("samcli.hook_packages.terraform.hooks.prepare.resource_linking._get_configuration_address")
def test_resolve_module_output_with_var(self, config_mock, resolve_var_mock):
module = TFModule("", None, {"mycoolref": "mycoolvar"}, [], {}, {"mycooloutput": References(["var.mycoolref"])})

config_mock.return_value = "mycoolref"

_resolve_module_output(module, "mycooloutput")

config_mock.assert_called_with("mycoolref")
resolve_var_mock.assert_called_with(module, "mycoolref")

@patch("samcli.hook_packages.terraform.hooks.prepare.resource_linking._get_configuration_address")
def test_resolve_module_output_with_module(self, config_mock):
module = TFModule("", None, {}, [], {}, {"mycooloutput": References(["module.mycoolmod"])})
module2 = TFModule("module.mycoolmod", module, {}, [], {}, {"mycoolmod": ConstantValue("mycoolconst")})
module.child_modules.update({"mycoolmod": module2})

config_mock.return_value = "mycoolmod"

results = _resolve_module_output(module, "mycooloutput")

self.assertEqual(len(results), 1)
self.assertEqual(results[0].value, "mycoolconst")

@parameterized.expand(
[
(TFModule("", None, {}, [], {}, {"mycooloutput": ConstantValue("mycoolconst")}),),
(TFModule("", None, {}, [], {}, {"mycooloutput": References(["mycoolconst"])}),),
]
)
@patch("samcli.hook_packages.terraform.hooks.prepare.resource_linking._get_configuration_address")
def test_resolve_module_output_already_resolved(self, module, config_mock):
results = _resolve_module_output(module, "mycooloutput")

self.assertEqual(len(results), 1)
self.assertEqual(results[0].value, "mycoolconst")