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

Jump to bottom

Add support for cf and codepipeline parameters file to cf deploy #5443

Merged
merged 4 commits into from Aug 10, 2020
Merged

Add support for cf and codepipeline parameters file to cf deploy #5443

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ef5ba82
Add support for cf and codepipeline parameters file formats to cf dep…
Aug 1, 2020
d95d284
Add inline json support and functional tests
Aug 6, 2020
1d928af
Update docstring and examples
Aug 6, 2020
4d56f51
Improve parsers and tests
Aug 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changes/next-release/feature-cloudformation-43713.json
View file
Open in desktop
@@ -0,0 +1,5 @@
{
"type": "feature",
Copy link
Member

Choose a reason for hiding this comment

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

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.

"category": "``cloudformation``",
"description": "CloudFormation ``deploy`` command now supports various JSON file formats as an input for ``--parameter-overrides`` option."
Copy link
Member

Choose a reason for hiding this comment

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

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

}
92 changes: 81 additions & 11 deletions awscli/customizations/cloudformation/deploy.py
View file
Open in desktop
Expand Up @@ -11,13 +11,16 @@
# ANY KIND, either express or implied. See the License for the specific
# language governing permissions and limitations under the License.

import functools
import json
import os
import sys
import logging

from botocore.client import Config

from awscli.compat import compat_open
from awscli.customizations.exceptions import ParamValidationError
from awscli.customizations.cloudformation import exceptions
from awscli.customizations.cloudformation.deployer import Deployer
from awscli.customizations.s3uploader import S3Uploader
Expand Down Expand Up @@ -107,12 +110,7 @@ class DeployCommand(BasicCommand):
'name': PARAMETER_OVERRIDE_CMD,
'action': 'store',
'required': False,
'schema': {
'type': 'array',
'items': {
'type': 'string'
}
},
'nargs': '+',
'default': [],
'help_text': (
'A list of parameter structures that specify input parameters'
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

We also should add a changelog entry for this.

Expand All @@ -121,7 +119,7 @@ class DeployCommand(BasicCommand):
' existing value. For new stacks, you must specify'
' parameters that don\'t have a default value.'
' Syntax: ParameterKey1=ParameterValue1'
' ParameterKey2=ParameterValue2 ...'
' ParameterKey2=ParameterValue2 ... or JSON file (see Examples)'
)
},
{
Expand Down Expand Up @@ -255,10 +253,9 @@ def _run_main(self, parsed_args, parsed_globals):
template_str = handle.read()

stack_name = parsed_args.stack_name
parameter_overrides = self.parse_key_value_arg(
parsed_args.parameter_overrides,
self.PARAMETER_OVERRIDE_CMD)

parameter_overrides = self.parse_parameter_overrides(
parsed_args.parameter_overrides
)
tags_dict = self.parse_key_value_arg(parsed_args.tags, self.TAGS_CMD)
tags = [{"Key": key, "Value": value}
for key, value in tags_dict.items()]
Expand Down Expand Up @@ -359,6 +356,79 @@ 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)

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

def _parse_input_as_json(self, arg_value):
# 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):
return json.loads(arg_value)
Comment on lines +428 to +429
Copy link
Member

Choose a reason for hiding this comment

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

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

try:
return json.loads(arg_value[0])
except json.JSONDecodeError:
return None

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,

]
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
Copy link
Member

Choose a reason for hiding this comment

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

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.

else:
Copy link
Member

Choose a reason for hiding this comment

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

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.

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'm not sure anyone will do it but to be consistent we need to allow it, I think

# In case it was in deploy command format
# and was input via command line
return self.parse_key_value_arg(
arg_value,
self.PARAMETER_OVERRIDE_CMD
)

def parse_key_value_arg(self, arg_value, argname):
"""
Converts arguments that are passed as list of "Key=Value" strings
Expand Down
37 changes: 37 additions & 0 deletions awscli/examples/cloudformation/deploy.rst
View file
Open in desktop
Expand Up @@ -4,3 +4,40 @@ Following command deploys template named ``template.json`` to a stack named

aws cloudformation deploy --template-file /path_to_template/template.json --stack-name my-new-stack --parameter-overrides Key1=Value1 Key2=Value2 --tags Key1=Value1 Key2=Value2

or the same command using parameters from JSON file ``parameters.json``::

aws cloudformation deploy --template-file /path_to_template/template.json --stack-name my-new-stack --parameter-overrides file://path_to_parameters/parameters.json --tags Key1=Value1 Key2=Value2

Supported JSON syntax
~~~~~~~~~~~~~~~~~~~~~

Original format::

[
"Key1=Value1",
"Key2=Value2"
]

CloudFormation like format::

[
{
"ParameterKey": "Key1",
"ParameterValue": "Value1",
},
{
"ParameterKey": "Key2",
"ParameterValue": "Value2",
Copy link
Member

Choose a reason for hiding this comment

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

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

},
]

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

Choose a reason for hiding this comment

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

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.


CodePipeline like format::

[
"Parameters": {
"Key1": "Value1",
"Key2": "Value2"
}
]
119 changes: 119 additions & 0 deletions tests/functional/cloudformation/test_deploy.py
View file
Open in desktop
Expand Up @@ -60,3 +60,122 @@ def test_does_return_zero_exit_code_on_empty_changeset(self):
def test_does_return_non_zero_exit_code_on_empty_changeset(self):
self.command += ' --fail-on-empty-changeset'
self.run_cmd(self.command, expected_rc=255)


class TestDeployCommandParameterOverrides(TestDeployCommand):
def setUp(self):
super(TestDeployCommandParameterOverrides, self).setUp()
template = '''{
"AWSTemplateFormatVersion": "2010-09-09",
"Parameters": {
"Key1": {
"Type": "String"
},
"Key2": {
"Type": "String"
}
}
}'''
Comment on lines +70 to +80
Copy link
Member

Choose a reason for hiding this comment

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

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).

path = self.files.create_file('template.json', template)
self.command = (
'cloudformation deploy --template-file %s '
'--stack-name Stack '
) % path

def _assert_parameters_parsed(self):
self.assertEqual(
self.operations_called[1][1]['Parameters'],
[
{'ParameterKey': 'Key1', 'ParameterValue': 'Value1'},
{'ParameterKey': 'Key2', 'ParameterValue': 'Value2'}
]
)

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)
self.command += ' --parameter-overrides file://%s' % path
self.run_cmd(self.command)
self._assert_parameters_parsed()

def test_parameter_overrides_from_inline_cf_like_json(self):
cf_like_json = ('[{"ParameterKey":"Key1",'
'"ParameterValue":"Value1"},'
'{"ParameterKey":"Key2",'
'"ParameterValue":"Value2"}]')
self.command += ' --parameter-overrides %s' % cf_like_json
self.run_cmd(self.command)
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)
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"}}')
self.command += ' --parameter-overrides %s' % codepipeline_like_json
self.run_cmd(self.command)
self._assert_parameters_parsed()

def test_parameter_overrides_from_codepipeline_like_json_file(self):
codepipeline_like_json = '''
{
"Parameters":
{"Key1": "Value1",
"Key2": "Value2"}
}
'''
path = self.files.create_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)
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)
self.command += ' --parameter-overrides file://%s' % path
_, err, _ = self.run_cmd(self.command, expected_rc=255)
self.assertTrue("['SomeKey'] value passed to --parameter-overrides"
in err)