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

chore(addons): merge cfn templates Mappings section #1009

Merged
merged 2 commits into from
Jun 10, 2020
Merged

chore(addons): merge cfn templates Mappings section #1009

merged 2 commits into from
Jun 10, 2020

Conversation

efekarakus
Copy link
Contributor

@efekarakus efekarakus commented Jun 9, 2020 •
edited

Enables merging the Mappings section of cfn templates under the "addons/" directory.

Related #994

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

@efekarakus efekarakus added the area/addon Issues about addons. label Jun 9, 2020
@efekarakus efekarakus requested review from kohidave, bvtujo, iamhopaul123 and a team June 9, 2020 23:59
iamhopaul123
iamhopaul123 approved these changes Jun 10, 2020
View reviewed changes
Copy link
Contributor

@iamhopaul123 iamhopaul123 left a comment

Choose a reason for hiding this comment

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

:shipit:

bvtujo
bvtujo reviewed Jun 10, 2020
View reviewed changes
internal/pkg/addons/testdata/merge/invalid-mappings.yaml
@@ -0,0 +1,8 @@
Mappings:
MyTableDynamoDBSettings:
test: # The order is different inside "test" so it should error out.
Copy link
Contributor

Choose a reason for hiding this comment

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

why should the order matter? just in the way yaml is marshaled?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I think I see. It's the way that nodes are marshaled into an array above (how you have to do this dance of checking the ith index, then the i+1th index for the value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think we can be smarter in the isEqual fn and not have the order matter if the node is a "!!map" but I kept it dumb for a first iteration.

bvtujo
bvtujo approved these changes Jun 10, 2020
View reviewed changes
Copy link
Contributor

@bvtujo bvtujo left a comment •
edited

Choose a reason for hiding this comment

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

clever! looks good to me.

ed: What happens if there is more than two levels of mapping? forbidden by CF? Or recursive?

@mergify mergify bot merged commit 5b3f6d9 into aws:master Jun 10, 2020
Sprint 🏃‍♀️ automation moved this from In review to Pending release Jun 10, 2020
@efekarakus
Copy link
Contributor Author

@bvtujo

ed: What happens if there is more than two levels of mapping? forbidden by CF? Or recursive?

Not possible AFAIK https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/mappings-section-structure.html
At least when I try it i get the cryptic err:

An error occurred (ValidationError) when calling the ValidateTemplate operation: Template format error: Every Mappings attribute must be a String or a List.

@efekarakus efekarakus deleted the addons/mappings branch June 10, 2020 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bvtujo bvtujo bvtujo approved these changes

@iamhopaul123 iamhopaul123 iamhopaul123 approved these changes

@kohidave kohidave Awaiting requested review from kohidave

Assignees
No one assigned
Labels
area/addon Issues about addons.
Projects
Sprint 🏃‍♀️
  
Pending release
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@efekarakus @bvtujo @iamhopaul123