Added ECS PlacementConstraints, PlacementStrategy, and ServiceName #706
Conversation
added support for placement constraint
ktruckenmiller
changed the title
troposphere/ecs.py
Outdated
|
|
||
| class PlacementStrategy(AWSProperty): | ||
| props = { | ||
| 'Type': (basestring, True), |
There was a problem hiding this comment.
Looks like we both added this, so I closed mine (#707). You could add validation on the Type field here that it's one of the allowed random, spread, binpack.
There was a problem hiding this comment.
I was adding that, but then I noticed that they had no other validators of this type. I could definitely do this, I wanted to see what cloudtools suggests
There was a problem hiding this comment.
Yeah, I think there's a mix of validation when it's simple, like in emr, and just a basestring in others, so there's not really 100% consistency. IMHO, it's always nice to have early validation, but I can also see the argument that it's more future-proof to let cloudformation be the validator.
There was a problem hiding this comment.
Quite a bit of the validation was put in later in the project and there has never been an effort to go back to add it. If there is an easy way to constrain the value it is always good to add it now. This can be done like the emr examples of using a function instead of basestring, or easier by adding a validate() method to the object. The former will validate at object creation time and the latter at template creation time. I'm fine with either since that is better than having stacks rollback with illegal options.
| @@ -35,6 +65,9 @@ class Service(AWSObject): | |||
| 'DesiredCount': (positive_integer, False), | |||
| 'LoadBalancers': ([LoadBalancer], False), | |||
| 'Role': (basestring, False), | |||
| 'PlacementConstraints': ([PlacementConstraint], False), | |||
There was a problem hiding this comment.
I think the convention is for these props to be alphabetical. The Placement... ones should probably be moved above Role.
|
Thanks! |
Added placement strategies and placement constraints for the ecs services.
We could also add this to task definitions.
Let us know if you'd like to do that and we can add it