Allow the characters _:./+-@= to be used in tag keys during deploy #1798
Conversation
sriram-mv
force-pushed
the
fix-tags-key-characters
branch
from
f94bd3b to
a1059ef
Compare
|
Thanks! Just discovered this when deploying. |
samcli/cli/types.py
Outdated
| def _value_regex(delim): | ||
| return f'(\\"(?:\\\\.|[^\\"\\\\]+)*\\"|(?:\\\\.|[^{delim}\\"\\\\]+)+)' | ||
| def _match_regex(match, delim): | ||
| return f'(\\"(?:\\\\{match}|[^\\"\\\\]+)*\\"|(?:\\\\{match}|[^{delim}\\"\\\\]+)+)' |
There was a problem hiding this comment.
This is one crazy to read regex... :(
There was a problem hiding this comment.
Will add a docstring.
samcli/cli/types.py
Outdated
| @@ -9,15 +9,47 @@ | |||
| import click | |||
|
|
|||
|
|
|||
| def _value_regex(delim): | |||
| return f'(\\"(?:\\\\.|[^\\"\\\\]+)*\\"|(?:\\\\.|[^{delim}\\"\\\\]+)+)' | |||
| def _match_regex(match, delim): | |||
There was a problem hiding this comment.
Can you throw a quick doc string on this? Finding it slightly hard to understand what/why you would need the match here.
|
|
||
| def _unquote(value): | ||
| r""" | ||
| Removes wrapping double quotes and any '\ ' characters. They are usually added to preserve spaces when passing |
There was a problem hiding this comment.
You say "They are usually added". So does that mean there is a case that we wouldn't want to replace the double or '\ ' characters from the input?
There was a problem hiding this comment.
I dont think so, we will want to replace it. "Usually added" maybe a misnomer, preserving spaces inside quotes or even escaping quotes within quotes requires a \ (escape) character.
| """ | ||
| if value and (value[0] == value[-1] == '"'): | ||
| # Remove quotes only if the string is wrapped in quotes | ||
| value = value.strip('"') |
There was a problem hiding this comment.
The intention here is to only remove the first and last quotes right? Do we need to worry about the case where there are two " in the beginning or end of the string input?
Below was run in Python3.8
>>> value = '"a\""'
>>> value.strip('"')
'a'
>>> value = '""a""'
>>> value.strip('"')
'a'
Doing a slice might be better here:
>>> value = '"a\""'
>>> value[1:-1]
'a"'
>>> value = '""a""'
>>> value[1:-1]
'"a"'
There was a problem hiding this comment.
This only cares about wrapping quotes and not the entire nesting, can change the function name to reflect that.
sriram-mv
force-pushed
the
fix-tags-key-characters
branch
from
99e4f13 to
9b83a54
Compare
samcli/cli/types.py
Outdated
| # Use this regex when you have space as delimiter Ex: "KeyName1=string KeyName2=string" | ||
| VALUE_REGEX_SPACE_DELIM = _value_regex(" ") | ||
| VALUE_REGEX_SPACE_DELIM = _generate_match_regex(match_pattern=".", delim=" ") |
There was a problem hiding this comment.
There are two different values here: one for tags and one for parameter overrides. Can we update these names to make them read better.
Also, should this and VALUE_REGEX_COMMA_DELIM be moved to CfnParameterOverridesType instead of the global scope?
Why is this change necessary? * Tag keys and values need to allow characters as shown on https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/Using_Tags.html How does it address the issue? * Adds appropriate regex with unit tests. What side effects does this change have? * None, tag keys and values are escaped which is good, cloudformation rejects them anyways.
sriram-mv
force-pushed
the
fix-tags-key-characters
branch
from
10540fb to
fc890c3
Compare
Copied from #1727
Why is this change necessary?
Using the
aws cloudformation deploy --tagscommand, you are allowed to use these characters in the tag keys. Documentation also lists these characters as supported and allowed. Withsam deploy --tags, it did not work properly due to a regex limiting supported characters to A-Za-z0-9.How does it address the issue?
By adding a regex specifically for tag keys. By using this new regex,
sam deploy --tags 'owner:name=son of anton'no longer converts the tag key tonameinstead ofowner:name.Before:
After:
What side effects does this change have?
None. More characters allowed in keys and fully backwards compatible.
Checklist:
Write Design Document (Do I need to write a design document?)Write unit testsWrite/update functional testsmake prpassesWrite documentationBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.