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.

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

Fix schema for resource tags. #8314

Merged
merged 1 commit into from Oct 2, 2020
Merged

Fix schema for resource tags. #8314

merged 1 commit into from Oct 2, 2020

Conversation

dashmug
Copy link
Contributor

@dashmug dashmug commented Sep 30, 2020

AWS allows for resource tags to have colons in them. In fact, they use it themselves for their reserved tags and they recommend their use in their best practices. Serverless config schema validation should also allow for it.

The schema validation is based on the documentation in this page: https://docs.aws.amazon.com/AWSCloudFormation/latest/APIReference/API_Tag.html.

This PR closes #8304.

@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2020

Codecov Report

Merging #8314 into master will decrease coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8314      +/-   ##
==========================================
- Coverage   88.08%   88.01%   -0.07%     
==========================================
  Files         250      249       -1     
  Lines        9368     9307      -61     
==========================================
- Hits         8252     8192      -60     
+ Misses       1116     1115       -1     
Impacted Files Coverage Δ
lib/plugins/aws/provider/awsProvider.js 93.04% <ø> (-0.18%) ⬇️
lib/classes/ConfigSchemaHandler/index.js 89.10% <0.00%> (-0.42%) ⬇️
...ib/plugins/aws/package/lib/generateCoreTemplate.js 95.12% <0.00%> (-0.34%) ⬇️
lib/plugins/aws/package/compile/layers/index.js 95.45% <0.00%> (-0.15%) ⬇️
lib/configSchema.js 100.00% <0.00%> (ø)
lib/plugins/aws/package/lib/mergeIamTemplates.js 100.00% <0.00%> (ø)
...ib/plugins/aws/package/compile/events/alb/index.js 100.00% <0.00%> (ø)
...ib/plugins/aws/package/compile/events/sns/index.js 100.00% <0.00%> (ø)
...lugins/aws/package/compile/events/httpApi/index.js 92.59% <0.00%> (ø)
... and 3 more

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 dd9a011...d205a63. Read the comment docs.

@dashmug dashmug changed the title Allow for colons in tags. Fix schema for resource tags. Sep 30, 2020
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Thank you @dashmug, it looks great! Still I think I've spotted one issue in pattern, let me know

@@ -356,9 +356,8 @@ class AwsProvider {
awsResourceTags: {
type: 'object',
patternProperties: {
'^(?!aws:)[\\w./=+-]{1,128}$': {
'^(?!aws:)[\\w./=+-:]{1,128}$': {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to move - so it's listed last, otherwise +-: implies a range

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @medikoo. I've updated it.

@dashmug dashmug requested a review from medikoo October 2, 2020 01:23
Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Thank you @dashmug !

@medikoo medikoo merged commit fc34140 into serverless:master Oct 2, 2020
@dashmug dashmug deleted the 8304-fix-tags-schema branch October 20, 2020 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect schema for AWS tags
3 participants