-
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
Support task placement via ecs-params #586
Changes from all commits
f9e3100
309fbee
13768d7
ff2b846
6f73765
2f0e76b
2f57301
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -607,6 +607,54 @@ func TestRunTask_WithTaskNetworking(t *testing.T) { | |
assert.NoError(t, err, "Unexpected error when calling RunTask") | ||
} | ||
|
||
func TestRunTask_WithTaskPlacement(t *testing.T) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just something to think about (not a blocker for merging): Now that you've refactored the ECS Client's RunTask to just take the RunTaskInput and then call the SDK's RunTask, do we really need tests like this? This test just passes in the 2 new things through the RunTaskInput object and then verifies that the client was called with the same inputs. One might say that it never hurts to have more tests... but I disagree because the more tests you have, the more code you have to change during future refactorings/code changes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I definitely agree that tests should be treated as first-class citizens for purposes of code maintenance; in this case however, given our mocking strategy I think this is still a useful test to ensure that we are passing the right inputs through to our ECS Client (and that certain fields are not passed through in certain scenarios, to ensure we get the expected defaults that ECS provides. |
||
mockEcs, _, client, ctrl := setupTestController(t, getDefaultCLIConfigParams(t)) | ||
defer ctrl.Finish() | ||
|
||
td := "taskDef" | ||
group := "taskGroup" | ||
count := 5 | ||
|
||
placementConstraints := []*ecs.PlacementConstraint{ | ||
{ | ||
Type: aws.String("distinctInstance"), | ||
}, { | ||
Expression: aws.String("attribute:ecs.instance-type =~ t2.*"), | ||
Type: aws.String("memberOf"), | ||
}, | ||
} | ||
placementStrategy := []*ecs.PlacementStrategy{ | ||
{ | ||
Type: aws.String("random"), | ||
}, { | ||
Field: aws.String("instanceId"), | ||
Type: aws.String("binpack"), | ||
}, | ||
} | ||
|
||
mockEcs.EXPECT().RunTask(gomock.Any()).Do(func(input interface{}) { | ||
req := input.(*ecs.RunTaskInput) | ||
assert.Equal(t, clusterName, aws.StringValue(req.Cluster), "Expected clusterName to match") | ||
assert.Equal(t, td, aws.StringValue(req.TaskDefinition), "Expected taskDefinition to match") | ||
assert.Equal(t, group, aws.StringValue(req.Group), "Expected group to match") | ||
assert.Equal(t, int64(count), aws.Int64Value(req.Count), "Expected count to match") | ||
assert.Equal(t, placementConstraints, req.PlacementConstraints, "Expected placement constraints to match") | ||
assert.Equal(t, placementStrategy, req.PlacementStrategy, "Expected placement strategy to match") | ||
}).Return(&ecs.RunTaskOutput{}, nil) | ||
|
||
runTaskInput := &ecs.RunTaskInput{ | ||
Cluster: aws.String(clusterName), | ||
TaskDefinition: aws.String(td), | ||
Group: aws.String(group), | ||
Count: aws.Int64(int64(count)), | ||
LaunchType: aws.String("EC2"), | ||
PlacementConstraints: placementConstraints, | ||
PlacementStrategy: placementStrategy, | ||
} | ||
_, err := client.RunTask(runTaskInput) | ||
assert.NoError(t, err, "Unexpected error when calling RunTask") | ||
} | ||
|
||
func TestIsActiveCluster(t *testing.T) { | ||
mockEcs, _, client, ctrl := setupTestController(t, getDefaultCLIConfigParams(t)) | ||
defer ctrl.Finish() | ||
|
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'm curious about this- my reading of
ValidateFargateParams()
doesn't indicate it matters if its called before or afterGetOrCreateTaskDefinition()
- what am I missing?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.
So there is nothing wrong with the
ValidateFargateParams()
method itself, but in the places it's being called from the top-level compose commands (in this case,Create()
, we are callingentity.GetOrCreateTaskDefinitions(s)
first (see the comment inentity_helper.go
from the same commit). In other words, due to the order-dependent nature of actual calls to ECS and the fact that we mock these in our unit tests, this Validation is not useful for validating the actual request, in this case, to CreateService, because bad parameters on the TaskDefinition itself will cause the server to return a validation error. E.g.:ECS returns a ClientException on
RegisterTaskDefinition
:I discovered this case when trying to test Fargate validation (since placement is not supported with Fargate launch type).