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 mappings #1041

Merged
merged 2 commits into from
Oct 11, 2019
Merged

Fix mappings #1041

merged 2 commits into from
Oct 11, 2019

Conversation

lrascao
Copy link
Contributor

@lrascao lrascao commented May 9, 2018

Multiple template.add_mapping calls were squashing the previous key, this patch fixes that

@phobologic
Copy link
Member

Hey @lrascao thanks for the PR, the code looks good - I'm a little worried that this is a change in behavior which might be used by other users of troposphere (the code for add_mapping is quite old, so there's a decent chance that someone is doing so). Also the term add_mapping might not necessarily mean update existing mapping at first. @markpeek any thoughts?

@lrascao
Copy link
Contributor Author

lrascao commented May 29, 2018

I made the tests check the json output, if you try and revert the fix you'll see that the single_mapping test continues to pass and the multiple mappings one begins to fail. This implies that no previous expected behaviour should change with this patch.

@lrascao
Copy link
Contributor Author

lrascao commented Aug 27, 2018

ping?

@lrascao
Copy link
Contributor Author

lrascao commented Oct 11, 2019

bump

Copy link
Member

@phobologic phobologic left a comment

Choose a reason for hiding this comment

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

LGTM, sorry for the radio silence, I missed your last ping!

@markpeek markpeek merged commit e82644f into cloudtools:master Oct 11, 2019
@markpeek
Copy link
Member

Thanks! Sorry for the delay.

davemasino pushed a commit to davemasino/troposphere that referenced this pull request Oct 17, 2019
* Fix multiple mappings being overwritten
* Add test coverage on multiple template mappings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants