-
Notifications
You must be signed in to change notification settings - Fork 301
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
Add launchtype check before setting platform version #1041
Conversation
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.
Looks good -- can we include manual test out put plz?
log.Warn("Learn more: https://aws.amazon.com/blogs/containers/aws-fargate-launches-platform-version-1-4/") | ||
platformVersion = aws.String("1.4.0") | ||
var platformVersion *string | ||
if launchType == "FARGATE" { |
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.
can we add a unit test (perhaps add assertions on existing EC2 service tests) which ensure that platform version is not set when launchType != FARGATE?
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 it might be nice to have a separate unit test, like TestEFSWithEC2. We can do this by setting the launchtype in the CommandConfig within the createServiceTest helper:
commandConfig *config.CommandConfig, |
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.
Eg:
func TestCreateWithEFS_EC2(t *testing.T) {
flagSet := flag.NewFlagSet("ecs-cli-up", 0)
createServiceTest(
t,
flagSet,
&config.CommandConfig{LaunchType: config.LaunchTypeEC2},
ecsParamsWithFargateEFSVolume(),
func(input *ecs.CreateServiceInput) {
launchType := input.LaunchType
assert.Equal(t, config.LaunchTypeEC2, aws.StringValue(launchType), "launch type is not ec2")
platformVersion := input.PlatformVersion
assert.Nil(t, platformVersion)
},
ecsSettingDisabled,
)
}
manual output for launching ec2 service without EFS via version: '3'
services:
web:
image: acct.dkr.ecr.us-west-2.amazonaws.com/otter:latest
ports:
- "80:80"
logging:
driver: awslogs
options:
awslogs-group: otter
awslogs-region: us-west-2
awslogs-stream-prefix: web ecs-params.yml task_definition:
task_execution_role: ecsTaskExecutionRole
ecs_network_mode: awsvpc
task_size:
mem_limit: 1.0GB
cpu_limit: 512
run_params:
network_configuration:
awsvpc_configuration:
subnets:
- "subnet-1234"
- "subnet-5678"
security_groups:
- "sg-1234" ./ecs-cli compose service up --aws-profile system-test --cluster-config -ec2-tutorial-config --launch-type EC2
INFO[0000] Using ECS task definition TaskDefinition="oaas:20"
INFO[0000] Updated the ECS service with a new task definition. Old containers will be stopped automatically, and replaced with new ones desiredCount=1 force-deployment=false service=oaas
INFO[0010] (service oaas) stopped 1 pending tasks. timestamp="2020-05-27 18:48:41 +0000 UTC"
INFO[0010] (service oaas) has started 1 tasks: (task c596d2ac-d6ae-4db1-897a-f754d9abd6e2). timestamp="2020-05-27 18:48:41 +0000 UTC"
INFO[0071] Service status desiredCount=1 runningCount=1 serviceName=oaas
INFO[0071] ECS Service has reached a stable state desiredCount=1 runningCount=1 serviceName=oaas |
manual test of fargate service with efs volume version: 1
task_definition:
task_execution_role: ecsTaskExecutionRole
ecs_network_mode: awsvpc
task_size:
mem_limit: 1.0GB
cpu_limit: 512
efs_volumes:
- name: "myEFSVolume"
filesystem_id: "fs-fedc8554"
root_directory: /
transit_encryption: DISABLED
iam: DISABLED
run_params:
network_configuration:
awsvpc_configuration:
subnets:
- "subnet-1234"
- "subnet-5678"
security_groups:
- "sg-1234"
assign_public_ip: "ENABLED" docker-compose.yml
➜ oaas git:(ecs-cli) ✗ ./ecs-cli compose service up --aws-profile system-test --cluster-config -fargate-tutorial-config --launch-type FARGATE
INFO[0000] Using ECS task definition TaskDefinition="oaas:11"
INFO[0000] Updated the ECS service with a new task definition. Old containers will be stopped automatically, and replaced with new ones desiredCount=1 force-deployment=false service=oaas
INFO[0000] Service status desiredCount=1 runningCount=1 serviceName=oaas
INFO[0020] (service oaas) has started 1 tasks: (task f6bf3318-fc65-4782-a099-144d69ca200d). timestamp="2020-05-27 19:19:42 +0000 UTC"
INFO[0066] Service status desiredCount=1 runningCount=2 serviceName=oaas
INFO[0137] Service status desiredCount=1 runningCount=1 serviceName=oaas
INFO[0137] (service oaas) has stopped 1 running tasks: (task fcc33890-1157-4b81-8b52-72d238be622d). timestamp="2020-05-27 19:21:39 +0000 UTC"
INFO[0147] ECS Service has reached a stable state desiredCount=1 runningCount=1 serviceName=oaas |
integ test run:
|
@@ -256,7 +301,7 @@ func TestCreateWithTaskPlacement(t *testing.T) { | |||
Type: aws.String("binpack"), | |||
}, | |||
} | |||
|
|||
assert.Nil(t, input.LaunchType, "launch type should be nil") |
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.
do we want to check for launch type and PV?
Checks the launchtype before setting platform version. This reverts a breaking change in compose service up introduced in 1.19.0.
Addresses issue #1040
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.