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

Allow "aws cloudformation package" to succeed when a Resource is not a key-value pair #8096

Merged
merged 17 commits into from
Dec 14, 2023
+176 −3
Merged

Allow "aws cloudformation package" to succeed when a Resource is not a key-value pair #8096

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
2337581
Allow "aws cloudformation package" to succeed when a Resource is not …
arthurboghossian Aug 11, 2023
1e80751
Merge branch 'aws:develop' into develop
arthurboghossian Aug 12, 2023
307056b
Add warning message that local artifacts aren't supported in Resource…
arthurboghossian Aug 15, 2023
f7dcbb8
Merge branch 'aws:develop' into develop
arthurboghossian Aug 15, 2023
db8d728
Modify artifact_explorer to be able to handle the Fn::ForEach intrins…
arthurboghossian Sep 1, 2023
d23436f
Merge branch 'aws:develop' into develop
arthurboghossian Sep 1, 2023
1e84495
Move export_resources method into the Template class
arthurboghossian Sep 5, 2023
8a9bab0
Merge branch 'aws:develop' into develop
arthurboghossian Sep 5, 2023
aeac7d1
Merge branch 'aws:develop' into develop
arthurboghossian Oct 11, 2023
b9ef4e0
Merge branch 'aws:develop' into develop
arthurboghossian Nov 2, 2023
10ae676
Create changelog entry, define and use exception, and modify test to …
arthurboghossian Nov 2, 2023
2a473b3
Modify Fn::ForEach syntax definition to be stricter
arthurboghossian Nov 17, 2023
2584490
Merge branch 'aws:develop' into develop
arthurboghossian Nov 17, 2023
f171389
Merge branch 'aws:develop' into develop
arthurboghossian Nov 28, 2023
75cc5d7
Merge branch 'aws:develop' into develop
arthurboghossian Dec 5, 2023
8f3be23
Remove dict type check on 3rd parameter of Fn::ForEach intrinsic func…
arthurboghossian Dec 11, 2023
7e7b8c2
Merge branch 'aws:develop' into develop
arthurboghossian Dec 11, 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
18 changes: 15 additions & 3 deletions awscli/customizations/cloudformation/artifact_exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -659,9 +659,23 @@ def export(self):

self.template_dict = self.export_global_artifacts(self.template_dict)

for resource_id, resource in self.template_dict["Resources"].items():
self.export_resources(self.template_dict["Resources"])

return self.template_dict

def export_resources(self, resource_dict):
for resource_id, resource in resource_dict.items():

if resource_id.startswith("Fn::ForEach"):
if not isinstance(resource, list) or len(resource) < 3:
raise ValueError("Fn::ForEach with name {0} has invalid format".format(resource_id))
if isinstance(resource[2], dict):
self.export_resources(resource[2])
continue

resource_type = resource.get("Type", None)
if resource_type is None:
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 a particular reason this if check was added for the Type? A sample CFN template snippet would help illustrate what it is addressing.

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 had added this (removed it from the latest commit) to prevent the need to go through the items in self.resources_to_export if the resource_type resolves to None (see loop below)

            resource_type = resource.get("Type", None)
            # ...
            for exporter_class in self.resources_to_export:
                if exporter_class.RESOURCE_TYPE != resource_type:
                    continue
                # ...

However, looking at the Resource class, if RESOURCE_TYPE is not defined for a subclass of a Resource, it would also resolve to None, thought I don't think we'd every have such a subclass, but it is possible, so opting to not have this change as part of this PR

continue
resource_dict = resource.get("Properties", None)

for exporter_class in self.resources_to_export:
Expand All @@ -671,5 +685,3 @@ def export(self):
# Export code resources
exporter = exporter_class(self.uploader)
exporter.export(resource_id, resource_dict, self.template_dir)

return self.template_dict
145 changes: 145 additions & 0 deletions tests/unit/customizations/cloudformation/test_artifact_exporter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1016,6 +1016,151 @@ def test_template_export(self, yaml_parse_mock):
resource_type2_instance.export.assert_called_once_with(
"Resource2", mock.ANY, template_dir)

@mock.patch("awscli.customizations.cloudformation.artifact_exporter.yaml_parse")
def test_template_export_foreach_valid(self, yaml_parse_mock):
parent_dir = os.path.sep
template_dir = os.path.join(parent_dir, 'foo', 'bar')
template_path = os.path.join(template_dir, 'path')
template_str = self.example_yaml_template()

resource_type1_class = mock.Mock()
resource_type1_class.RESOURCE_TYPE = "resource_type1"
resource_type1_instance = mock.Mock()
resource_type1_class.return_value = resource_type1_instance
resource_type2_class = mock.Mock()
resource_type2_class.RESOURCE_TYPE = "resource_type2"
resource_type2_instance = mock.Mock()
resource_type2_class.return_value = resource_type2_instance

resources_to_export = [
resource_type1_class,
resource_type2_class
]

properties = {"foo": "bar"}
template_dict = {
"Resources": {
"Resource1": {
"Type": "resource_type1",
"Properties": properties
},
"Resource2": {
"Type": "resource_type2",
"Properties": properties
},
"Resource3": {
"Type": "some-other-type",
"Properties": properties
},
"Fn::ForEach::OuterLoopName": [
"Identifier1",
["4", "5"],
{
"Fn::ForEach::InnerLoopName": [
"Identifier2",
["6", "7"],
{
"Resource${Identifier1}${Identifier2}": {
"Type": "resource_type2",
"Properties": properties
}
}
],
"Resource${Identifier1}": {
"Type": "resource_type1",
"Properties": properties
}
}
]
}
}

open_mock = mock.mock_open()
yaml_parse_mock.return_value = template_dict

# Patch the file open method to return template string
with mock.patch(
"awscli.customizations.cloudformation.artifact_exporter.open",
open_mock(read_data=template_str)) as open_mock:

template_exporter = Template(
template_path, parent_dir, self.s3_uploader_mock,
resources_to_export)
exported_template = template_exporter.export()
self.assertEqual(exported_template, template_dict)

open_mock.assert_called_once_with(
make_abs_path(parent_dir, template_path), "r")

self.assertEqual(1, yaml_parse_mock.call_count)

resource_type1_class.assert_called_with(self.s3_uploader_mock)
resource_type1_instance.export.assert_called_with(
"Resource${Identifier1}", mock.ANY, template_dir)
resource_type2_class.assert_called_with(self.s3_uploader_mock)
resource_type2_instance.export.assert_called_with(
"Resource${Identifier1}${Identifier2}", mock.ANY, template_dir)

@mock.patch("awscli.customizations.cloudformation.artifact_exporter.yaml_parse")
def test_template_export_foreach_invalid(self, yaml_parse_mock):
parent_dir = os.path.sep
template_dir = os.path.join(parent_dir, 'foo', 'bar')
template_path = os.path.join(template_dir, 'path')
template_str = self.example_yaml_template()

resource_type1_class = mock.Mock()
resource_type1_class.RESOURCE_TYPE = "resource_type1"
resource_type1_instance = mock.Mock()
resource_type1_class.return_value = resource_type1_instance
resource_type2_class = mock.Mock()
resource_type2_class.RESOURCE_TYPE = "resource_type2"
resource_type2_instance = mock.Mock()
resource_type2_class.return_value = resource_type2_instance

resources_to_export = [
resource_type1_class,
resource_type2_class
]

properties = {"foo": "bar"}
template_dict = {
"Resources": {
"Resource1": {
"Type": "resource_type1",
"Properties": properties
},
"Resource2": {
"Type": "resource_type2",
"Properties": properties
},
"Resource3": {
"Type": "some-other-type",
"Properties": properties
},
"Fn::ForEach::OuterLoopName": [
"Identifier1",
{
"Resource${Identifier1}": {
}
}
]
}
}

open_mock = mock.mock_open()
yaml_parse_mock.return_value = template_dict

# Patch the file open method to return template string
with mock.patch(
"awscli.customizations.cloudformation.artifact_exporter.open",
open_mock(read_data=template_str)) as open_mock:
template_exporter = Template(
template_path, parent_dir, self.s3_uploader_mock,
resources_to_export)
with self.assertRaises(ValueError):
template_exporter.export()


@mock.patch("awscli.customizations.cloudformation.artifact_exporter.yaml_parse")
def test_template_global_export(self, yaml_parse_mock):
parent_dir = os.path.sep
Expand Down