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.
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
Add support for cf and codepipeline parameters file to cf deploy #5443
Conversation
d23c1e8
to
e713790
Compare
Codecov Report
@@ Coverage Diff @@
## v2 #5443 +/- ##
=======================================
Coverage 94.47% 94.48%
=======================================
Files 234 234
Lines 17838 17887 +49
=======================================
+ Hits 16853 16900 +47
- Misses 985 987 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a good start. Just had some suggestions to build on it.
""" | ||
Tests that we can parse parameter arguments from file in | ||
CloudFormation parameters file format | ||
:return: | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
self.PARAMETER_OVERRIDE_CMD | ||
) | ||
return result | ||
else: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
'type': 'string' | ||
} | ||
}, | ||
'nargs': '+', | ||
'default': [], | ||
'help_text': ( | ||
'A list of parameter structures that specify input parameters' |
There was a problem hiding this comment.
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.
'type': 'string' | ||
} | ||
}, | ||
'nargs': '+', | ||
'default': [], | ||
'help_text': ( | ||
'A list of parameter structures that specify input parameters' |
There was a problem hiding this comment.
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.
output = {"Key1": "Value1", | ||
"Key2": '[1,2,3]', | ||
"Key3": '{"a":"val", "b": 2}'} | ||
result = self.deploy_command.parse_parameter_overrides(data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
# } | ||
if isinstance(data, dict): | ||
return data.get('Parameters', None) | ||
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
8175c75
to
889219a
Compare
889219a
to
1d928af
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. It's looking good. I just had a few more suggestions.
@@ -0,0 +1,5 @@ | |||
{ | |||
"type": "feature", |
There was a problem hiding this comment.
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.
{ | ||
"type": "feature", | ||
"category": "``cloudformation``", | ||
"description": "CloudFormation ``deploy`` command now supports various JSON file formats as an input for ``--parameter-overrides`` option." |
There was a problem hiding this comment.
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.
template = '''{ | ||
"AWSTemplateFormatVersion": "2010-09-09", | ||
"Parameters": { | ||
"Key1": { | ||
"Type": "String" | ||
}, | ||
"Key2": { | ||
"Type": "String" | ||
} | ||
} | ||
}''' |
There was a problem hiding this comment.
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).
}, | ||
{ | ||
"ParameterKey": "Key2", | ||
"ParameterValue": "Value2", |
There was a problem hiding this comment.
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.
# } | ||
if isinstance(data, dict): | ||
return data.get('Parameters', None) | ||
|
There was a problem hiding this comment.
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.
if result is None: | ||
return self.parse_key_value_arg( | ||
data, | ||
self.PARAMETER_OVERRIDE_CMD | ||
) | ||
return result |
There was a problem hiding this comment.
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.
}, | ||
] | ||
|
||
Note: Only ParameterKey and ParameterValue are expected keys, command will through an exception if receives unexpected keys (e.g. UsePreviousValue or ResolvedValue). |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. Just had a few really small comments and we should be good to merge this.
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): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably keep this test but just make sure we assert that the new error message is coming back.
if parser.can_parse(data): | ||
return parser.parse(data) | ||
raise ParamValidationError( | ||
'JSON passed to --parameter-overrides must be in of ' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of must be in of
let's change it to must be one of
if isinstance(arg_value, str): | ||
return json.loads(arg_value) |
There was a problem hiding this comment.
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.
|
||
.. note:: | ||
|
||
Only ParameterKey and ParameterValue are expected keys, command will through an exception if receives unexpected keys (e.g. UsePreviousValue or ResolvedValue). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: through
-> throw
56cb38a
to
4d56f51
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 🚢
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
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
***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 namedmy-new-stack
:You can do the same with inline json
Or you can do the same using JSON file
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.