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.

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

Enable container_insights for ecs_cluster #9530

Closed
wants to merge 7 commits into from

Conversation

chaspy
Copy link
Contributor

@chaspy chaspy commented Jul 28, 2019

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Relates OR Closes #9294

Release note for CHANGELOG:

resource/aws_ecs_cluster: Add `container_insights` argument 

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSEcsCluster_basic'

...

@ghost ghost added size/XS Managed by automation to categorize the size of a PR. service/ecs Issues and PRs that pertain to the ecs service. documentation Introduces or discusses updates to documentation. labels Jul 28, 2019
@chaspy chaspy force-pushed the chaspy/ecs-container-insights branch from eb9eac3 to fffed99 Compare July 28, 2019 08:56
@ghost ghost added size/S Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. and removed size/XS Managed by automation to categorize the size of a PR. labels Jul 28, 2019
@chaspy chaspy force-pushed the chaspy/ecs-container-insights branch from 5ec0be1 to 92970b6 Compare July 28, 2019 14:04
@chaspy chaspy changed the title Add container_insights to schema Enable container_insights for ecs_cluster Jul 28, 2019
@chaspy chaspy force-pushed the chaspy/ecs-container-insights branch from 92970b6 to 6a43091 Compare July 28, 2019 17:50
@chaspy
Copy link
Contributor Author

chaspy commented Jul 28, 2019

🤔

$ make testacc TESTARGS='-run=TestAccAWSEcsCluster_basic'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSEcsCluster_basic -timeout 120m
# github.com/terraform-providers/terraform-provider-aws
./test.go:11:6: main redeclared in this block
	previous declaration at ./main.go:8:6
=== RUN   TestAccAWSEcsCluster_basic
=== PAUSE TestAccAWSEcsCluster_basic
=== CONT  TestAccAWSEcsCluster_basic
--- FAIL: TestAccAWSEcsCluster_basic (38.32s)
    testing.go:568: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.

        (map[string]string) {
        }


        (map[string]string) (len=1) {
         (string) (len=18) "container_insights": (string) (len=4) "true"
        }

FAIL
FAIL	github.com/terraform-providers/terraform-provider-aws/aws	38.383s
make: *** [testacc] Error 2

@bflad
Copy link
Contributor

bflad commented Sep 4, 2019

Hi @chaspy 👋 Thank you so much for your contribution here and sorry it was never reviewed in its draft state. Since this was still work in progress and a fully finished implementation came along in #9720, we opted to merge that to release this functionality.

Some general notes that may help for future contributions:

testing.go:568: Step 1 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.

This error occurs when an attribute is not being saved correctly in the Terraform state during the Read function. e.g. d.Set("attribute_name", attributeValue)

  • We typically prefer to match the AWS API implementations when implementing the Terraform resource to remove any abstractions or future complications with API changes in the future. This also helps Terraform match the AWS CLI, SDKs, and CloudFormation syntax. With the API using a data structure to represent each setting, we would prefer to match that in Terraform via configuration block syntax. resource/ecs_cluster: Add ability to enable ECS Cluster Insights #9720 shows this type of schema handling with a TypeSet attribute and expand/flatten functions.
  • When enhancing an existing resource, we typically prefer to add a new acceptance test to cover the new functionality and leave the existing testing to cover potential regressions. More information about this can be found in the Contributing Guide.

We appreciate the effort you put in here and hope to see contributions from you in the future. 😄

@bflad bflad closed this Sep 4, 2019
@ghost
Copy link

ghost commented Nov 1, 2019

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 Nov 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
documentation Introduces or discusses updates to documentation. service/ecs Issues and PRs that pertain to the ecs service. size/S 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
Development

Successfully merging this pull request may close these issues.

ECS clusters: enable Container Insights
2 participants