-
Notifications
You must be signed in to change notification settings - Fork 167
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
Troposphere types #257
Troposphere types #257
Conversation
@@ -1,3 +1,7 @@ | |||
## TBD | |||
|
|||
- Add support for `TroposphereType` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd just move this into the 1.0.0a1 section - I plan to just update that to be the 1.0.0 release when we're ready to go.
# Specify that Buckets will be a list of s3.Bucket types. | ||
This means the config should take a list of dictionaries | ||
which will be converted into troposphere buckets. | ||
"type": TroposphereType([s3.Bucket]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this, and a single type, the only applicable syntax for TroposphereType
? If so, probably worth documenting both.
Args: | ||
defined_type (Union[list, type]): List of or single Troposphere | ||
type | ||
resource_name (str): The name to use for the resource when creating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than this, what about making it so that every resource should have a title
attribute that is required? That's the name of the variable that is used in troposphere. Another option is that we could do away with both the list & plain type syntax, and instead replace it with a single format that uses dictionaries like I did with the SecurityGroup blueprint: remind101/stacker_blueprints@ab094c3#diff-e1bf8fc9a0d5fa2e2b507057d3fcd3aaR29
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the way I did it in the security group blueprint actually makes a lot of sense. You have to give unique names to all resources in a blueprint, so you might as well make it a requirement in the config. Going to hold off on further review till you reply to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good call
When passing values to TroposphereTypes you now must include the resource name that will be used when adding the resource to the CloudFormation template.
@phobologic updated based on your feedback |
variables: | ||
Buckets: | ||
# resource name that will be added to CloudFormation | ||
FirstBucket: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, at this point, my only concern is the fact we call it a list
type, but when you define it in the config its actually a dict. Not sure if this is a big deal or not - dict makes more sense from the config perspective, but maybe making it a list of single item dicts would bridge the gap:
Buckets:
- FirstBucket:
BucketName: my-first-bucket
- SecondBucket:
BucketName: my-second-bucket
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, the other option is to step away from the python types altogether and instead go with our own syntax. TroposphereType
ManyTroposphereType
or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We talked about this on slack, but we decided we should do TroposphereType(<type>, many=True)
We now use `TroposphereType(s3.Bucket, many=True)` to indicate that a type will create more that one tropopshere type. Since the value provided in the config is always a dictionary, this is easier to understand than the previous `TroposphereType([s3.Bucket])` syntax, since you're not passing in a list.
@phobologic i think this is good to go |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - only one potential enhancement I'd like to see. Thanks!
output = [new_type.from_dict(title, values) for title, values in | ||
if not self._many and len(value) > 1: | ||
message = ( | ||
"Only one resource can be provided for single " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there anyway we can make this error more helpful in determining where the issue is? Maybe we need to catch this higher and point out what caused this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NM, just realized this is caught and re-raised as a ValidatorError, which will point out where the issue stems from. Awesome, going to go ahead and merge this.
Troposphere types
Adds a new custom variable type:
TroposphereType
. This builds off @phobologic's improvements tofrom_dict
within Troposphere.TroposphereType
lets you pass through the yaml from a config directly to troposphere types which saves you the boilerplate of specifying variables within the blueprint or instantiating the nested types yourself.Open to suggestions about naming or other syntax.
An example:
config file:
stacker_blueprints.s3.Buckets: