Improve parsers and tests

.changes/next-release/feature-cloudformation-43713.json

@@ -1,5 +1,5 @@
 {
-  "type": "feature",
+  "type": "enhancement",
   "category": "``cloudformation``",
-  "description": "CloudFormation ``deploy`` command now supports various JSON file formats as an input for ``--parameter-overrides`` option."
+  "description": "CloudFormation ``deploy`` command now supports various JSON file formats as an input for ``--parameter-overrides`` option `#2828 <https://github.com/aws/aws-cli/issues/2828>`__"
 }

awscli/customizations/cloudformation/deploy.py

@@ -33,6 +33,70 @@
 LOG = logging.getLogger(__name__)
 
 
+class BaseParameterOverrideParser:
+    def can_parse(self, data):
+        # Returns true/false if it can parse
+        raise NotImplementedError('can_parse')
+
+    def parse(self, data):
+        # Return the properly formatted parameter dictionary
+        raise NotImplementedError('parse')
+
+
+class CodePipelineLikeParameterOverrideParser(BaseParameterOverrideParser):
+    def can_parse(self, data):
+        return isinstance(data, dict) and 'Parameters' in data
+
+    def parse(self, data):
+        # Parse parameter_overrides if they were given in
+        # CodePipeline params file format
+        # {
+        #     "Parameters": {
+        #         "ParameterKey": "ParameterValue"
+        #     }
+        # }
+        return data['Parameters']
+
+
+class CloudFormationLikeParameterOverrideParser(BaseParameterOverrideParser):
+    def can_parse(self, data):
+        for param_pair in data:
+            if ('ParameterKey' not in param_pair or
+                    'ParameterValue' not in param_pair):
+                return False
+            if len(param_pair.keys()) > 2:
+                return False
+        return True
+
+    def parse(self, data):
+        # Parse parameter_overrides if they were given in
+        # CloudFormation params file format
+        # [{
+        #   "ParameterKey": "string",
+        #   "ParameterValue": "string",
+        # }]
+        return {
+            param['ParameterKey']: param['ParameterValue']
+            for param in data
+        }
+
+
+class StringEqualsParameterOverrideParser(BaseParameterOverrideParser):
+    def can_parse(self, data):
+        return all(
+            isinstance(param, str) and len(param.split("=", 1)) == 2
+            for param in data
+        )
+
+    def parse(self, data):
+        result = {}
+        for param in data:
+            # Split at first '=' from left
+            key_value_pair = param.split("=", 1)
+            result[key_value_pair[0]] = key_value_pair[1]
+        return result
+
+
 class DeployCommand(BasicCommand):
 
     MSG_NO_EXECUTE_CHANGESET = \
@@ -356,42 +420,9 @@ def merge_parameters(self, template_dict, parameter_overrides):
 
         return parameter_values
 
-    def _validate_cf_params(self, param):
-        if 'ParameterKey' in param and 'ParameterValue' in param:
-            if len(param.keys()) > 2:
-                raise ParamValidationError(
-                    'CloudFormation like parameter JSON should have format '
-                    '[{"ParameterKey": "string", "ParameterValue": "string"}]')
-        return True
-
-    def _cf_param_parser(self, data):
-        # Parse parameter_overrides if they were given in
-        # CloudFormation params file format
-        # [{
-        #   "ParameterKey": "string",
-        #   "ParameterValue": "string",
-        # }]
-        try:
-            return {
-                param['ParameterKey']: param['ParameterValue']
-                for param in data if self._validate_cf_params(param)
-            }
-        except (KeyError, TypeError):
-            return None
-
-    def _codepipeline_param_parser(self, data):
-        # Parse parameter_overrides if they were given in
-        # CodePipeline params file format
-        # {
-        #     "Parameters": {
-        #         "ParameterKey": "ParameterValue"
-        #     }
-        # }
-        if isinstance(data, dict):
-            return data.get('Parameters', None)
-
     def _parse_input_as_json(self, arg_value):
-        # In case of inline json input it'll be list where json string
+        # In case of reading from file it'll be string and in case
+        # of inline json input it'll be list where json string
         # will be the first element
         if arg_value:
             if isinstance(arg_value, str):
@@ -405,22 +436,18 @@ def parse_parameter_overrides(self, arg_value):
         data = self._parse_input_as_json(arg_value)
         if data is not None:
             parsers = [
-                self._cf_param_parser,
-                self._codepipeline_param_parser,
-
+                CloudFormationLikeParameterOverrideParser(),
+                CodePipelineLikeParameterOverrideParser(),
+                StringEqualsParameterOverrideParser()
             ]
-            result = functools.reduce(
-                lambda a, parser: a or parser(data),
-                parsers, None
-            )
-            # In case it was in deploy command format
-            # but was in file
-            if result is None:
-                return self.parse_key_value_arg(
-                    data,
-                    self.PARAMETER_OVERRIDE_CMD
-                )
-            return result
+            for parser in parsers:
+                if parser.can_parse(data):
+                    return parser.parse(data)
+            raise ParamValidationError(
+                'JSON passed to --parameter-overrides must be one of '
+                'the formats: ["Key1=Value1","Key2=Value2", ...] , '
+                '[{"ParameterKey": "Key1", "ParameterValue": "Value1"}, ...] , '
+                '["Parameters": {"Key1": "Value1", "Key2": "Value2", ...}]')
         else:
             # In case it was in deploy command format
             # and was input via command line

awscli/examples/cloudformation/deploy.rst

@@ -23,15 +23,17 @@ CloudFormation like format::
     [
        {
             "ParameterKey": "Key1",
-            "ParameterValue": "Value1",
+            "ParameterValue": "Value1"
         },
         {
             "ParameterKey": "Key2",
-            "ParameterValue": "Value2",
-        },
+            "ParameterValue": "Value2"
+        }
     ]
 
-    Note: Only ParameterKey and ParameterValue are expected keys, command will through an exception if receives unexpected keys (e.g. UsePreviousValue or ResolvedValue).
+.. note::
+
+ Only ParameterKey and ParameterValue are expected keys, command will throw an exception if receives unexpected keys (e.g. UsePreviousValue or ResolvedValue).
 
 CodePipeline like format::
 

tests/functional/cloudformation/test_deploy.py

@@ -10,6 +10,8 @@
 # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF
 # ANY KIND, either express or implied. See the License for the specific
 # language governing permissions and limitations under the License.
+import json
+
 from awscli.testutils import BaseAWSCommandParamsTest
 from awscli.testutils import FileCreator
 
@@ -91,14 +93,17 @@ def _assert_parameters_parsed(self):
             ]
         )
 
+    def create_json_file(self, filename, data):
+        return self.files.create_file(filename, json.dumps(data))
+
     def test_parameter_overrides_shorthand(self):
         self.command += ' --parameter-overrides Key1=Value1 Key2=Value2'
         self.run_cmd(self.command)
         self._assert_parameters_parsed()
 
     def test_parameter_overrides_from_inline_original_json(self):
-        original_like_json = '["Key1=Value1","Key2=Value2"]'
-        path = self.files.create_file('param.json', original_like_json)
+        original_like_json = ['Key1=Value1', 'Key2=Value2']
+        path = self.create_json_file('param.json', original_like_json)
         self.command += ' --parameter-overrides file://%s' % path
         self.run_cmd(self.command)
         self._assert_parameters_parsed()
@@ -113,33 +118,15 @@ def test_parameter_overrides_from_inline_cf_like_json(self):
         self._assert_parameters_parsed()
 
     def test_parameter_overrides_from_cf_like_json_file(self):
-        cf_like_json = '''
-            [
-                {"ParameterKey": "Key1", "ParameterValue": "Value1"},
-                {"ParameterKey": "Key2", "ParameterValue": "Value2"}
-            ]
-        '''
-        path = self.files.create_file('param.json', cf_like_json)
+        cf_like_json = [
+            {'ParameterKey': 'Key1', 'ParameterValue': 'Value1'},
+            {'ParameterKey': 'Key2', 'ParameterValue': 'Value2'}
+        ]
+        path = self.create_json_file('param.json', cf_like_json)
         self.command += ' --parameter-overrides file://%s' % path
         self.run_cmd(self.command)
         self._assert_parameters_parsed()
 
-    def test_parameter_overrides_from_invalid_cf_like_json_file(self):
-        cf_like_json = '''
-            [
-                {"ParameterKey": "Key1",
-                 "ParameterValue": "Value1",
-                 "RedundantKey": "RedundantValue"
-                 },
-                {"ParameterKey": "Key2", "ParameterValue": "Value2"}
-            ]
-        '''
-        path = self.files.create_file('param.json', cf_like_json)
-        self.command += ' --parameter-overrides file://%s' % path
-        _, err, _ = self.run_cmd(self.command, expected_rc=252)
-        self.assertTrue('CloudFormation like parameter JSON should have format'
-                        in err)
-
     def test_parameter_overrides_from_inline_codepipeline_like_json(self):
         codepipeline_like_json = ('{"Parameters":{"Key1":"Value1",'
                                   '"Key2":"Value2"}}')
@@ -148,34 +135,46 @@ def test_parameter_overrides_from_inline_codepipeline_like_json(self):
         self._assert_parameters_parsed()
 
     def test_parameter_overrides_from_codepipeline_like_json_file(self):
-        codepipeline_like_json = '''
-            {
-                "Parameters":
-                {"Key1": "Value1",
-                 "Key2": "Value2"}
+        codepipeline_like_json = {
+            'Parameters': {
+                'Key1': 'Value1',
+                'Key2': 'Value2'
             }
-        '''
-        path = self.files.create_file('param.json', codepipeline_like_json)
+        }
+        path = self.create_json_file('param.json', codepipeline_like_json)
         self.command += ' --parameter-overrides file://%s' % path
         self.run_cmd(self.command)
         self._assert_parameters_parsed()
 
     def test_parameter_overrides_from_original_json_file(self):
-        original_like_json = '''
-            [
-                "Key1=Value1",
-                "Key2=Value2"
-            ]
-        '''
-        path = self.files.create_file('param.json', original_like_json)
+        original_like_json = ['Key1=Value1', 'Key2=Value2']
+        path = self.create_json_file('param.json', original_like_json)
         self.command += ' --parameter-overrides file://%s' % path
         self.run_cmd(self.command, expected_rc=0)
         self._assert_parameters_parsed()
 
-    def test_parameter_overrides_from_random_invalid_json(self):
-        cf_like_json = '{"SomeKey":[{"RedundantKey": "RedundantValue"}]}'
-        path = self.files.create_file('param.json', cf_like_json)
+    def test_parameter_overrides_from_invalid_cf_like_json_file(self):
+        invalid_cf_like_json = [
+            {
+                'ParameterKey': 'Key1',
+                'ParameterValue': 'Value1',
+                'RedundantKey': 'RedundantValue'
+            },
+            {
+                'ParameterKey': 'Key2',
+                'ParameterValue': 'Value2'
+            }
+        ]
+        path = self.create_json_file('param.json', invalid_cf_like_json)
+        self.command += ' --parameter-overrides file://%s' % path
+        _, err, _ = self.run_cmd(self.command, expected_rc=252)
+        self.assertTrue('JSON passed to --parameter-overrides must be'
+                        in err)
+
+    def test_parameter_overrides_from_invalid_json(self):
+        cf_like_json = {'SomeKey': [{'RedundantKey': 'RedundantValue'}]}
+        path = self.create_json_file('param.json', cf_like_json)
         self.command += ' --parameter-overrides file://%s' % path
-        _, err, _ = self.run_cmd(self.command, expected_rc=255)
-        self.assertTrue("['SomeKey'] value passed to --parameter-overrides"
+        _, err, _ = self.run_cmd(self.command, expected_rc=252)
+        self.assertTrue('JSON passed to --parameter-overrides must be'
                         in err)