From 8e3fa41bb831e835c130390e4881da485003d618 Mon Sep 17 00:00:00 2001 From: Mathieu Grandis Date: Tue, 9 Feb 2021 18:00:48 -0800 Subject: [PATCH 01/10] feat: Function/DeploymentPreference/Alarms now accepts a simple Fn::If, transforms the alarms inside into CFN format and passes it to CFN --- .../deployment_preference_collection.py | 17 +- ...oyment_preference_alarms_intrinsic_if.yaml | 25 +++ .../test_deployment_preference_collection.py | 16 ++ ...oyment_preference_alarms_intrinsic_if.json | 196 ++++++++++++++++++ tests/translator/test_translator.py | 1 + 5 files changed, 253 insertions(+), 2 deletions(-) create mode 100644 tests/translator/input/function_with_deployment_preference_alarms_intrinsic_if.yaml create mode 100644 tests/translator/output/function_with_deployment_preference_alarms_intrinsic_if.json diff --git a/samtranslator/model/preferences/deployment_preference_collection.py b/samtranslator/model/preferences/deployment_preference_collection.py index a226eb766e..e87cc8225e 100644 --- a/samtranslator/model/preferences/deployment_preference_collection.py +++ b/samtranslator/model/preferences/deployment_preference_collection.py @@ -2,7 +2,7 @@ from samtranslator.model.codedeploy import CodeDeployApplication from samtranslator.model.codedeploy import CodeDeployDeploymentGroup from samtranslator.model.iam import IAMRole -from samtranslator.model.intrinsics import fnSub, is_intrinsic +from samtranslator.model.intrinsics import fnSub, is_intrinsic, is_intrinsic_if from samtranslator.model.update_policy import UpdatePolicy from samtranslator.translator.arn_generator import ArnGenerator import copy @@ -128,7 +128,7 @@ def deployment_group(self, function_logical_id): if deployment_preference.alarms is not None: deployment_group.AlarmConfiguration = { "Enabled": True, - "Alarms": [{"Name": alarm} for alarm in deployment_preference.alarms], + "Alarms": self._process_alarms(deployment_preference.alarms), } deployment_group.ApplicationName = self.codedeploy_application.get_runtime_attr("name") @@ -152,6 +152,19 @@ def deployment_group(self, function_logical_id): return deployment_group + def _process_alarms(self, alarms): + if is_intrinsic_if(alarms): + processed_alarms = copy.deepcopy(alarms) + alarms_list = processed_alarms.get("Fn::If") + alarms_list[1] = self._transform_alarms_list(alarms_list[1]) + alarms_list[2] = self._transform_alarms_list(alarms_list[2]) + return processed_alarms + + return self._transform_alarms_list(alarms) + + def _transform_alarms_list(self, alarms): + return [{"Name": alarm} for alarm in alarms] + def _replace_deployment_types(self, value, key=None): if isinstance(value, list): for i in range(len(value)): diff --git a/tests/translator/input/function_with_deployment_preference_alarms_intrinsic_if.yaml b/tests/translator/input/function_with_deployment_preference_alarms_intrinsic_if.yaml new file mode 100644 index 0000000000..15ed112f0e --- /dev/null +++ b/tests/translator/input/function_with_deployment_preference_alarms_intrinsic_if.yaml @@ -0,0 +1,25 @@ +Conditions: + MyCondition: + Fn::Equals: + - true + - false +Resources: + MinimalFunction: + Type: "AWS::Serverless::Function" + Properties: + CodeUri: s3://sam-demo-bucket/hello.zip + Handler: hello.handler + Runtime: python2.7 + AutoPublishAlias: live + DeploymentPreference: + Type: Linear10PercentEvery3Minutes + Alarms: + Fn::If: + - MyCondition + - - Alarm1 + - Alarm2 + - Alarm3 + - - Alarm1 + - Alarm4 + - Alarm5 + - Alarm6 diff --git a/tests/translator/model/preferences/test_deployment_preference_collection.py b/tests/translator/model/preferences/test_deployment_preference_collection.py index ae8b39bdc8..15e5e3e765 100644 --- a/tests/translator/model/preferences/test_deployment_preference_collection.py +++ b/tests/translator/model/preferences/test_deployment_preference_collection.py @@ -163,6 +163,22 @@ def test_deployment_group_with_all_parameters(self): self.assertEqual(deployment_group.to_dict(), expected_deployment_group.to_dict()) + @patch("boto3.session.Session.region_name", "ap-southeast-1") + def test_deployment_preference_with_alarms_intrinsic_if(self): + deployment_preference = { + "Type": "TestDeploymentConfiguration", + "Alarms": {"Fn::If": ["MyCondition", ["Alarm1", "Alarm2"], ["Alarm3"]]}, + } + expected_alarm_configuration = { + "Enabled": True, + "Alarms": {"Fn::If": ["MyCondition", [{"Name": "Alarm1"}, {"Name": "Alarm2"}], [{"Name": "Alarm3"}]]}, + } + deployment_preference_collection = DeploymentPreferenceCollection() + deployment_preference_collection.add(self.function_logical_id, deployment_preference) + deployment_group = deployment_preference_collection.deployment_group(self.function_logical_id) + + self.assertEqual(expected_alarm_configuration, deployment_group.AlarmConfiguration) + @patch("boto3.session.Session.region_name", "ap-southeast-1") def test_update_policy_with_minimal_parameters(self): expected_update_policy = { diff --git a/tests/translator/output/function_with_deployment_preference_alarms_intrinsic_if.json b/tests/translator/output/function_with_deployment_preference_alarms_intrinsic_if.json new file mode 100644 index 0000000000..2295c56518 --- /dev/null +++ b/tests/translator/output/function_with_deployment_preference_alarms_intrinsic_if.json @@ -0,0 +1,196 @@ +{ + "Conditions": { + "MyCondition": { + "Fn::Equals": [ + true, + false + ] + } + }, + "Resources": { + "MinimalFunction": { + "Type": "AWS::Lambda::Function", + "Properties": { + "Code": { + "S3Bucket": "sam-demo-bucket", + "S3Key": "hello.zip" + }, + "Handler": "hello.handler", + "Role": { + "Fn::GetAtt": [ + "MinimalFunctionRole", + "Arn" + ] + }, + "Runtime": "python2.7", + "Tags": [ + { + "Key": "lambda:createdBy", + "Value": "SAM" + } + ] + } + }, + "MinimalFunctionVersion640128d35d": { + "Type": "AWS::Lambda::Version", + "DeletionPolicy": "Retain", + "Properties": { + "FunctionName": { + "Ref": "MinimalFunction" + } + } + }, + "MinimalFunctionAliaslive": { + "Type": "AWS::Lambda::Alias", + "UpdatePolicy": { + "CodeDeployLambdaAliasUpdate": { + "ApplicationName": { + "Ref": "ServerlessDeploymentApplication" + }, + "DeploymentGroupName": { + "Ref": "MinimalFunctionDeploymentGroup" + } + } + }, + "Properties": { + "Name": "live", + "FunctionName": { + "Ref": "MinimalFunction" + }, + "FunctionVersion": { + "Fn::GetAtt": [ + "MinimalFunctionVersion640128d35d", + "Version" + ] + } + } + }, + "MinimalFunctionRole": { + "Type": "AWS::IAM::Role", + "Properties": { + "AssumeRolePolicyDocument": { + "Version": "2012-10-17", + "Statement": [ + { + "Action": [ + "sts:AssumeRole" + ], + "Effect": "Allow", + "Principal": { + "Service": [ + "lambda.amazonaws.com" + ] + } + } + ] + }, + "ManagedPolicyArns": [ + "arn:aws:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole" + ], + "Tags": [ + { + "Key": "lambda:createdBy", + "Value": "SAM" + } + ] + } + }, + "ServerlessDeploymentApplication": { + "Type": "AWS::CodeDeploy::Application", + "Properties": { + "ComputePlatform": "Lambda" + } + }, + "CodeDeployServiceRole": { + "Type": "AWS::IAM::Role", + "Properties": { + "AssumeRolePolicyDocument": { + "Version": "2012-10-17", + "Statement": [ + { + "Action": [ + "sts:AssumeRole" + ], + "Effect": "Allow", + "Principal": { + "Service": [ + "codedeploy.amazonaws.com" + ] + } + } + ] + }, + "ManagedPolicyArns": [ + "arn:aws:iam::aws:policy/service-role/AWSCodeDeployRoleForLambda" + ] + } + }, + "MinimalFunctionDeploymentGroup": { + "Type": "AWS::CodeDeploy::DeploymentGroup", + "Properties": { + "AlarmConfiguration": { + "Enabled": true, + "Alarms": { + "Fn::If": [ + "MyCondition", + [ + { + "Name": "Alarm1" + }, + { + "Name": "Alarm2" + }, + { + "Name": "Alarm3" + } + ], + [ + { + "Name": "Alarm1" + }, + { + "Name": "Alarm4" + }, + { + "Name": "Alarm5" + }, + { + "Name": "Alarm6" + } + ] + ] + } + }, + "ApplicationName": { + "Ref": "ServerlessDeploymentApplication" + }, + "AutoRollbackConfiguration": { + "Enabled": true, + "Events": [ + "DEPLOYMENT_FAILURE", + "DEPLOYMENT_STOP_ON_ALARM", + "DEPLOYMENT_STOP_ON_REQUEST" + ] + }, + "DeploymentConfigName": { + "Fn::Sub": [ + "CodeDeployDefault.Lambda${ConfigName}", + { + "ConfigName": "Linear10PercentEvery3Minutes" + } + ] + }, + "DeploymentStyle": { + "DeploymentType": "BLUE_GREEN", + "DeploymentOption": "WITH_TRAFFIC_CONTROL" + }, + "ServiceRoleArn": { + "Fn::GetAtt": [ + "CodeDeployServiceRole", + "Arn" + ] + } + } + } + } +} \ No newline at end of file diff --git a/tests/translator/test_translator.py b/tests/translator/test_translator.py index f380d34c39..09d1191d56 100644 --- a/tests/translator/test_translator.py +++ b/tests/translator/test_translator.py @@ -240,6 +240,7 @@ class TestTranslatorEndToEnd(TestCase): "function_with_deployment_preference_all_parameters", "function_with_deployment_preference_from_parameters", "function_with_deployment_preference_multiple_combinations", + "function_with_deployment_preference_alarms_intrinsic_if", "function_with_alias_and_event_sources", "function_with_resource_refs", "function_with_deployment_and_custom_role", From f14f860d821b3bcd97a2e3dafed4093ee172a292 Mon Sep 17 00:00:00 2001 From: Mathieu Grandis Date: Wed, 10 Feb 2021 17:26:09 -0800 Subject: [PATCH 02/10] feat: Refactor + added tests --- .../deployment_preference_collection.py | 45 ++++-- samtranslator/translator/translator.py | 7 +- ...oyment_preference_alarms_intrinsic_if.yaml | 2 - .../test_deployment_preference_collection.py | 152 +++++++++++++++++- ...oyment_preference_alarms_intrinsic_if.json | 27 ++-- 5 files changed, 198 insertions(+), 35 deletions(-) diff --git a/samtranslator/model/preferences/deployment_preference_collection.py b/samtranslator/model/preferences/deployment_preference_collection.py index e87cc8225e..f7b2182fc1 100644 --- a/samtranslator/model/preferences/deployment_preference_collection.py +++ b/samtranslator/model/preferences/deployment_preference_collection.py @@ -1,8 +1,9 @@ from .deployment_preference import DeploymentPreference from samtranslator.model.codedeploy import CodeDeployApplication from samtranslator.model.codedeploy import CodeDeployDeploymentGroup +from samtranslator.model.exceptions import InvalidResourceException from samtranslator.model.iam import IAMRole -from samtranslator.model.intrinsics import fnSub, is_intrinsic, is_intrinsic_if +from samtranslator.model.intrinsics import fnSub, is_intrinsic, is_intrinsic_if, is_intrinsic_no_value from samtranslator.model.update_policy import UpdatePolicy from samtranslator.translator.arn_generator import ArnGenerator import copy @@ -125,11 +126,10 @@ def deployment_group(self, function_logical_id): deployment_group = CodeDeployDeploymentGroup(self.deployment_group_logical_id(function_logical_id)) - if deployment_preference.alarms is not None: - deployment_group.AlarmConfiguration = { - "Enabled": True, - "Alarms": self._process_alarms(deployment_preference.alarms), - } + try: + self._process_alarms(deployment_preference, deployment_group) + except ValueError as e: + raise InvalidResourceException(function_logical_id, str(e)) deployment_group.ApplicationName = self.codedeploy_application.get_runtime_attr("name") deployment_group.AutoRollbackConfiguration = { @@ -152,18 +152,33 @@ def deployment_group(self, function_logical_id): return deployment_group - def _process_alarms(self, alarms): - if is_intrinsic_if(alarms): - processed_alarms = copy.deepcopy(alarms) + def _process_alarms(self, preference, group): + if not preference.alarms or is_intrinsic_no_value(preference.alarms): + return + + if is_intrinsic_if(preference.alarms): + processed_alarms = copy.deepcopy(preference.alarms) alarms_list = processed_alarms.get("Fn::If") - alarms_list[1] = self._transform_alarms_list(alarms_list[1]) - alarms_list[2] = self._transform_alarms_list(alarms_list[2]) - return processed_alarms + if not isinstance(alarms_list, list) or not len(alarms_list) == 3: + raise ValueError("Fn::If requires 3 arguments") + alarms_list[1] = self._build_alarms_configuration(alarms_list[1]) + alarms_list[2] = self._build_alarms_configuration(alarms_list[2]) + group.AlarmConfiguration = processed_alarms + return + + group.AlarmConfiguration = self._build_alarms_configuration(preference.alarms) + + def _build_alarms_configuration(self, alarms): + if not isinstance(alarms, list): + raise ValueError("Alarms must be a list") - return self._transform_alarms_list(alarms) + if len(alarms) == 0 or is_intrinsic_no_value(alarms[0]): + return {} - def _transform_alarms_list(self, alarms): - return [{"Name": alarm} for alarm in alarms] + return { + "Enabled": True, + "Alarms": [{"Name": alarm} for alarm in alarms], + } def _replace_deployment_types(self, value, key=None): if isinstance(value, list): diff --git a/samtranslator/translator/translator.py b/samtranslator/translator/translator.py index 29e6974894..a0a2c78eb0 100644 --- a/samtranslator/translator/translator.py +++ b/samtranslator/translator/translator.py @@ -152,7 +152,12 @@ def translate(self, sam_template, parameter_values, feature_toggle=None): template["Resources"].update(deployment_preference_collection.codedeploy_iam_role.to_dict()) for logical_id in deployment_preference_collection.enabled_logical_ids(): - template["Resources"].update(deployment_preference_collection.deployment_group(logical_id).to_dict()) + try: + template["Resources"].update( + deployment_preference_collection.deployment_group(logical_id).to_dict() + ) + except InvalidResourceException as e: + document_errors.append(e) # Run the after-transform plugin target try: diff --git a/tests/translator/input/function_with_deployment_preference_alarms_intrinsic_if.yaml b/tests/translator/input/function_with_deployment_preference_alarms_intrinsic_if.yaml index 15ed112f0e..f392f10628 100644 --- a/tests/translator/input/function_with_deployment_preference_alarms_intrinsic_if.yaml +++ b/tests/translator/input/function_with_deployment_preference_alarms_intrinsic_if.yaml @@ -20,6 +20,4 @@ Resources: - Alarm2 - Alarm3 - - Alarm1 - - Alarm4 - Alarm5 - - Alarm6 diff --git a/tests/translator/model/preferences/test_deployment_preference_collection.py b/tests/translator/model/preferences/test_deployment_preference_collection.py index 15e5e3e765..1d9b14b2d1 100644 --- a/tests/translator/model/preferences/test_deployment_preference_collection.py +++ b/tests/translator/model/preferences/test_deployment_preference_collection.py @@ -163,6 +163,22 @@ def test_deployment_group_with_all_parameters(self): self.assertEqual(deployment_group.to_dict(), expected_deployment_group.to_dict()) + @patch("boto3.session.Session.region_name", "ap-southeast-1") + def test_deployment_preference_with_alarms(self): + deployment_preference = { + "Type": "TestDeploymentConfiguration", + "Alarms": ["Alarm1", "Alarm2", "Alarm3"], + } + expected_alarm_configuration = { + "Enabled": True, + "Alarms": [{"Name": "Alarm1"}, {"Name": "Alarm2"}, {"Name": "Alarm3"}], + } + deployment_preference_collection = DeploymentPreferenceCollection() + deployment_preference_collection.add(self.function_logical_id, deployment_preference) + deployment_group = deployment_preference_collection.deployment_group(self.function_logical_id) + + self.assertEqual(expected_alarm_configuration, deployment_group.AlarmConfiguration) + @patch("boto3.session.Session.region_name", "ap-southeast-1") def test_deployment_preference_with_alarms_intrinsic_if(self): deployment_preference = { @@ -170,8 +186,26 @@ def test_deployment_preference_with_alarms_intrinsic_if(self): "Alarms": {"Fn::If": ["MyCondition", ["Alarm1", "Alarm2"], ["Alarm3"]]}, } expected_alarm_configuration = { - "Enabled": True, - "Alarms": {"Fn::If": ["MyCondition", [{"Name": "Alarm1"}, {"Name": "Alarm2"}], [{"Name": "Alarm3"}]]}, + "Fn::If": [ + "MyCondition", + {"Enabled": True, "Alarms": [{"Name": "Alarm1"}, {"Name": "Alarm2"}]}, + {"Enabled": True, "Alarms": [{"Name": "Alarm3"}]}, + ], + } + deployment_preference_collection = DeploymentPreferenceCollection() + deployment_preference_collection.add(self.function_logical_id, deployment_preference) + deployment_group = deployment_preference_collection.deployment_group(self.function_logical_id) + + self.assertEqual(expected_alarm_configuration, deployment_group.AlarmConfiguration) + + @patch("boto3.session.Session.region_name", "ap-southeast-1") + def test_deployment_preference_with_alarms_intrinsic_if_empty_then(self): + deployment_preference = { + "Type": "TestDeploymentConfiguration", + "Alarms": {"Fn::If": ["MyCondition", [], ["Alarm3"]]}, + } + expected_alarm_configuration = { + "Fn::If": ["MyCondition", {}, {"Enabled": True, "Alarms": [{"Name": "Alarm3"}]}], } deployment_preference_collection = DeploymentPreferenceCollection() deployment_preference_collection.add(self.function_logical_id, deployment_preference) @@ -179,6 +213,120 @@ def test_deployment_preference_with_alarms_intrinsic_if(self): self.assertEqual(expected_alarm_configuration, deployment_group.AlarmConfiguration) + @patch("boto3.session.Session.region_name", "ap-southeast-1") + def test_deployment_preference_with_alarms_intrinsic_if_noref_then(self): + deployment_preference = { + "Type": "TestDeploymentConfiguration", + "Alarms": {"Fn::If": ["MyCondition", [{"Ref": "AWS::NoValue"}], ["Alarm3"]]}, + } + expected_alarm_configuration = { + "Fn::If": ["MyCondition", {}, {"Enabled": True, "Alarms": [{"Name": "Alarm3"}]}], + } + deployment_preference_collection = DeploymentPreferenceCollection() + deployment_preference_collection.add(self.function_logical_id, deployment_preference) + deployment_group = deployment_preference_collection.deployment_group(self.function_logical_id) + + self.assertEqual(expected_alarm_configuration, deployment_group.AlarmConfiguration) + + @patch("boto3.session.Session.region_name", "ap-southeast-1") + def test_deployment_preference_with_alarms_intrinsic_if_empty_else(self): + deployment_preference = { + "Type": "TestDeploymentConfiguration", + "Alarms": {"Fn::If": ["MyCondition", ["Alarm1", "Alarm2"], []]}, + } + expected_alarm_configuration = { + "Fn::If": ["MyCondition", {"Enabled": True, "Alarms": [{"Name": "Alarm1"}, {"Name": "Alarm2"}]}, {}], + } + deployment_preference_collection = DeploymentPreferenceCollection() + deployment_preference_collection.add(self.function_logical_id, deployment_preference) + deployment_group = deployment_preference_collection.deployment_group(self.function_logical_id) + + self.assertEqual(expected_alarm_configuration, deployment_group.AlarmConfiguration) + + @patch("boto3.session.Session.region_name", "ap-southeast-1") + def test_deployment_preference_with_alarms_intrinsic_if_noref_else(self): + deployment_preference = { + "Type": "TestDeploymentConfiguration", + "Alarms": {"Fn::If": ["MyCondition", ["Alarm1", "Alarm2"], [{"Ref": "AWS::NoValue"}]]}, + } + expected_alarm_configuration = { + "Fn::If": ["MyCondition", {"Enabled": True, "Alarms": [{"Name": "Alarm1"}, {"Name": "Alarm2"}]}, {}], + } + deployment_preference_collection = DeploymentPreferenceCollection() + deployment_preference_collection.add(self.function_logical_id, deployment_preference) + deployment_group = deployment_preference_collection.deployment_group(self.function_logical_id) + + self.assertEqual(expected_alarm_configuration, deployment_group.AlarmConfiguration) + + @patch("boto3.session.Session.region_name", "ap-southeast-1") + def test_deployment_preference_with_alarms_ref_novalue(self): + deployment_preference = { + "Type": "TestDeploymentConfiguration", + "Alarms": {"Ref": "AWS::NoValue"}, + } + deployment_preference_collection = DeploymentPreferenceCollection() + deployment_preference_collection.add(self.function_logical_id, deployment_preference) + deployment_group = deployment_preference_collection.deployment_group(self.function_logical_id) + + self.assertIsNone(deployment_group.AlarmConfiguration) + + @patch("boto3.session.Session.region_name", "ap-southeast-1") + def test_deployment_preference_with_alarms_empty(self): + deployment_preference = { + "Type": "TestDeploymentConfiguration", + "Alarms": [], + } + deployment_preference_collection = DeploymentPreferenceCollection() + deployment_preference_collection.add(self.function_logical_id, deployment_preference) + deployment_group = deployment_preference_collection.deployment_group(self.function_logical_id) + + self.assertIsNone(deployment_group.AlarmConfiguration) + + @patch("boto3.session.Session.region_name", "ap-southeast-1") + def test_deployment_preference_with_alarms_not_list(self): + deployment_preference = { + "Type": "TestDeploymentConfiguration", + "Alarms": "Alarm1", + } + deployment_preference_collection = DeploymentPreferenceCollection() + deployment_preference_collection.add(self.function_logical_id, deployment_preference) + with self.assertRaises(InvalidResourceException) as e: + deployment_preference_collection.deployment_group(self.function_logical_id) + self.assertEqual( + e.exception.message, + "Resource with id [{}] is invalid. Alarms must be a list".format(self.function_logical_id), + ) + + @patch("boto3.session.Session.region_name", "ap-southeast-1") + def test_deployment_preference_with_alarms_intrinsic_if_missing_arg(self): + deployment_preference = { + "Type": "TestDeploymentConfiguration", + "Alarms": {"Fn::If": ["MyCondition", ["Alarm1", "Alarm2"]]}, + } + deployment_preference_collection = DeploymentPreferenceCollection() + deployment_preference_collection.add(self.function_logical_id, deployment_preference) + with self.assertRaises(InvalidResourceException) as e: + deployment_preference_collection.deployment_group(self.function_logical_id) + self.assertEqual( + e.exception.message, + "Resource with id [{}] is invalid. Fn::If requires 3 arguments".format(self.function_logical_id), + ) + + @patch("boto3.session.Session.region_name", "ap-southeast-1") + def test_deployment_preference_with_alarms_intrinsic_if_not_list(self): + deployment_preference = { + "Type": "TestDeploymentConfiguration", + "Alarms": {"Fn::If": "Alarm1"}, + } + deployment_preference_collection = DeploymentPreferenceCollection() + deployment_preference_collection.add(self.function_logical_id, deployment_preference) + with self.assertRaises(InvalidResourceException) as e: + deployment_preference_collection.deployment_group(self.function_logical_id) + self.assertEqual( + e.exception.message, + "Resource with id [{}] is invalid. Fn::If requires 3 arguments".format(self.function_logical_id), + ) + @patch("boto3.session.Session.region_name", "ap-southeast-1") def test_update_policy_with_minimal_parameters(self): expected_update_policy = { diff --git a/tests/translator/output/function_with_deployment_preference_alarms_intrinsic_if.json b/tests/translator/output/function_with_deployment_preference_alarms_intrinsic_if.json index 2295c56518..72643d9ca5 100644 --- a/tests/translator/output/function_with_deployment_preference_alarms_intrinsic_if.json +++ b/tests/translator/output/function_with_deployment_preference_alarms_intrinsic_if.json @@ -129,11 +129,11 @@ "Type": "AWS::CodeDeploy::DeploymentGroup", "Properties": { "AlarmConfiguration": { - "Enabled": true, - "Alarms": { - "Fn::If": [ - "MyCondition", - [ + "Fn::If": [ + "MyCondition", + { + "Enabled": true, + "Alarms": [ { "Name": "Alarm1" }, @@ -143,23 +143,20 @@ { "Name": "Alarm3" } - ], - [ + ] + }, + { + "Enabled": true, + "Alarms": [ { "Name": "Alarm1" }, - { - "Name": "Alarm4" - }, { "Name": "Alarm5" - }, - { - "Name": "Alarm6" } ] - ] - } + } + ] }, "ApplicationName": { "Ref": "ServerlessDeploymentApplication" From 923b3166e69f8bb049f97603790d09993cab224f Mon Sep 17 00:00:00 2001 From: Mathieu Grandis Date: Wed, 10 Feb 2021 17:47:50 -0800 Subject: [PATCH 03/10] tests: Added missing cn/gov tests --- ...oyment_preference_alarms_intrinsic_if.json | 193 ++++++++++++++++++ ...oyment_preference_alarms_intrinsic_if.json | 193 ++++++++++++++++++ 2 files changed, 386 insertions(+) create mode 100644 tests/translator/output/aws-cn/function_with_deployment_preference_alarms_intrinsic_if.json create mode 100644 tests/translator/output/aws-us-gov/function_with_deployment_preference_alarms_intrinsic_if.json diff --git a/tests/translator/output/aws-cn/function_with_deployment_preference_alarms_intrinsic_if.json b/tests/translator/output/aws-cn/function_with_deployment_preference_alarms_intrinsic_if.json new file mode 100644 index 0000000000..077d23fc8e --- /dev/null +++ b/tests/translator/output/aws-cn/function_with_deployment_preference_alarms_intrinsic_if.json @@ -0,0 +1,193 @@ +{ + "Conditions": { + "MyCondition": { + "Fn::Equals": [ + true, + false + ] + } + }, + "Resources": { + "MinimalFunction": { + "Type": "AWS::Lambda::Function", + "Properties": { + "Code": { + "S3Bucket": "sam-demo-bucket", + "S3Key": "hello.zip" + }, + "Handler": "hello.handler", + "Role": { + "Fn::GetAtt": [ + "MinimalFunctionRole", + "Arn" + ] + }, + "Runtime": "python2.7", + "Tags": [ + { + "Key": "lambda:createdBy", + "Value": "SAM" + } + ] + } + }, + "MinimalFunctionVersion640128d35d": { + "Type": "AWS::Lambda::Version", + "DeletionPolicy": "Retain", + "Properties": { + "FunctionName": { + "Ref": "MinimalFunction" + } + } + }, + "MinimalFunctionAliaslive": { + "Type": "AWS::Lambda::Alias", + "UpdatePolicy": { + "CodeDeployLambdaAliasUpdate": { + "ApplicationName": { + "Ref": "ServerlessDeploymentApplication" + }, + "DeploymentGroupName": { + "Ref": "MinimalFunctionDeploymentGroup" + } + } + }, + "Properties": { + "Name": "live", + "FunctionName": { + "Ref": "MinimalFunction" + }, + "FunctionVersion": { + "Fn::GetAtt": [ + "MinimalFunctionVersion640128d35d", + "Version" + ] + } + } + }, + "MinimalFunctionRole": { + "Type": "AWS::IAM::Role", + "Properties": { + "AssumeRolePolicyDocument": { + "Version": "2012-10-17", + "Statement": [ + { + "Action": [ + "sts:AssumeRole" + ], + "Effect": "Allow", + "Principal": { + "Service": [ + "lambda.amazonaws.com" + ] + } + } + ] + }, + "ManagedPolicyArns": [ + "arn:aws-cn:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole" + ], + "Tags": [ + { + "Key": "lambda:createdBy", + "Value": "SAM" + } + ] + } + }, + "ServerlessDeploymentApplication": { + "Type": "AWS::CodeDeploy::Application", + "Properties": { + "ComputePlatform": "Lambda" + } + }, + "CodeDeployServiceRole": { + "Type": "AWS::IAM::Role", + "Properties": { + "AssumeRolePolicyDocument": { + "Version": "2012-10-17", + "Statement": [ + { + "Action": [ + "sts:AssumeRole" + ], + "Effect": "Allow", + "Principal": { + "Service": [ + "codedeploy.amazonaws.com" + ] + } + } + ] + }, + "ManagedPolicyArns": [ + "arn:aws-cn:iam::aws:policy/service-role/AWSCodeDeployRoleForLambda" + ] + } + }, + "MinimalFunctionDeploymentGroup": { + "Type": "AWS::CodeDeploy::DeploymentGroup", + "Properties": { + "AlarmConfiguration": { + "Fn::If": [ + "MyCondition", + { + "Enabled": true, + "Alarms": [ + { + "Name": "Alarm1" + }, + { + "Name": "Alarm2" + }, + { + "Name": "Alarm3" + } + ] + }, + { + "Enabled": true, + "Alarms": [ + { + "Name": "Alarm1" + }, + { + "Name": "Alarm5" + } + ] + } + ] + }, + "ApplicationName": { + "Ref": "ServerlessDeploymentApplication" + }, + "AutoRollbackConfiguration": { + "Enabled": true, + "Events": [ + "DEPLOYMENT_FAILURE", + "DEPLOYMENT_STOP_ON_ALARM", + "DEPLOYMENT_STOP_ON_REQUEST" + ] + }, + "DeploymentConfigName": { + "Fn::Sub": [ + "CodeDeployDefault.Lambda${ConfigName}", + { + "ConfigName": "Linear10PercentEvery3Minutes" + } + ] + }, + "DeploymentStyle": { + "DeploymentType": "BLUE_GREEN", + "DeploymentOption": "WITH_TRAFFIC_CONTROL" + }, + "ServiceRoleArn": { + "Fn::GetAtt": [ + "CodeDeployServiceRole", + "Arn" + ] + } + } + } + } +} \ No newline at end of file diff --git a/tests/translator/output/aws-us-gov/function_with_deployment_preference_alarms_intrinsic_if.json b/tests/translator/output/aws-us-gov/function_with_deployment_preference_alarms_intrinsic_if.json new file mode 100644 index 0000000000..b156bf3184 --- /dev/null +++ b/tests/translator/output/aws-us-gov/function_with_deployment_preference_alarms_intrinsic_if.json @@ -0,0 +1,193 @@ +{ + "Conditions": { + "MyCondition": { + "Fn::Equals": [ + true, + false + ] + } + }, + "Resources": { + "MinimalFunction": { + "Type": "AWS::Lambda::Function", + "Properties": { + "Code": { + "S3Bucket": "sam-demo-bucket", + "S3Key": "hello.zip" + }, + "Handler": "hello.handler", + "Role": { + "Fn::GetAtt": [ + "MinimalFunctionRole", + "Arn" + ] + }, + "Runtime": "python2.7", + "Tags": [ + { + "Key": "lambda:createdBy", + "Value": "SAM" + } + ] + } + }, + "MinimalFunctionVersion640128d35d": { + "Type": "AWS::Lambda::Version", + "DeletionPolicy": "Retain", + "Properties": { + "FunctionName": { + "Ref": "MinimalFunction" + } + } + }, + "MinimalFunctionAliaslive": { + "Type": "AWS::Lambda::Alias", + "UpdatePolicy": { + "CodeDeployLambdaAliasUpdate": { + "ApplicationName": { + "Ref": "ServerlessDeploymentApplication" + }, + "DeploymentGroupName": { + "Ref": "MinimalFunctionDeploymentGroup" + } + } + }, + "Properties": { + "Name": "live", + "FunctionName": { + "Ref": "MinimalFunction" + }, + "FunctionVersion": { + "Fn::GetAtt": [ + "MinimalFunctionVersion640128d35d", + "Version" + ] + } + } + }, + "MinimalFunctionRole": { + "Type": "AWS::IAM::Role", + "Properties": { + "AssumeRolePolicyDocument": { + "Version": "2012-10-17", + "Statement": [ + { + "Action": [ + "sts:AssumeRole" + ], + "Effect": "Allow", + "Principal": { + "Service": [ + "lambda.amazonaws.com" + ] + } + } + ] + }, + "ManagedPolicyArns": [ + "arn:aws-us-gov:iam::aws:policy/service-role/AWSLambdaBasicExecutionRole" + ], + "Tags": [ + { + "Key": "lambda:createdBy", + "Value": "SAM" + } + ] + } + }, + "ServerlessDeploymentApplication": { + "Type": "AWS::CodeDeploy::Application", + "Properties": { + "ComputePlatform": "Lambda" + } + }, + "CodeDeployServiceRole": { + "Type": "AWS::IAM::Role", + "Properties": { + "AssumeRolePolicyDocument": { + "Version": "2012-10-17", + "Statement": [ + { + "Action": [ + "sts:AssumeRole" + ], + "Effect": "Allow", + "Principal": { + "Service": [ + "codedeploy.amazonaws.com" + ] + } + } + ] + }, + "ManagedPolicyArns": [ + "arn:aws-us-gov:iam::aws:policy/service-role/AWSCodeDeployRoleForLambda" + ] + } + }, + "MinimalFunctionDeploymentGroup": { + "Type": "AWS::CodeDeploy::DeploymentGroup", + "Properties": { + "AlarmConfiguration": { + "Fn::If": [ + "MyCondition", + { + "Enabled": true, + "Alarms": [ + { + "Name": "Alarm1" + }, + { + "Name": "Alarm2" + }, + { + "Name": "Alarm3" + } + ] + }, + { + "Enabled": true, + "Alarms": [ + { + "Name": "Alarm1" + }, + { + "Name": "Alarm5" + } + ] + } + ] + }, + "ApplicationName": { + "Ref": "ServerlessDeploymentApplication" + }, + "AutoRollbackConfiguration": { + "Enabled": true, + "Events": [ + "DEPLOYMENT_FAILURE", + "DEPLOYMENT_STOP_ON_ALARM", + "DEPLOYMENT_STOP_ON_REQUEST" + ] + }, + "DeploymentConfigName": { + "Fn::Sub": [ + "CodeDeployDefault.Lambda${ConfigName}", + { + "ConfigName": "Linear10PercentEvery3Minutes" + } + ] + }, + "DeploymentStyle": { + "DeploymentType": "BLUE_GREEN", + "DeploymentOption": "WITH_TRAFFIC_CONTROL" + }, + "ServiceRoleArn": { + "Fn::GetAtt": [ + "CodeDeployServiceRole", + "Arn" + ] + } + } + } + } +} \ No newline at end of file From 0bc04605635ae30c0c2394f2705c365caa56e6b5 Mon Sep 17 00:00:00 2001 From: Mathieu Grandis Date: Wed, 10 Feb 2021 17:58:54 -0800 Subject: [PATCH 04/10] chore: Added docstrings --- .../deployment_preference_collection.py | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/samtranslator/model/preferences/deployment_preference_collection.py b/samtranslator/model/preferences/deployment_preference_collection.py index f7b2182fc1..480e89bcdc 100644 --- a/samtranslator/model/preferences/deployment_preference_collection.py +++ b/samtranslator/model/preferences/deployment_preference_collection.py @@ -153,6 +153,21 @@ def deployment_group(self, function_logical_id): return deployment_group def _process_alarms(self, preference, group): + """ + Processes the deployment preferences alarms and updates the deployment group accordingly + + Parameters + ---------- + preference : dict + Deployment preferences + group : dict + Deployment group + + Raises + ------ + ValueError + If Alarms is in the wrong format + """ if not preference.alarms or is_intrinsic_no_value(preference.alarms): return @@ -169,6 +184,24 @@ def _process_alarms(self, preference, group): group.AlarmConfiguration = self._build_alarms_configuration(preference.alarms) def _build_alarms_configuration(self, alarms): + """ + Builds an Alarms configuration from a list of alarms + + Parameters + ---------- + alarms : list[str] + Alarms + + Returns + ------- + dict + AlarmsConfiguration for a deployment group + + Raises + ------ + ValueError + If alarms is not a list + """ if not isinstance(alarms, list): raise ValueError("Alarms must be a list") From e72f2c10dae5cdfcd30dcfde53b6ab69059ebdcf Mon Sep 17 00:00:00 2001 From: Mathieu Grandis Date: Fri, 12 Feb 2021 14:53:18 -0800 Subject: [PATCH 05/10] refactor: Refactor to make the code clearer --- .../deployment_preference_collection.py | 38 ++++++++++--------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/samtranslator/model/preferences/deployment_preference_collection.py b/samtranslator/model/preferences/deployment_preference_collection.py index 480e89bcdc..f48d411a40 100644 --- a/samtranslator/model/preferences/deployment_preference_collection.py +++ b/samtranslator/model/preferences/deployment_preference_collection.py @@ -127,7 +127,7 @@ def deployment_group(self, function_logical_id): deployment_group = CodeDeployDeploymentGroup(self.deployment_group_logical_id(function_logical_id)) try: - self._process_alarms(deployment_preference, deployment_group) + deployment_group.AlarmConfiguration = self._convert_alarms(deployment_preference.alarms) except ValueError as e: raise InvalidResourceException(function_logical_id, str(e)) @@ -152,40 +152,42 @@ def deployment_group(self, function_logical_id): return deployment_group - def _process_alarms(self, preference, group): + def _convert_alarms(self, preference_alarms): """ - Processes the deployment preferences alarms and updates the deployment group accordingly + Converts deployment preference alarms to an AlarmsConfiguration Parameters ---------- - preference : dict - Deployment preferences - group : dict - Deployment group + preference_alarms : dict + Deployment preference alarms + + Returns + ------- + dict + AlarmsConfiguration if alarms is set, None otherwise Raises ------ ValueError If Alarms is in the wrong format """ - if not preference.alarms or is_intrinsic_no_value(preference.alarms): - return + if not preference_alarms or is_intrinsic_no_value(preference_alarms): + return None - if is_intrinsic_if(preference.alarms): - processed_alarms = copy.deepcopy(preference.alarms) + if is_intrinsic_if(preference_alarms): + processed_alarms = copy.deepcopy(preference_alarms) alarms_list = processed_alarms.get("Fn::If") if not isinstance(alarms_list, list) or not len(alarms_list) == 3: raise ValueError("Fn::If requires 3 arguments") - alarms_list[1] = self._build_alarms_configuration(alarms_list[1]) - alarms_list[2] = self._build_alarms_configuration(alarms_list[2]) - group.AlarmConfiguration = processed_alarms - return + alarms_list[1] = self._build_alarm_configuration(alarms_list[1]) + alarms_list[2] = self._build_alarm_configuration(alarms_list[2]) + return processed_alarms - group.AlarmConfiguration = self._build_alarms_configuration(preference.alarms) + return self._build_alarm_configuration(preference_alarms) - def _build_alarms_configuration(self, alarms): + def _build_alarm_configuration(self, alarms): """ - Builds an Alarms configuration from a list of alarms + Builds an AlarmConfiguration from a list of alarms Parameters ---------- From 3491b3deabaf713a2df128dd218ebc78e488c5df Mon Sep 17 00:00:00 2001 From: Mathieu Grandis Date: Fri, 12 Feb 2021 15:48:37 -0800 Subject: [PATCH 06/10] test: Added translator test --- ...ith_deployment_preference_invalid_alarms.yaml | 16 ++++++++++++++++ ...ith_deployment_preference_invalid_alarms.json | 11 +++++++++++ tests/translator/test_translator.py | 1 + 3 files changed, 28 insertions(+) create mode 100644 tests/translator/input/error_function_with_deployment_preference_invalid_alarms.yaml create mode 100644 tests/translator/output/error_function_with_deployment_preference_invalid_alarms.json diff --git a/tests/translator/input/error_function_with_deployment_preference_invalid_alarms.yaml b/tests/translator/input/error_function_with_deployment_preference_invalid_alarms.yaml new file mode 100644 index 0000000000..1ef2d22d39 --- /dev/null +++ b/tests/translator/input/error_function_with_deployment_preference_invalid_alarms.yaml @@ -0,0 +1,16 @@ +Conditions: + MyCondition: + Fn::Equals: + - true + - false +Resources: + MinimalFunction: + Type: "AWS::Serverless::Function" + Properties: + CodeUri: s3://sam-demo-bucket/hello.zip + Handler: hello.handler + Runtime: python2.7 + AutoPublishAlias: live + DeploymentPreference: + Type: Linear10PercentEvery3Minutes + Alarms: MyAlarm diff --git a/tests/translator/output/error_function_with_deployment_preference_invalid_alarms.json b/tests/translator/output/error_function_with_deployment_preference_invalid_alarms.json new file mode 100644 index 0000000000..ee4e8d6174 --- /dev/null +++ b/tests/translator/output/error_function_with_deployment_preference_invalid_alarms.json @@ -0,0 +1,11 @@ +{ + "errors": [ + { + "errorMessage": "Resource with id [MinimalFunction] is invalid. Alarms must be a list" + }, + { + "errorMessage": "Resource with id [MinimalFunction] is invalid. Alarms must be a list" + } + ], + "errorMessage": "Invalid Serverless Application Specification document. Number of errors found: 2. Resource with id [MinimalFunction] is invalid. Alarms must be a list Resource with id [MinimalFunction] is invalid. Alarms must be a list" +} \ No newline at end of file diff --git a/tests/translator/test_translator.py b/tests/translator/test_translator.py index 09d1191d56..07f2016023 100644 --- a/tests/translator/test_translator.py +++ b/tests/translator/test_translator.py @@ -622,6 +622,7 @@ def _generate_new_deployment_hash(self, logical_id, dict_to_hash, rest_api_to_sw "error_function_no_codeuri", "error_function_no_handler", "error_function_no_runtime", + "error_function_with_deployment_preference_invalid_alarms", "error_function_with_deployment_preference_missing_alias", "error_function_with_invalid_deployment_preference_hook_property", "error_function_invalid_request_parameters", From 3c1376ee6e0cddd02c78fa1ac965eb7f1e139170 Mon Sep 17 00:00:00 2001 From: Mathieu Grandis Date: Wed, 17 Feb 2021 16:51:58 -0800 Subject: [PATCH 07/10] refactor: Updated the integration test engine with a detailed error report --- integration/helpers/base_test.py | 4 +++- integration/helpers/resource.py | 15 +++++++++++---- 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/integration/helpers/base_test.py b/integration/helpers/base_test.py index 7b054c0e01..01e19d3b70 100644 --- a/integration/helpers/base_test.py +++ b/integration/helpers/base_test.py @@ -347,7 +347,9 @@ def verify_stack(self): # verify if the stack was successfully created self.assertEqual(self.stack_description["Stacks"][0]["StackStatus"], "CREATE_COMPLETE") # verify if the stack contains the expected resources - self.assertTrue(verify_stack_resources(self.expected_resource_path, self.stack_resources)) + error = verify_stack_resources(self.expected_resource_path, self.stack_resources) + if error: + self.fail(error) def _load_yaml(self, file_path): """ diff --git a/integration/helpers/resource.py b/integration/helpers/resource.py index 8a32a1de63..35476da33d 100644 --- a/integration/helpers/resource.py +++ b/integration/helpers/resource.py @@ -34,7 +34,7 @@ def verify_stack_resources(expected_file_path, stack_resources): parsed_resources = _sort_resources(stack_resources["StackResourceSummaries"]) if len(expected_resources) != len(parsed_resources): - return False + return "'{}' resources expected, '{}' found".format(len(expected_resources), len(parsed_resources)) for i in range(len(expected_resources)): exp = expected_resources[i] @@ -43,10 +43,17 @@ def verify_stack_resources(expected_file_path, stack_resources): "^" + exp["LogicalResourceId"] + "([0-9a-f]{" + str(LogicalIdGenerator.HASH_LENGTH) + "})?$", parsed["LogicalResourceId"], ): - return False + parsed_trimed_down = { + "LogicalResourceId": parsed["LogicalResourceId"], + "ResourceType": parsed["ResourceType"], + } + + return "'{}' expected, '{}' found (Resources must appear in the same order, don't include the LogicalId random suffix)".format( + exp, parsed_trimed_down + ) if exp["ResourceType"] != parsed["ResourceType"]: - return False - return True + return "'{}' expected, '{}' found".format(exp["ResourceType"], parsed["ResourceType"]) + return None def generate_suffix(): From a4d462ce989d80eeb01df016ba6b9aaff03bd0bc Mon Sep 17 00:00:00 2001 From: Mathieu Grandis Date: Wed, 17 Feb 2021 16:54:21 -0800 Subject: [PATCH 08/10] test: Added an integration test with an Fn::If on the Alarms property --- ...oyment_preference_alarms_intrinsic_if.json | 30 +++++++++++++++++++ ...oyment_preference_alarms_intrinsic_if.yaml | 23 ++++++++++++++ integration/single/test_basic_function.py | 3 ++ 3 files changed, 56 insertions(+) create mode 100644 integration/resources/expected/single/function_with_deployment_preference_alarms_intrinsic_if.json create mode 100644 integration/resources/templates/single/function_with_deployment_preference_alarms_intrinsic_if.yaml diff --git a/integration/resources/expected/single/function_with_deployment_preference_alarms_intrinsic_if.json b/integration/resources/expected/single/function_with_deployment_preference_alarms_intrinsic_if.json new file mode 100644 index 0000000000..c71bcec273 --- /dev/null +++ b/integration/resources/expected/single/function_with_deployment_preference_alarms_intrinsic_if.json @@ -0,0 +1,30 @@ +[ + { + "LogicalResourceId": "ServerlessDeploymentApplication", + "ResourceType": "AWS::CodeDeploy::Application" + }, + { + "LogicalResourceId": "MyLambdaFunctionRole", + "ResourceType": "AWS::IAM::Role" + }, + { + "LogicalResourceId": "CodeDeployServiceRole", + "ResourceType": "AWS::IAM::Role" + }, + { + "LogicalResourceId": "MyLambdaFunction", + "ResourceType": "AWS::Lambda::Function" + }, + { + "LogicalResourceId": "MyLambdaFunctionDeploymentGroup", + "ResourceType": "AWS::CodeDeploy::DeploymentGroup" + }, + { + "LogicalResourceId": "MyLambdaFunctionVersion", + "ResourceType": "AWS::Lambda::Version" + }, + { + "LogicalResourceId": "MyLambdaFunctionAliaslive", + "ResourceType": "AWS::Lambda::Alias" + } +] \ No newline at end of file diff --git a/integration/resources/templates/single/function_with_deployment_preference_alarms_intrinsic_if.yaml b/integration/resources/templates/single/function_with_deployment_preference_alarms_intrinsic_if.yaml new file mode 100644 index 0000000000..261571ff5e --- /dev/null +++ b/integration/resources/templates/single/function_with_deployment_preference_alarms_intrinsic_if.yaml @@ -0,0 +1,23 @@ +Conditions: + MyCondition: + Fn::Equals: + - true + - false +Resources: + MyLambdaFunction: + Type: "AWS::Serverless::Function" + Properties: + CodeUri: ${codeuri} + Handler: hello.handler + Runtime: python2.7 + AutoPublishAlias: live + DeploymentPreference: + Type: Linear10PercentEvery3Minutes + Alarms: + Fn::If: + - MyCondition + - - Alarm1 + - Alarm2 + - Alarm3 + - - Alarm1 + - Alarm5 diff --git a/integration/single/test_basic_function.py b/integration/single/test_basic_function.py index 992a44245d..2a5992ec0a 100644 --- a/integration/single/test_basic_function.py +++ b/integration/single/test_basic_function.py @@ -40,6 +40,9 @@ def test_function_with_http_api_events(self, file_name): self.assertEqual(requests.get(endpoint).text, self.FUNCTION_OUTPUT) + def test_function_with_deployment_preference_alarms_intrinsic_if(self): + self.create_and_verify_stack("function_with_deployment_preference_alarms_intrinsic_if") + @parameterized.expand( [ ("basic_function_with_sns_dlq", "sns:Publish"), From 382347a8b691c198e449ab7f732a29c39c78609b Mon Sep 17 00:00:00 2001 From: Mathieu Grandis Date: Wed, 17 Feb 2021 17:00:03 -0800 Subject: [PATCH 09/10] refactor: Put the intrinsic_if validation test into its own function, moved and added tests --- samtranslator/model/function_policies.py | 9 +++- samtranslator/model/intrinsics.py | 17 ++++++ .../deployment_preference_collection.py | 10 +++- samtranslator/model/resource_policies.py | 9 +++- tests/model/test_function_policies.py | 32 ----------- tests/test_intrinsics.py | 53 ++++++++++++++++++- 6 files changed, 91 insertions(+), 39 deletions(-) diff --git a/samtranslator/model/function_policies.py b/samtranslator/model/function_policies.py index 91a5be28a1..2f1000b99a 100644 --- a/samtranslator/model/function_policies.py +++ b/samtranslator/model/function_policies.py @@ -3,7 +3,12 @@ from six import string_types -from samtranslator.model.intrinsics import is_intrinsic, is_intrinsic_if, is_intrinsic_no_value +from samtranslator.model.intrinsics import ( + is_intrinsic, + is_intrinsic_if, + is_intrinsic_no_value, + is_valid_intrinsic_if_items, +) from samtranslator.model.exceptions import InvalidTemplateException PolicyEntry = namedtuple("PolicyEntry", "data type") @@ -165,7 +170,7 @@ def _get_type_from_intrinsic_if(self, policy): """ intrinsic_if_value = policy["Fn::If"] - if not len(intrinsic_if_value) == 3: + if not is_valid_intrinsic_if_items(intrinsic_if_value): raise InvalidTemplateException("Fn::If requires 3 arguments") if_data = intrinsic_if_value[1] diff --git a/samtranslator/model/intrinsics.py b/samtranslator/model/intrinsics.py index 57982f5148..6ef99d088c 100644 --- a/samtranslator/model/intrinsics.py +++ b/samtranslator/model/intrinsics.py @@ -162,6 +162,23 @@ def is_intrinsic_if(input): return key == "Fn::If" +def is_valid_intrinsic_if_items(items): + """ + Returns a value indicating whether the items are valid Fn::If items or not + + Parameters + ---------- + items : list + Fn::If items + + Returns + ------- + boolean + True if valid items, False otherwise + """ + return isinstance(items, list) and len(items) == 3 + + def is_intrinsic_no_value(input): """ Is the given input an intrinsic Ref: AWS::NoValue? Intrinsic function is a dictionary with single diff --git a/samtranslator/model/preferences/deployment_preference_collection.py b/samtranslator/model/preferences/deployment_preference_collection.py index f48d411a40..aba313fc53 100644 --- a/samtranslator/model/preferences/deployment_preference_collection.py +++ b/samtranslator/model/preferences/deployment_preference_collection.py @@ -3,7 +3,13 @@ from samtranslator.model.codedeploy import CodeDeployDeploymentGroup from samtranslator.model.exceptions import InvalidResourceException from samtranslator.model.iam import IAMRole -from samtranslator.model.intrinsics import fnSub, is_intrinsic, is_intrinsic_if, is_intrinsic_no_value +from samtranslator.model.intrinsics import ( + fnSub, + is_intrinsic, + is_intrinsic_if, + is_intrinsic_no_value, + is_valid_intrinsic_if_items, +) from samtranslator.model.update_policy import UpdatePolicy from samtranslator.translator.arn_generator import ArnGenerator import copy @@ -177,7 +183,7 @@ def _convert_alarms(self, preference_alarms): if is_intrinsic_if(preference_alarms): processed_alarms = copy.deepcopy(preference_alarms) alarms_list = processed_alarms.get("Fn::If") - if not isinstance(alarms_list, list) or not len(alarms_list) == 3: + if not is_valid_intrinsic_if_items(alarms_list): raise ValueError("Fn::If requires 3 arguments") alarms_list[1] = self._build_alarm_configuration(alarms_list[1]) alarms_list[2] = self._build_alarm_configuration(alarms_list[2]) diff --git a/samtranslator/model/resource_policies.py b/samtranslator/model/resource_policies.py index 810eb7976e..3f2478fef2 100644 --- a/samtranslator/model/resource_policies.py +++ b/samtranslator/model/resource_policies.py @@ -3,7 +3,12 @@ from six import string_types -from samtranslator.model.intrinsics import is_intrinsic, is_intrinsic_if, is_intrinsic_no_value +from samtranslator.model.intrinsics import ( + is_intrinsic, + is_intrinsic_if, + is_intrinsic_no_value, + is_valid_intrinsic_if_items, +) from samtranslator.model.exceptions import InvalidTemplateException PolicyEntry = namedtuple("PolicyEntry", "data type") @@ -165,7 +170,7 @@ def _get_type_from_intrinsic_if(self, policy): """ intrinsic_if_value = policy["Fn::If"] - if not len(intrinsic_if_value) == 3: + if not is_valid_intrinsic_if_items(intrinsic_if_value): raise InvalidTemplateException("Fn::If requires 3 arguments") if_data = intrinsic_if_value[1] diff --git a/tests/model/test_function_policies.py b/tests/model/test_function_policies.py index c7d9d90414..bcbe7f1525 100644 --- a/tests/model/test_function_policies.py +++ b/tests/model/test_function_policies.py @@ -3,7 +3,6 @@ from samtranslator.model.function_policies import FunctionPolicies, PolicyTypes, PolicyEntry from samtranslator.model.exceptions import InvalidTemplateException -from samtranslator.model.intrinsics import is_intrinsic_if, is_intrinsic_no_value class TestFunctionPolicies(TestCase): @@ -271,37 +270,6 @@ def test_is_policy_template_must_return_false_without_the_processor(self): self.assertFalse(function_policies_obj._is_policy_template(policy)) self.policy_template_processor_mock.has.assert_not_called() - def test_is_intrinsic_if_must_return_true_for_if(self): - policy = {"Fn::If": "some value"} - - self.assertTrue(is_intrinsic_if(policy)) - - def test_is_intrinsic_if_must_return_false_for_others(self): - too_many_keys = {"Fn::If": "some value", "Fn::And": "other value"} - - not_if = {"Fn::Or": "some value"} - - self.assertFalse(is_intrinsic_if(too_many_keys)) - self.assertFalse(is_intrinsic_if(not_if)) - self.assertFalse(is_intrinsic_if(None)) - - def test_is_intrinsic_no_value_must_return_true_for_no_value(self): - policy = {"Ref": "AWS::NoValue"} - - self.assertTrue(is_intrinsic_no_value(policy)) - - def test_is_intrinsic_no_value_must_return_false_for_other_value(self): - bad_key = {"sRefs": "AWS::NoValue"} - - bad_value = {"Ref": "SWA::NoValue"} - - too_many_keys = {"Ref": "AWS::NoValue", "feR": "SWA::NoValue"} - - self.assertFalse(is_intrinsic_no_value(bad_key)) - self.assertFalse(is_intrinsic_no_value(bad_value)) - self.assertFalse(is_intrinsic_no_value(None)) - self.assertFalse(is_intrinsic_no_value(too_many_keys)) - def test_get_type_with_intrinsic_if_must_return_managed_policy_type(self): managed_policy = {"Fn::If": ["SomeCondition", "some managed policy arn", "other managed policy arn"]} diff --git a/tests/test_intrinsics.py b/tests/test_intrinsics.py index 5a3f22f4f5..06c7447fc6 100644 --- a/tests/test_intrinsics.py +++ b/tests/test_intrinsics.py @@ -1,7 +1,13 @@ from parameterized import parameterized from unittest import TestCase -from samtranslator.model.intrinsics import is_intrinsic, make_shorthand +from samtranslator.model.intrinsics import ( + is_intrinsic, + make_shorthand, + is_intrinsic_if, + is_valid_intrinsic_if_items, + is_intrinsic_no_value, +) class TestIntrinsics(TestCase): @@ -30,3 +36,48 @@ def test_make_short_hand_failure(self): with self.assertRaises(NotImplementedError): make_shorthand(input) + + def test_is_intrinsic_no_value_must_return_true_for_no_value(self): + policy = {"Ref": "AWS::NoValue"} + + self.assertTrue(is_intrinsic_no_value(policy)) + + def test_is_intrinsic_no_value_must_return_false_for_other_value(self): + bad_key = {"sRefs": "AWS::NoValue"} + bad_value = {"Ref": "SWA::NoValue"} + too_many_keys = {"Ref": "AWS::NoValue", "feR": "SWA::NoValue"} + + self.assertFalse(is_intrinsic_no_value(bad_key)) + self.assertFalse(is_intrinsic_no_value(bad_value)) + self.assertFalse(is_intrinsic_no_value(None)) + self.assertFalse(is_intrinsic_no_value(too_many_keys)) + + def test_is_intrinsic_if_must_return_true_for_if(self): + policy = {"Fn::If": "some value"} + + self.assertTrue(is_intrinsic_if(policy)) + + def test_is_intrinsic_if_must_return_false_for_others(self): + too_many_keys = {"Fn::If": "some value", "Fn::And": "other value"} + not_if = {"Fn::Or": "some value"} + + self.assertFalse(is_intrinsic_if(too_many_keys)) + self.assertFalse(is_intrinsic_if(not_if)) + self.assertFalse(is_intrinsic_if(None)) + + def test_is_valid_intrinsic_if_items_valid(self): + self.assertTrue(is_valid_intrinsic_if_items(["Condition", "Then", "Else"])) + + def test_is_valid_intrinsic_if_items_invalid(self): + not_enough_items = ["Then", "Else"] + is_string = "Then" + is_integer = 3 + is_boolean = True + is_dict = {"Fn::If": "some value", "Fn::And": "other value"} + + self.assertFalse(is_valid_intrinsic_if_items(not_enough_items)) + self.assertFalse(is_valid_intrinsic_if_items(is_string)) + self.assertFalse(is_valid_intrinsic_if_items(None)) + self.assertFalse(is_valid_intrinsic_if_items(is_integer)) + self.assertFalse(is_valid_intrinsic_if_items(is_boolean)) + self.assertFalse(is_valid_intrinsic_if_items(is_dict)) From d071243c1c9347462723a8a5e6de868c37faeee0 Mon Sep 17 00:00:00 2001 From: Mathieu Grandis Date: Wed, 17 Feb 2021 17:34:23 -0800 Subject: [PATCH 10/10] refactor: Final refactor --- samtranslator/model/function_policies.py | 8 +++++--- samtranslator/model/intrinsics.py | 15 +++++++------- .../deployment_preference_collection.py | 5 ++--- samtranslator/model/resource_policies.py | 8 +++++--- tests/test_intrinsics.py | 20 +++++++++---------- 5 files changed, 30 insertions(+), 26 deletions(-) diff --git a/samtranslator/model/function_policies.py b/samtranslator/model/function_policies.py index 2f1000b99a..1866cd9921 100644 --- a/samtranslator/model/function_policies.py +++ b/samtranslator/model/function_policies.py @@ -7,7 +7,7 @@ is_intrinsic, is_intrinsic_if, is_intrinsic_no_value, - is_valid_intrinsic_if_items, + validate_intrinsic_if_items, ) from samtranslator.model.exceptions import InvalidTemplateException @@ -170,8 +170,10 @@ def _get_type_from_intrinsic_if(self, policy): """ intrinsic_if_value = policy["Fn::If"] - if not is_valid_intrinsic_if_items(intrinsic_if_value): - raise InvalidTemplateException("Fn::If requires 3 arguments") + try: + validate_intrinsic_if_items(intrinsic_if_value) + except ValueError as e: + raise InvalidTemplateException(e) if_data = intrinsic_if_value[1] else_data = intrinsic_if_value[2] diff --git a/samtranslator/model/intrinsics.py b/samtranslator/model/intrinsics.py index 6ef99d088c..c81e40d00a 100644 --- a/samtranslator/model/intrinsics.py +++ b/samtranslator/model/intrinsics.py @@ -162,21 +162,22 @@ def is_intrinsic_if(input): return key == "Fn::If" -def is_valid_intrinsic_if_items(items): +def validate_intrinsic_if_items(items): """ - Returns a value indicating whether the items are valid Fn::If items or not + Validates Fn::If items Parameters ---------- items : list Fn::If items - Returns - ------- - boolean - True if valid items, False otherwise + Raises + ------ + ValueError + If the items are invalid """ - return isinstance(items, list) and len(items) == 3 + if not isinstance(items, list) or len(items) != 3: + raise ValueError("Fn::If requires 3 arguments") def is_intrinsic_no_value(input): diff --git a/samtranslator/model/preferences/deployment_preference_collection.py b/samtranslator/model/preferences/deployment_preference_collection.py index aba313fc53..2387931541 100644 --- a/samtranslator/model/preferences/deployment_preference_collection.py +++ b/samtranslator/model/preferences/deployment_preference_collection.py @@ -8,7 +8,7 @@ is_intrinsic, is_intrinsic_if, is_intrinsic_no_value, - is_valid_intrinsic_if_items, + validate_intrinsic_if_items, ) from samtranslator.model.update_policy import UpdatePolicy from samtranslator.translator.arn_generator import ArnGenerator @@ -183,8 +183,7 @@ def _convert_alarms(self, preference_alarms): if is_intrinsic_if(preference_alarms): processed_alarms = copy.deepcopy(preference_alarms) alarms_list = processed_alarms.get("Fn::If") - if not is_valid_intrinsic_if_items(alarms_list): - raise ValueError("Fn::If requires 3 arguments") + validate_intrinsic_if_items(alarms_list) alarms_list[1] = self._build_alarm_configuration(alarms_list[1]) alarms_list[2] = self._build_alarm_configuration(alarms_list[2]) return processed_alarms diff --git a/samtranslator/model/resource_policies.py b/samtranslator/model/resource_policies.py index 3f2478fef2..7bf3a23b18 100644 --- a/samtranslator/model/resource_policies.py +++ b/samtranslator/model/resource_policies.py @@ -7,7 +7,7 @@ is_intrinsic, is_intrinsic_if, is_intrinsic_no_value, - is_valid_intrinsic_if_items, + validate_intrinsic_if_items, ) from samtranslator.model.exceptions import InvalidTemplateException @@ -170,8 +170,10 @@ def _get_type_from_intrinsic_if(self, policy): """ intrinsic_if_value = policy["Fn::If"] - if not is_valid_intrinsic_if_items(intrinsic_if_value): - raise InvalidTemplateException("Fn::If requires 3 arguments") + try: + validate_intrinsic_if_items(intrinsic_if_value) + except ValueError as e: + raise InvalidTemplateException(e) if_data = intrinsic_if_value[1] else_data = intrinsic_if_value[2] diff --git a/tests/test_intrinsics.py b/tests/test_intrinsics.py index 06c7447fc6..4de2e82de4 100644 --- a/tests/test_intrinsics.py +++ b/tests/test_intrinsics.py @@ -5,7 +5,7 @@ is_intrinsic, make_shorthand, is_intrinsic_if, - is_valid_intrinsic_if_items, + validate_intrinsic_if_items, is_intrinsic_no_value, ) @@ -65,19 +65,19 @@ def test_is_intrinsic_if_must_return_false_for_others(self): self.assertFalse(is_intrinsic_if(not_if)) self.assertFalse(is_intrinsic_if(None)) - def test_is_valid_intrinsic_if_items_valid(self): - self.assertTrue(is_valid_intrinsic_if_items(["Condition", "Then", "Else"])) + def test_validate_intrinsic_if_items_valid(self): + validate_intrinsic_if_items(["Condition", "Then", "Else"]) - def test_is_valid_intrinsic_if_items_invalid(self): + def test_validate_intrinsic_if_items_invalid(self): not_enough_items = ["Then", "Else"] is_string = "Then" is_integer = 3 is_boolean = True is_dict = {"Fn::If": "some value", "Fn::And": "other value"} - self.assertFalse(is_valid_intrinsic_if_items(not_enough_items)) - self.assertFalse(is_valid_intrinsic_if_items(is_string)) - self.assertFalse(is_valid_intrinsic_if_items(None)) - self.assertFalse(is_valid_intrinsic_if_items(is_integer)) - self.assertFalse(is_valid_intrinsic_if_items(is_boolean)) - self.assertFalse(is_valid_intrinsic_if_items(is_dict)) + self.assertRaises(ValueError, validate_intrinsic_if_items, not_enough_items) + self.assertRaises(ValueError, validate_intrinsic_if_items, is_string) + self.assertRaises(ValueError, validate_intrinsic_if_items, None) + self.assertRaises(ValueError, validate_intrinsic_if_items, is_integer) + self.assertRaises(ValueError, validate_intrinsic_if_items, is_boolean) + self.assertRaises(ValueError, validate_intrinsic_if_items, is_dict)