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

Convert fake AWSHelperFns into AWSProperties #478

Merged
merged 1 commit into from
Jun 4, 2016

Conversation

jonapich
Copy link
Contributor

This updates 3 objects that were incorrectly subclassing AWSHelperFn. They are now subclassing AWSProperty as intended. This should be backward compatible.

@jonapich
Copy link
Contributor Author

The remaining AwsHelperFn objects that don't represent a real AWS Intrinsinc Function are very problematic and I haven't found a way to convert them in a backward-compatible manner.

I got really close with the "Tags" object provided in troposphere's init. It cannot be translated into an AWSProperty because it is a list of tags, making the props = {} impossible to write. I wrote a new AWSProperty called "Tag" with a proper 'props' object (Key/Value as required basestrings), and then changing Tags for a subclass of list that internally creates Tag objects and acts as a list of such. Then I changed a lot of the 'Tags: (list, False)' into 'Tags: ([Tag], False)' and got clean test runs, but this introduces a breaking change if anyone actually passed a list of {Key: k, Value:v} dictionaries because the inner types are now checked. Before, it only made sure it was a list.

The other major problem will be the Metadata elements from the cloudformation module. I never worked with them but I believe they don't have a set structure like typical properties. For those, maybe it should be better to just wrap them into a new no-validation object that would be separate from AWSHelperFn, instead of trying to wrap them into actual AWSProperty objects...?

@jonapich
Copy link
Contributor Author

The Tags object's breaking change could potentially be non-breaking by implementing multiple-type checks?

props = {'Tags': (([(Tag, dict)]), False)

@markpeek
Copy link
Member

markpeek commented Jun 3, 2016

@jonapich regardless the issue with Tags, is the rest of this PR good to go?

@jonapich
Copy link
Contributor Author

jonapich commented Jun 4, 2016

@markpeek yep!

@markpeek markpeek merged commit 54d0376 into cloudtools:master Jun 4, 2016
@markpeek
Copy link
Member

markpeek commented Jun 4, 2016

Thanks!

amosshapira pushed a commit to amosshapira/troposphere that referenced this pull request Oct 24, 2016
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.

2 participants