Add support for cf and codepipeline parameters file to cf deploy by vz10 · Pull Request #5443 · aws/aws-cli

vz10 · 2020-08-04T19:07:43Z

Add support for cf and codepipeline parameters file to cf deploy command

Issue #, if available: #2828

Description of changes:

Current state:

The current format for “—parameter-overrides“ option is

ParameterKey1=ParameterValue1 ParameterKey2=ParameterKey2

or JSON format

  [
    "ParameterKey1=ParameterValue1",
    "ParameterKey2=ParameterKey2"
  ]

Proposed improvements:

Keep the shorthand format without changing but update the file parser to accept data in “CloudFromation format” and “CodePipeline format”, it means such JSON formats will be valid as well

“CloudFromation format”

[
   {
        "ParameterKey": "ParameterKey1",
        "ParameterValue": "ParameterValue1",
    },
    {
        "ParameterKey": "ParameterKey2",
        "ParameterValue": "ParameterValue2",
    },
...
]

“CodePipeline format”

[
    "Parameters": {
        "ParameterKey1": "ParameterValue1",
        "ParameterKey2": "ParameterValue2"`
    }

...
]

***Note: *the logic of the “—parameter-overrides“ option won’t be changed, it means that it will expect only parameter’s keys and values and throw ValidationError if get unexpected keys (e. g. UsePreviousValue or **ResolvedValue **which exist in CloudFormation format).. If parameter not set will be used previous value if stack is updated or default value for stack creation.

Examples

Following command deploys template named template.json to a stack named my-new-stack:

aws cloudformation deploy --template-file /path_to_template/template.json --stack-name my-new-stack --parameter-overrides BucketName=MyBucket Tag1value=awesomeBucket

template.json{
  "AWSTemplateFormatVersion": "2010-09-09",
  "Description": "AWS CloudFormation Sample S3 bucket Template.",
  "Parameters": {
    "BucketName": {
      "Description": "Name of s3 Bucket",
      "Type": "String"
    },
    "Tag1value": {
      "Description": "Tag1 value",
      "Default": "value1",
      "Type": "String"
    },
    "Tag2value": {
      "Description": "Tag2 value",
      "Default": "value2",
      "Type": "String"
    }
  },
  "Resources": {
    "s3bucket": {
      "Type": "AWS::S3::Bucket",
      "Properties": {
        "BucketName": {"Ref": "BucketName"},
        "Tags": [
          {
            "Key": "Tag1",
            "Value": {"Ref": "Tag1value"}
          },
          {
            "Key": "Tag2",
            "Value": {"Ref": "Tag2value"}
          }
        ]
      }
    }
  }
}

You can do the same with inline json

aws cloudformation deploy --template-file /path_to_template/template.json --stack-name my-new-stack --parameter-overrides '["BucketName=MyBucket", "Tag1value=awesomeBucket"]'

Or you can do the same using JSON file

aws cloudformation deploy --template-file /path_to_template/template.json --stack-name my-new-stack --parameter-overrides file://parameter.json 

parameter.json in CloudFormation format
[
   {
        "ParameterKey": "BucketName",
        "ParameterValue": "MyBucket",
    },
    {
        "ParameterKey": "Tag1value",
        "ParameterValue": "awesomeBucket",
    },
]

parameter.json in CodePipeline format
[
    "Parameters": {
        "BucketName": "MyBucket",
        "Tag1value": "awesomeBucket"
    }
]

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

codecov-commenter · 2020-08-04T23:23:27Z

Codecov Report

Merging #5443 into v2 will increase coverage by 0.00%.
The diff coverage is 96.00%.

@@           Coverage Diff           @@
##               v2    #5443   +/-   ##
=======================================
  Coverage   94.47%   94.48%           
=======================================
  Files         234      234           
  Lines       17838    17887   +49     
=======================================
+ Hits        16853    16900   +47     
- Misses        985      987    +2

Impacted Files	Coverage Δ
awscli/customizations/cloudformation/deploy.py	`98.42% <96.00%> (-1.58%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7d1487...4d56f51. Read the comment docs.

kyleknap

Looks like a good start. Just had some suggestions to build on it.

kyleknap · 2020-08-05T22:23:19Z

tests/unit/customizations/cloudformation/test_deploy.py

+        """
+        Tests that we can parse parameter arguments from file in
+        CloudFormation parameters file format
+        :return:
+        """


I think the cloudformation package/deploy commands are a bit of an outlier, but typically we avoid putting docstrings on the test cases. The name of the test, input, and expected output should speak for themselves.

kyleknap · 2020-08-05T22:44:25Z

awscli/customizations/cloudformation/deploy.py

+                    self.PARAMETER_OVERRIDE_CMD
+                )
+            return result
+        else:


I'm wondering if we should allow users to be able to send these new formats through the command line as JSON? For example, this should probably work:

aws cloudformation deploy \ --stack-name test-cfn \ --template-file template.json \ --parameter-overrides "[{\"ParameterKey\":\"Tag1value\",\"ParameterValue\":\"awesomeBucket\"}]"

Generally, the expectation for CLI usage is that if it can be specified as a file, it can also be specified directly to the command line. While I expect most people to be using the file, I do not think we should be limiting the ability to pass the new formats in directly to the command line as users may be confused by this inconsistency and they may not necessarily want to write out a file in order to run a command.

I'm not sure anyone will do it but to be consistent we need to allow it, I think

kyleknap · 2020-08-05T22:45:11Z

awscli/customizations/cloudformation/deploy.py

+            'nargs': '+',
            'default': [],
            'help_text': (
                'A list of parameter structures that specify input parameters'


Just a reminder we need doc updates for these new formats.

kyleknap · 2020-08-05T22:45:29Z

awscli/customizations/cloudformation/deploy.py

+            'nargs': '+',
            'default': [],
            'help_text': (
                'A list of parameter structures that specify input parameters'


We also should add a changelog entry for this.

kyleknap · 2020-08-05T22:55:35Z

tests/unit/customizations/cloudformation/test_deploy.py

+        output = {"Key1": "Value1",
+                  "Key2": '[1,2,3]',
+                  "Key3": '{"a":"val", "b": 2}'}
+        result = self.deploy_command.parse_parameter_overrides(data)


The unit tests are fine here, but I think we should probably have extensive functional tests here to make sure that this parsing logic syncs up with how the values are initially parsed by the main CLI parser. Specifically, I think we should have all of the following cases (assuming we accept in-line JSON for the new formats):

Short-hand syntax for original --parameter-overrides

In-line JSON for original --parameter-overrides

File-based syntax for original --parameter-overrides

In-line JSON for “CloudFromation format”

File-based JSON for “CloudFromation format”

Validation of invalid “CloudFromation format”

In-line JSON for “CodePipeline format”

File-based JSON for “CodePipeline format”

Validate against just generally invalid JSON documents that do not follow any of the three valid schemas.

I realize the cloudformation deploy commands do not have great functional tests, but this will help ensure we do not introduce any regressions now or in the future for the various options we support.

kyleknap · 2020-08-05T23:09:45Z

awscli/customizations/cloudformation/deploy.py

+        # }
+        if isinstance(data, dict):
+            return data.get('Parameters', None)
+


Left this off of my last review... Should we validate that it does not have any of the top-level keys like Tags and StackPolicy? I think it makes sense because the CLI is generally strict on making sure all parameters provided will be used.

It's discussible, if we take only Parameters key and just ignore the rest it'll allow customers to reuse the same files without charging

Yeah that's fair given this change is more of about expanding compatibility.

…loy command

kyleknap

Nice. It's looking good. I just had a few more suggestions.

kyleknap · 2020-08-07T19:53:29Z

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

@@ -0,0 +1,5 @@
+{
+  "type": "feature",


Let's change this to an enhancement. Typically, feature means we will do a minor version bump and I'm not sure if this update warrants one.

kyleknap · 2020-08-07T19:53:47Z

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

+{
+  "type": "feature",
+  "category": "``cloudformation``",
+  "description": "CloudFormation ``deploy`` command now supports various JSON file formats as an input for ``--parameter-overrides`` option."


We should probably link the GitHub issue that it fixes in the description.

kyleknap · 2020-08-07T20:04:02Z

tests/functional/cloudformation/test_deploy.py

+        template = '''{
+          "AWSTemplateFormatVersion": "2010-09-09",
+          "Parameters": {
+            "Key1": {
+              "Type": "String"
+            },
+            "Key2": {
+              "Type": "String"
+            }
+          }
+        }'''


For any time we are writing a JSON document out to a file, it may make sense to expose a create_json_file method on the test class that accepts the content of the json document as a dictionary. So for example here:

template = { "AWSTemplateFormatVersion": "2010-09-09", "Parameters": { "Key1": { "Type": "String" }, "Key2": { "Type": "String" } } } path = self.create_json_file('template.json', template) ...

Specifically when the template or parameters is in the form of python dictionary it is a bit easier to work with (e.g. you don't have to worry about getting JSON syntax correctly) and expand on it (e.g you want to add or remove keys from some default JSON document).

kyleknap · 2020-08-07T20:04:50Z

awscli/examples/cloudformation/deploy.rst

+        },
+        {
+            "ParameterKey": "Key2",
+            "ParameterValue": "Value2",


I'm not sure if this is valid JSON with the trailing commas.

kyleknap · 2020-08-07T20:06:02Z

awscli/customizations/cloudformation/deploy.py

+        # }
+        if isinstance(data, dict):
+            return data.get('Parameters', None)
+


Yeah that's fair given this change is more of about expanding compatibility.

kyleknap · 2020-08-07T21:12:21Z

awscli/customizations/cloudformation/deploy.py

+            if result is None:
+                return self.parse_key_value_arg(
+                    data,
+                    self.PARAMETER_OVERRIDE_CMD
+                )
+            return result


So I'm not entire sure we should be relying on the old parsing behavior as the final attempt and not provide any helpful error message after it. Specifically playing around with this I got some really weird looking errors.

In this example, I accidentally misspelled ParameterValue as ParameterVlue

$ aws cloudformation deploy --stack-name test-cfn --template-file template.json --parameter-overrides "[{\"ParameterKey\":\"BucketName\",\"ParameterVlue\":\"kyleknap-test-cfn-bucket\"},{\"ParameterKey\":\"Tag1value\",\"ParameterValue\":\"awesomeBucket\"}]" 'dict' object has no attribute 'split'

Then even in the test case for just a generally bad structure, this message is confusing:

$ aws cloudformation deploy --stack-name test-cfn --template-file template.json --parameter-overrides '{"SomeKey":[{"RedundantKey": "RedundantValue"}]}' ['SomeKey'] value passed to --parameter-overrides must be of format Key=Value

In general, it might make sense to abstract this logic a bit into classes/this format:

class BaseParameterOverrideParser: def can_parse(self, data): # Returns true/false if it can parse def parse(self, data): # Return the properly formatted parameter dictionary # Example parser class CodePipelineParameterOverrideParser(BaseParameterOverrideParser): def can_parse(self, data): return isinstance(data, dict) and 'Parameters' in data def parse(self, data): return data['Parameters'] # In the DeployCommand class def parse_parameter_overrides(self, arg_value): # After checking if it is json: parsers = [ CfnParameterOverrideParser(), # maybe find better names here CodePipelineParameterOverrideParser(), StringEqualsParameterOverrideParser() ] for parser in parsers: if parser.can_parse(data): return parser.parse(data) raise InvalidParameterOverrideFormat('Show helpful information about the accepted formats')

The advantages with this approach is that:

We can avoid weird/unexpected errors from trying to parse a format that will not work.

We can throw a more helpful error about generally incorrect parameter formats instead of relying on any one specific parser errors.

kyleknap · 2020-08-07T21:19:39Z

awscli/examples/cloudformation/deploy.rst

+        },
+    ]
+
+    Note: Only ParameterKey and ParameterValue are expected keys, command will through an exception if receives unexpected keys (e.g. UsePreviousValue or ResolvedValue).


Another thing I noticed when I generated the docs we should use the reStructuredText note e.g. (.. note::) so that this wording does not get captured in the codeblock.

kyleknap

Awesome. Just had a few really small comments and we should be good to merge this.

kyleknap · 2020-08-07T22:28:20Z

tests/functional/cloudformation/test_deploy.py

        self.run_cmd(self.command)
        self._assert_parameters_parsed()

-    def test_parameter_overrides_from_invalid_cf_like_json_file(self):


We should probably keep this test but just make sure we assert that the new error message is coming back.

kyleknap · 2020-08-07T22:29:48Z

awscli/customizations/cloudformation/deploy.py

+                if parser.can_parse(data):
+                    return parser.parse(data)
+            raise ParamValidationError(
+                'JSON passed to --parameter-overrides must be in of '


Instead of must be in of let's change it to must be one of

kyleknap · 2020-08-07T22:33:57Z

awscli/customizations/cloudformation/deploy.py

+            if isinstance(arg_value, str):
+                return json.loads(arg_value)


Then is this the case where it got loaded from a file? If so, it might be good to include that in the comment.

kyleknap · 2020-08-07T22:34:15Z

awscli/examples/cloudformation/deploy.rst

+
+.. note::
+
+ Only ParameterKey and ParameterValue are expected keys, command will through an exception if receives unexpected keys (e.g. UsePreviousValue or ResolvedValue).


Typo: through -> throw

kyleknap

Looks good! 🚢

vz10 requested review from kyleknap and stealthycoin August 4, 2020 19:07

vz10 force-pushed the cf_deploy_parameters_file branch from d23c1e8 to e713790 Compare August 4, 2020 23:16

kdaily mentioned this pull request Aug 5, 2020

cli-input-json support for aws cloudformation deploy #5448

Open

kyleknap reviewed Aug 5, 2020

View reviewed changes

Illia Batozskyi added 2 commits August 6, 2020 18:54

Add support for cf and codepipeline parameters file formats to cf dep…

ef5ba82

…loy command

Add inline json support and functional tests

d95d284

vz10 force-pushed the cf_deploy_parameters_file branch from 8175c75 to 889219a Compare August 7, 2020 00:37

Update docstring and examples

1d928af

vz10 force-pushed the cf_deploy_parameters_file branch from 889219a to 1d928af Compare August 7, 2020 00:43

kyleknap reviewed Aug 7, 2020

View reviewed changes

Improve parsers and tests

4d56f51

vz10 force-pushed the cf_deploy_parameters_file branch from 56cb38a to 4d56f51 Compare August 7, 2020 23:57

kyleknap approved these changes Aug 10, 2020

View reviewed changes

vz10 merged commit a206042 into aws:v2 Aug 10, 2020

vz10 deleted the cf_deploy_parameters_file branch August 10, 2020 17:29

whereisaaron mentioned this pull request Apr 5, 2021

sam deploy should accept parameter file just like aws cloudformation *-stack aws/aws-sam-cli#2054

Closed


		.. note::

		Only ParameterKey and ParameterValue are expected keys, command will through an exception if receives unexpected keys (e.g. UsePreviousValue or ResolvedValue).

Conversation

vz10 commented Aug 4, 2020

Current state:

Proposed improvements:

Examples

Uh oh!

codecov-commenter commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kyleknap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyleknap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyleknap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kyleknap left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Aug 4, 2020 •

edited

Loading