Skip to content
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.

Sign up for GitHub

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

Jump to bottom

ECS Fargate Platform Version Support #6510

Merged
merged 2 commits into from
Dec 20, 2018
Merged

ECS Fargate Platform Version Support #6510

merged 2 commits into from
Dec 20, 2018

Conversation

trevorlauder
Copy link
Contributor

@trevorlauder trevorlauder commented Nov 18, 2018 •
edited
Loading

Changes proposed in this pull request:

  • This adds support for setting the ECS Fargate Platform Version

This is my first time diving into this code, please let me know if I'm missing anything. It works in my testing but the one item I'm not sure how to handle is when it's run against a service that isn't using the AWS default of LATEST, this change will cause terraform runs to trigger a change which people may not be expecting.

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSEcsService_withLaunchTypeFargateAndPlatformVersion'

==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSEcsService_withLaunchTypeFargateAndPlatformVersion -timeout 120m
?       github.com/terraform-providers/terraform-provider-aws   [no test files]
=== RUN   TestAccAWSEcsService_withLaunchTypeFargateAndPlatformVersion
--- PASS: TestAccAWSEcsService_withLaunchTypeFargateAndPlatformVersion (303.77s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       307.168s

@ghost ghost added size/L Managed by automation to categorize the size of a PR. documentation Introduces or discusses updates to documentation. service/ecs Issues and PRs that pertain to the ecs service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Nov 18, 2018
@bflad bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Nov 19, 2018
bflad
bflad reviewed Dec 5, 2018
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @trevorlauder! 👋 Thanks for submitting this and its off to a good start! A few adjustments and hopefully we can get this in.

aws/resource_aws_ecs_service_test.go
tdName := fmt.Sprintf("tf-acc-td-svc-ltf-w-pv-%s", rString)
svcName := fmt.Sprintf("tf-acc-svc-ltf-w-pv-%s", rString)

resource.Test(t, resource.TestCase{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can parallelize this testing with the others via resource.ParallelTest() 😄 (dependent on the changes below to remove the extraneous other resource.Test() calls)

Suggested change
resource.Test(t, resource.TestCase{
resource.ParallelTest(t, resource.TestCase{

aws/resource_aws_ecs_service_test.go
CheckDestroy: testAccCheckAWSEcsServiceDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSEcsServiceWithLaunchTypeFargateAndPlatformVersion(sg1Name, sg2Name, clusterName, tdName, svcName, "false"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally, testAccAWSEcsServiceWithLaunchTypeFargateAndPlatformVersion() should be accepting the platform version as its final parameter instead of assign public IP, which can just be hardcoded to true/false in the test configuration. This will allow us to properly test updating the platform version across multiple TestStep (e.g. 1.0.0 in the first step and 1.1.0 in the second step). 👍

aws/resource_aws_ecs_service_test.go
testAccCheckAWSEcsServiceExists("aws_ecs_service.main", &service),
resource.TestCheckResourceAttr("aws_ecs_service.main", "launch_type", "FARGATE"),
resource.TestCheckResourceAttr("aws_ecs_service.main", "platform_version", "LATEST"),
resource.TestCheckResourceAttr("aws_ecs_service.main", "network_configuration.0.assign_public_ip", "false"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're not concerned too much with the network configuration when adjusting the platform version, these network_configuration checks can be removed in preference of just ensuring we're checking the platform_version. 👍

aws/resource_aws_ecs_service_test.go
},
},
})
resource.Test(t, resource.TestCase{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing this acceptance testing was copied from the Fargate acceptance testing above which was unfortunately using multiple invocations of resource.Test(), a pattern not used anywhere else in the codebase (in preference of a single resource.ParallelTest() with multiple TestStep). 😅 Can you move the TestStep in these two resource.Test() calls to the Steps in the first (soon-to-be) resource.ParallelTest() above? Thanks! I'll submit a pull request to fix up the other Fargate acceptance test separately.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And here's what I mean, fixing the other acceptance test. 😄 https://github.com/terraform-providers/terraform-provider-aws/pull/6732/files

aws/resource_aws_ecs_service_test.go
@@ -1254,6 +1312,97 @@ resource "aws_ecs_service" "main" {
`, sg1Name, sg2Name, clusterName, tdName, svcName, assignPublicIP)
}

func testAccAWSEcsServiceWithLaunchTypeFargateAndPlatformVersion(sg1Name, sg2Name, clusterName, tdName, svcName, assignPublicIP string) string {
return fmt.Sprintf(`
provider "aws" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We prefer to not define the AWS provider configuration in the test configurations unless necessary. The below test configuration should work in other regions beyond us-east-1, so this provider configuration should be removed. 👍

aws/resource_aws_ecs_service_test.go
task_definition = "${aws_ecs_task_definition.mongo.arn}"
desired_count = 1
launch_type = "FARGATE"
platform_version = "LATEST"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned above, we should swap the assign public IP parameter for this one here, e.g.

Suggested change
platform_version = "LATEST"
platform_version = %q

website/docs/r/ecs_service.html.markdown
@@ -80,6 +80,7 @@ The following arguments are supported:
* `task_definition` - (Required) The family and revision (`family:revision`) or full ARN of the task definition that you want to run in your service.
* `desired_count` - (Optional) The number of instances of the task definition to place and keep running. Defaults to 0. Do not specify if using the `DAEMON` scheduling strategy.
* `launch_type` - (Optional) The launch type on which to run your service. The valid values are `EC2` and `FARGATE`. Defaults to `EC2`.
* `platform_version` - (Optional) The platform version on which to run your service. Defaults to `LATEST`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add a link to the AWS documentation and mention this only applies to the launch_type being set to FARGATE? e.g.

Suggested change
* `platform_version` - (Optional) The platform version on which to run your service. Defaults to `LATEST`.
* `platform_version` - (Optional) The platform version on which to run your service. Only applicable for `launch_type` set to `FARGATE`. Defaults to `LATEST`. More information about Fargate platform versions can be found in the [AWS ECS User Guide](https://docs.aws.amazon.com/AmazonECS/latest/developerguide/platform_versions.html).

aws/resource_aws_ecs_service.go
Type: schema.TypeString,
ForceNew: false,
Optional: true,
Default: "LATEST",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Defaulting this to LATEST breaks all the other tests with launch_type set to EC2 (or unset, which defaults to EC2) with the following:

--- FAIL: TestAccAWSEcsService_withARN (4.53s)
    testing.go:538: Step 0 error: Error applying: 1 error occurred:
        	* aws_ecs_service.mongo: 1 error occurred:
        	* aws_ecs_service.mongo: InvalidParameterException: The platform version must be null when specifying an EC2 launch type.

My recommendation would be to adjust this attribute's schema to the following:

"platform_version": {
	Type:     schema.TypeString,
	Optional: true,
	Computed: true,
},

The Computed: true here will address your original concern by allowing Terraform plans to ignore any difference if the argument is not defined in the Terraform configuration.

@bflad bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Dec 5, 2018
@bflad bflad mentioned this pull request Dec 19, 2018
Closed
@bflad bflad added this to the v1.53.0 milestone Dec 20, 2018
bflad
bflad approved these changes Dec 20, 2018
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @trevorlauder 👋 Since its been awhile and we haven't heard anything, I went ahead and implemented the pull request feedback in a followup commit (342f4fb) so we can get this released. Thanks for your contribution! 🚀

resource/aws_ecs_service: Address PR #6510 feedback

Changes:

  • resource/aws_ecs_service: Switch platform_version attribute from Default: "LATEST", which breaks EC2 compatibility, to Computed: true
  • tests/resource/aws_ecs_service: Adjust TestAccAWSEcsService_withLaunchTypeFargateAndPlatformVersion to use resource.ParallelTest() and verify platform_version updates
  • docs/resource/aws_ecs_service: Note platform_version only applies to Fargate and add documentation link

Output from acceptance testing:

--- PASS: TestAccAWSEcsService_basicImport (41.59s)
--- PASS: TestAccAWSEcsService_disappears (19.58s)
--- PASS: TestAccAWSEcsService_healthCheckGracePeriodSeconds (248.66s)
--- PASS: TestAccAWSEcsService_ManagedTags (17.56s)
--- PASS: TestAccAWSEcsService_PropagateTags (113.65s)
--- PASS: TestAccAWSEcsService_Tags (48.66s)
--- PASS: TestAccAWSEcsService_withAlb (233.55s)
--- PASS: TestAccAWSEcsService_withARN (38.08s)
--- PASS: TestAccAWSEcsService_withDaemonSchedulingStrategy (40.56s)
--- PASS: TestAccAWSEcsService_withDaemonSchedulingStrategySetDeploymentMinimum (28.94s)
--- PASS: TestAccAWSEcsService_withDeploymentController_Type_CodeDeploy (240.59s)
--- PASS: TestAccAWSEcsService_withDeploymentMinimumZeroMaximumOneHundred (39.71s)
--- PASS: TestAccAWSEcsService_withDeploymentValues (30.24s)
--- PASS: TestAccAWSEcsService_withEcsClusterName (22.13s)
--- PASS: TestAccAWSEcsService_withFamilyAndRevision (46.16s)
--- PASS: TestAccAWSEcsService_withIamRole (196.48s)
--- PASS: TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration (83.97s)
--- PASS: TestAccAWSEcsService_withLaunchTypeFargate (121.63s)
--- PASS: TestAccAWSEcsService_withLaunchTypeFargateAndPlatformVersion (113.31s)
--- PASS: TestAccAWSEcsService_withLbChanges (267.91s)
--- PASS: TestAccAWSEcsService_withPlacementConstraints (42.49s)
--- PASS: TestAccAWSEcsService_withPlacementConstraints_emptyExpression (39.86s)
--- PASS: TestAccAWSEcsService_withPlacementStrategy (140.80s)
--- PASS: TestAccAWSEcsService_withRenamedCluster (42.10s)
--- PASS: TestAccAWSEcsService_withReplicaSchedulingStrategy (71.50s)
--- PASS: TestAccAWSEcsService_withServiceRegistries (139.21s)
--- PASS: TestAccAWSEcsService_withServiceRegistries_container (138.46s)
--- PASS: TestAccAWSEcsService_withUnnormalizedPlacementStrategy (14.38s)

@bflad bflad merged commit 6b51a94 into hashicorp:master Dec 20, 2018
bflad added a commit that referenced this pull request Dec 20, 2018
@bflad
resource/aws_ecs_service: Address PR #6510 feedback
342f4fb 
Changes:
* resource/aws_ecs_service: Switch `platform_version` attribute from `Default: "LATEST"`, which breaks EC2 compatibility, to `Computed: true`
* tests/resource/aws_ecs_service: Adjust `TestAccAWSEcsService_withLaunchTypeFargateAndPlatformVersion` to use `resource.ParallelTest()` and verify `platform_version` updates
* docs/resource/aws_ecs_service: Note `platform_version` only applies to Fargate and add documentation link

Output from acceptance testing:

```
--- PASS: TestAccAWSEcsService_basicImport (41.59s)
--- PASS: TestAccAWSEcsService_disappears (19.58s)
--- PASS: TestAccAWSEcsService_healthCheckGracePeriodSeconds (248.66s)
--- PASS: TestAccAWSEcsService_ManagedTags (17.56s)
--- PASS: TestAccAWSEcsService_PropagateTags (113.65s)
--- PASS: TestAccAWSEcsService_Tags (48.66s)
--- PASS: TestAccAWSEcsService_withAlb (233.55s)
--- PASS: TestAccAWSEcsService_withARN (38.08s)
--- PASS: TestAccAWSEcsService_withDaemonSchedulingStrategy (40.56s)
--- PASS: TestAccAWSEcsService_withDaemonSchedulingStrategySetDeploymentMinimum (28.94s)
--- PASS: TestAccAWSEcsService_withDeploymentController_Type_CodeDeploy (240.59s)
--- PASS: TestAccAWSEcsService_withDeploymentMinimumZeroMaximumOneHundred (39.71s)
--- PASS: TestAccAWSEcsService_withDeploymentValues (30.24s)
--- PASS: TestAccAWSEcsService_withEcsClusterName (22.13s)
--- PASS: TestAccAWSEcsService_withFamilyAndRevision (46.16s)
--- PASS: TestAccAWSEcsService_withIamRole (196.48s)
--- PASS: TestAccAWSEcsService_withLaunchTypeEC2AndNetworkConfiguration (83.97s)
--- PASS: TestAccAWSEcsService_withLaunchTypeFargate (121.63s)
--- PASS: TestAccAWSEcsService_withLaunchTypeFargateAndPlatformVersion (113.31s)
--- PASS: TestAccAWSEcsService_withLbChanges (267.91s)
--- PASS: TestAccAWSEcsService_withPlacementConstraints (42.49s)
--- PASS: TestAccAWSEcsService_withPlacementConstraints_emptyExpression (39.86s)
--- PASS: TestAccAWSEcsService_withPlacementStrategy (140.80s)
--- PASS: TestAccAWSEcsService_withRenamedCluster (42.10s)
--- PASS: TestAccAWSEcsService_withReplicaSchedulingStrategy (71.50s)
--- PASS: TestAccAWSEcsService_withServiceRegistries (139.21s)
--- PASS: TestAccAWSEcsService_withServiceRegistries_container (138.46s)
--- PASS: TestAccAWSEcsService_withUnnormalizedPlacementStrategy (14.38s)
```
bflad added a commit that referenced this pull request Dec 20, 2018
@bflad
Update CHANGELOG for #6510
0e5e13d
@bflad
Copy link
Contributor

bflad commented Dec 20, 2018

This has been released in version 1.53.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

@ghost
Copy link

ghost commented Apr 1, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Apr 1, 2020
@breathingdust breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bflad bflad bflad approved these changes

Assignees
No one assigned
Labels
documentation Introduces or discusses updates to documentation. enhancement Requests to existing resources that expand the functionality or scope. service/ecs Issues and PRs that pertain to the ecs service. size/L Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Milestone
v1.53.0
Development

Successfully merging this pull request may close these issues.
3 participants
@trevorlauder @bflad @breathingdust