-
Notifications
You must be signed in to change notification settings - Fork 302
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
Taskdef constraints #913
Taskdef constraints #913
Conversation
CC @vadimi |
3c1eab3
to
9d3ea42
Compare
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
@@ -298,6 +309,9 @@ run_params: | |||
assert.Len(t, constraints, 2) | |||
assert.ElementsMatch(t, expectedStrategies, strategies) | |||
assert.ElementsMatch(t, expectedConstraints, constraints) | |||
|
|||
taskDefConstraints := ecsParams.TaskDefinition.PlacementConstraints | |||
assert.ElementsMatch(t, expectedTaskDefConstraints, taskDefConstraints) |
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.
Would it make sense to add an error case for wrong type of constraint/nil checking?
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.
Right now we're passing all given values through to ECS and letting it validate, to prevent needing to change logic if/when new constraint types are allowed. To show nils are passed in & handled correctly by ECS though, I'll add some test output to the PR.
@@ -490,6 +490,9 @@ task_definition: | |||
string: string | |||
labels: | |||
string: string | |||
placement_constraints: | |||
- type: string // Valid values: "memberOf" |
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.
Observation: I'm tempted to say if this is always "memberOf" to not include it at all but since we're reusing the Constraint struct, plus maybe we have to account for future constraint types, this should be fine.
test: nil constraint (ECS returns client error)ecs-params.yml
cmd output:
test: nil expression (ECS returns client error)ecs-params.yml
cmd output:
|
9d3ea42
to
c32fdfa
Compare
Fixes issue described in this comment.
test output
docker-compose.yml
ecs-params.yml
cmd output:
resulting task definition with constraints:
Enter
[N/A]
in the box, if an item is not applicable to your change.Testing
Documentation
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.