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.

Sign up for GitHub

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: Return early during linking if no destination resources are found #5220

Merged
merged 6 commits into from
May 31, 2023
Merged

feat: Return early during linking if no destination resources are found #5220

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
127defa
Returns during linking if no destination resources are found
lucashuy May 29, 2023
962f777
Merge branch 'develop' into linking_exit_early
lucashuy May 30, 2023
9ed3e5c
Merge branch 'develop' into linking_exit_early
moelasmar May 31, 2023
1ddaff0
Updated comment to correctly reflect state
lucashuy May 31, 2023
f801862
Cleaned extra word
lucashuy May 31, 2023
9913d78
Merge branch 'develop' into linking_exit_early
lucashuy May 31, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions samcli/hook_packages/terraform/hooks/prepare/resource_linking.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,9 +231,12 @@ def _link_using_terraform_config(self, source_tf_resource: TFResource, cfn_resou
)
if not dest_resources:
LOG.debug(
"There are destination resources defined for for the source resource %s",
"There are destination resources defined for for the source resource %s, skipping linking.",
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 update the log message to be There are no destination resources

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh whoops, will update this

source_tf_resource.full_address,
)

return
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM in general. I am just wondering why having 2 destinations is throwing an error (at line 229 above) but defining none is skipping this step.

Copy link
Contributor Author

@lucashuy lucashuy May 30, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This comment explains it in a little more detail, but essentially we don't want to block the execution of some testing cases where the customer may have a partially defined API (eg. defined gateway, but missing some parts of method or integration).

If they were looking to only test their Lambda function using sam local invoke, blocking here on missing links would require them to fully define an API Gateway to test their Lambda function.

In the case of > 1, using the example from above involving a Lambda function, there could be two (or more) layers references to a function in Terraform, but we can't parse this, so we raise an exception.

Copy link
Contributor

Choose a reason for hiding this comment

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

If this method linking layers to functions and also APIGW methods/paths to functions, should we have distinct ways?

Like different part of our logic can call this function (either linking layers to functions or apigw methods/paths to functions), then this method can do whatever it supposed to do. The caller function then can decide whether the case is valid for that purpose or not. I feel like this method is been overloaded with different purposes, and this might lead to a bug in the future. (Of course there is a chance that I mis-understood everything wrong, in that case please ignore my comment :D )

Copy link
Contributor

Choose a reason for hiding this comment

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

This function is a generic function that we used to link different resources. The logic of what to be linked is maintained here https://github.com/aws/aws-sam-cli/blob/develop/samcli/hook_packages/terraform/hooks/prepare/resources/resource_links.py#L29-L81

Copy link
Contributor

Choose a reason for hiding this comment

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

for your comment regarding the exception we raise in line 229, this is related to the Terraform limitations we have, at this point we could not differentiate if the customer is really linking to 2 different resources, or using some conditional expression to choose one of them. As, we do not know what to be done in this case, shall we use the 2 destination resources, or choose one of them, so, we decided to raise an exception to the customer that we do not support this case, and they can resolve this issue by running terraform apply command, so we can use a different linking approach where we do not have any issues.

But, for no destination resources found at all, this means that there is no destinations at all to be linked, and should not be an issue (this like if the customer does not have any layers defined for a lambda function), so we do not raise exception here. Sometimes, it can be an issue in the customer terraform project, but we decided not to raise the exception here, and leave this decision to each SAM CLI command to decide if it is an issue or not (Like the customer define a Rest API method, but does not link it to a Lambda function)


for cfn_resource in cfn_resources:
self._resource_pair.cfn_resource_update_call_back_function(cfn_resource, dest_resources) # type: ignore

Expand Down Expand Up @@ -284,7 +287,12 @@ def _link_using_linking_fields(self, cfn_resource: Dict) -> None:
else ExistingResourceReference(value)
for value in values
]
LOG.debug("The value of the source resource linking field after mapping $s", dest_resources)

if not dest_resources:
LOG.debug("Skipping linking call back, no destination resources discovered.")
return

LOG.debug("The value of the source resource linking field after mapping %s", dest_resources)
self._resource_pair.cfn_resource_update_call_back_function(cfn_resource, dest_resources) # type: ignore

def _process_resolved_resources(
Expand Down
15 changes: 15 additions & 0 deletions tests/unit/hook_packages/terraform/hooks/prepare/test_resource_linking.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1124,6 +1124,21 @@ def setUp(self) -> None:
linking_exceptions=self.linker_exceptions,
)

def test_applied_empty_destination_skip_call_back(self):
resource_linker = ResourceLinker(self.sample_resource_linking_pair)
resource_linker._link_using_linking_fields({"Properties": {"Layers": []}})

self.sample_resource_linking_pair.cfn_resource_update_call_back_function.assert_not_called()

@patch("samcli.hook_packages.terraform.hooks.prepare.resource_linking._resolve_resource_attribute")
@patch("samcli.hook_packages.terraform.hooks.prepare.resource_linking.ResourceLinker._process_resolved_resources")
def test_config_empty_destination_skip_call_back(self, proccess_resolved_res_mock, resolve_resource_attr_mock):
resource_linker = ResourceLinker(self.sample_resource_linking_pair)
proccess_resolved_res_mock.return_value = []
resource_linker._link_using_terraform_config(Mock(), Mock())

self.sample_resource_linking_pair.cfn_resource_update_call_back_function.assert_not_called()

def test_handle_linking_mix_of_applied_and_non_applied_resources(self):
cfn_resource_depend_on_applied_resources = {
"Type": "AWS::Lambda::Function",
Expand Down