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

provider/aws: Validate aws_ecs_task_definition.container_definitions #12161

Conversation

minamijoyo
Copy link
Contributor

Currently there is no validation in aws_ecs_task_definition.container_definitions,
but I would like to be able to validate it at plan.

The following is an example for invalid definition.
Note that "sleep 360" in command should be array as ["sleep", "360"].
This is easy to make a mistake. I was actually wrong and suffer for a while.

provider "aws" {
  region = "us-east-1"
}

resource "aws_ecs_task_definition" "test" {
  family = "test"

  container_definitions = <<EOF
[
  {
    "name": "sleep",
    "image": "busybox",
    "cpu": 10,
    "command": "sleep 360",
    "memory": 10,
    "essential": true
  }
]
EOF
}

Before change

There is no error at plan. ( Of course, it will be an error when applied. )

$ terraform plan
Refreshing Terraform state in-memory prior to plan...
The refreshed state will be used to calculate this plan, but will not be
persisted to local or remote state storage.

The Terraform execution plan has been generated and is shown below.
Resources are shown in alphabetical order for quick scanning. Green resources
will be created (or destroyed and then created if an existing resource
exists), yellow resources are being changed in-place, and red resources
will be destroyed. Cyan entries are data sources to be read.

Note: You didn't specify an "-out" parameter to save this plan, so when
"apply" is called, Terraform can't guarantee this is what will execute.

+ aws_ecs_task_definition.test
    arn:                   "<computed>"
    container_definitions: "f3458871d51e9a7690213fb82dba565e66458870"
    family:                "test"
    network_mode:          "<computed>"
    revision:              "<computed>"


Plan: 1 to add, 0 to change, 0 to destroy.

After change

We can now detect the error at plan:

$ terraform plan
1 error(s) occurred:

* aws_ecs_task_definition.test: ECS Task Definition container_definitions is invalid: Error decoding JSON: json: cannot unmarshal string into Go struct field ContainerDefinition.Command of type []*string

Furthermore, field names that failed in json.Unmarshal are now included in error messages thanks to recent update of terraform Go version to 1.8.

@minamijoyo
Copy link
Contributor Author

acceptance test passing:

[terraform@validate-ecs-task-container-definitions]$ make testacc TEST=./builtin/providers/aws TESTARGS='-run=TestAccAWSEcsTaskDefinition_'
==> Checking that code complies with gofmt requirements...
go generate $(go list ./... | grep -v /terraform/vendor/)
2017/02/26 22:30:23 Generated command/internal_plugin_list.go
TF_ACC=1 go test ./builtin/providers/aws -v -run=TestAccAWSEcsTaskDefinition_ -timeout 120m
=== RUN   TestAccAWSEcsTaskDefinition_basic
--- PASS: TestAccAWSEcsTaskDefinition_basic (29.78s)
=== RUN   TestAccAWSEcsTaskDefinition_withScratchVolume
--- PASS: TestAccAWSEcsTaskDefinition_withScratchVolume (14.89s)
=== RUN   TestAccAWSEcsTaskDefinition_withEcsService
--- PASS: TestAccAWSEcsTaskDefinition_withEcsService (150.38s)
=== RUN   TestAccAWSEcsTaskDefinition_withTaskRoleArn
--- PASS: TestAccAWSEcsTaskDefinition_withTaskRoleArn (22.90s)
=== RUN   TestAccAWSEcsTaskDefinition_withNetworkMode
--- PASS: TestAccAWSEcsTaskDefinition_withNetworkMode (52.33s)
=== RUN   TestAccAWSEcsTaskDefinition_constraint
--- PASS: TestAccAWSEcsTaskDefinition_constraint (14.05s)
PASS
ok      github.com/hashicorp/terraform/builtin/providers/aws    284.351s

@radeksimko
Copy link
Member

Hi @minamijoyo
thanks for the contribution & explanation & output from acceptance tests.
I believe this will be useful addition for the users.

👍

@radeksimko radeksimko merged commit 4b9ec9c into hashicorp:master Apr 1, 2017
@minamijoyo
Copy link
Contributor Author

@radeksimko Thanks !!!

@ghost
Copy link

ghost commented Apr 14, 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 have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants