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

aws_batch_job_definition does not respect lifecycle ignore_changes #11038

Closed
mmshin opened this issue Nov 27, 2019 · 3 comments · Fixed by #11488
Closed

aws_batch_job_definition does not respect lifecycle ignore_changes #11038

mmshin opened this issue Nov 27, 2019 · 3 comments · Fixed by #11488
Labels
bug Addresses a defect in current functionality. service/batch Issues and PRs that pertain to the batch service.
Milestone
v2.48.0

Comments

@mmshin
Copy link

mmshin commented Nov 27, 2019

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Terraform Version

Terraform v0.12.16

  • provider.aws v2.40.0

Affected Resource(s)

  • aws_batch_job_definition

Terraform Configuration Files

   # for the jsonencode magic
   resource "aws_batch_job_definition" "batch" {
     count                = var.create ? 1 : 0
     name                 = var.name
     type                 = "container"
     container_properties = replace(replace("${jsonencode(local.container_properties)}", "/\"(true|false|[[:digit:]]+)\"/", "$1"), "string:", "")

     lifecycle {
       ignore_changes = [container_properties]
     }
   }

Expected Behavior

No infrastructure changes

Actual Behavior

  + resource "aws_batch_job_definition" "batch" {
      + arn                  = (known after apply)
      + container_properties = jsonencode(
            {
              + image      = ""
              + jobRoleArn = "arn:aws:iam::123456789:role/dev-batchContainerRole"
              + memory     = 512
              + vcpus      = 2
            }
        )
      + id                   = (known after apply)
      + name                 = "dev"
      + revision             = (known after apply)
      + type                 = "container"
    }

References
@ghost ghost added the service/batch Issues and PRs that pertain to the batch service. label Nov 27, 2019
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Nov 27, 2019
@bflad bflad added bug Addresses a defect in current functionality. and removed needs-triage Waiting for first response or review from a maintainer. labels Jan 6, 2020
bflad added a commit that referenced this issue Jan 6, 2020
@bflad
resource/aws_batch_job_definition: Properly set container_properties …
ccd38f2 
…into Terraform state and perform drift detection

Reference: #9954
Reference: #11038

Previously, we were silently ignoring the `d.Set()` error for the `container_properties` attribute. This error can be seen with adding error checking on the call or with `tfproviderlint -R004`:

```
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_batch_job_definition.go:146:32: R004: ResourceData.Set() incompatible value type: *github.com/aws/aws-sdk-go/service/batch.ContainerProperties
```

Here we introduce the conversion the Batch `ContainerProperties` object into a JSON string, similar to the handling of ECS `ContainerDefinitions`. With the new attribute properly setting into the Terraform state, existing tests were failing with differences now being discovered:

```
--- FAIL: TestAccAWSBatchJobDefinition_basic (12.38s)
    testing.go:640: Step 0 error: After applying this step, the plan was not empty:

        DIFF:

        DESTROY/CREATE: aws_batch_job_definition.test
          ... omitted for clarity ...
          container_properties:               "{\"command\":[\"ls\",\"-la\"],\"environment\":[{\"name\":\"VARNAME\",\"value\":\"VARVAL\"}],\"image\":\"busybox\",\"memory\":512,\"mountPoints\":[{\"containerPath\":\"/tmp\",\"readOnly\":false,\"sourceVolume\":\"tmp\"}],\"resourceRequirements\":[],\"ulimits\":[{\"hardLimit\":1024,\"name\":\"nofile\",\"softLimit\":1024}],\"vcpus\":1,\"volumes\":[{\"host\":{\"sourcePath\":\"/tmp\"},\"name\":\"tmp\"}]}" => "{\"command\":[\"ls\",\"-la\"],\"environment\":[{\"name\":\"VARNAME\",\"value\":\"VARVAL\"}],\"image\":\"busybox\",\"memory\":512,\"mountPoints\":[{\"containerPath\":\"/tmp\",\"readOnly\":false,\"sourceVolume\":\"tmp\"}],\"ulimits\":[{\"hardLimit\":1024,\"name\":\"nofile\",\"softLimit\":1024}],\"vcpus\":1,\"volumes\":[{\"host\":{\"sourcePath\":\"/tmp\"},\"name\":\"tmp\"}]}" (forces new resource)
          ... omitted for clarity ...
```

Similar to some fields in ECS `ContainerDefinitions`, the API will inject an empty JSON array at least in the `RequiredResources` field. Instead of burdening operators with exactly matching the canonical API JSON, we insert difference suppression for this case. We also suppress reordered `Environment` objects (by `Name`; similar to ECS), since it is likely feature request as well.

Output from acceptance testing:

```
--- PASS: TestAccAWSBatchJobDefinition_basic (14.45s)
--- PASS: TestAccAWSBatchJobDefinition_updateForcesNewResource (24.61s)
```
@bflad bflad mentioned this issue Jan 6, 2020
Merged
bflad added a commit that referenced this issue Feb 5, 2020
@bflad
resource/aws_batch_job_definition: Properly set container_properties …
0a8efb1 
…and name into Terraform state and fix basic test (#11488)

* tests/resource/aws_batch_job_definition: Fix TestAccAWSBatchJobDefinition_basic ContainerProperties comparison to match updated API response

Previously:

```
--- FAIL: TestAccAWSBatchJobDefinition_basic (5.36s)
    testing.go:640: Step 0 error: Check failed: Check 2/2 error: Bad Job Definition Container Properties
        	 expected: {
          Command: ["ls","-la"],
          Environment: [{
              Name: "VARNAME",
              Value: "VARVAL"
            }],
          Image: "busybox",
          Memory: 512,
          MountPoints: [{
              ContainerPath: "/tmp",
              ReadOnly: false,
              SourceVolume: "tmp"
            }],
          Ulimits: [{
              HardLimit: 1024,
              Name: "nofile",
              SoftLimit: 1024
            }],
          Vcpus: 1,
          Volumes: [{
              Host: {
                SourcePath: "/tmp"
              },
              Name: "tmp"
            }]
        }
        	got: {
          Command: ["ls","-la"],
          Environment: [{
              Name: "VARNAME",
              Value: "VARVAL"
            }],
          Image: "busybox",
          Memory: 512,
          MountPoints: [{
              ContainerPath: "/tmp",
              ReadOnly: false,
              SourceVolume: "tmp"
            }],
          ResourceRequirements: [],
          Ulimits: [{
              HardLimit: 1024,
              Name: "nofile",
              SoftLimit: 1024
            }],
          Vcpus: 1,
          Volumes: [{
              Host: {
                SourcePath: "/tmp"
              },
              Name: "tmp"
            }]
        }
```

Output from acceptance testing:

```
--- PASS: TestAccAWSBatchJobDefinition_basic (15.23s)
--- PASS: TestAccAWSBatchJobDefinition_updateForcesNewResource (26.75s)
```

* resource/aws_batch_job_definition: Properly set container_properties into Terraform state and perform drift detection

Reference: #9954
Reference: #11038

Previously, we were silently ignoring the `d.Set()` error for the `container_properties` attribute. This error can be seen with adding error checking on the call or with `tfproviderlint -R004`:

```
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_batch_job_definition.go:146:32: R004: ResourceData.Set() incompatible value type: *github.com/aws/aws-sdk-go/service/batch.ContainerProperties
```

Here we introduce the conversion the Batch `ContainerProperties` object into a JSON string, similar to the handling of ECS `ContainerDefinitions`. With the new attribute properly setting into the Terraform state, existing tests were failing with differences now being discovered:

```
--- FAIL: TestAccAWSBatchJobDefinition_basic (12.38s)
    testing.go:640: Step 0 error: After applying this step, the plan was not empty:

        DIFF:

        DESTROY/CREATE: aws_batch_job_definition.test
          ... omitted for clarity ...
          container_properties:               "{\"command\":[\"ls\",\"-la\"],\"environment\":[{\"name\":\"VARNAME\",\"value\":\"VARVAL\"}],\"image\":\"busybox\",\"memory\":512,\"mountPoints\":[{\"containerPath\":\"/tmp\",\"readOnly\":false,\"sourceVolume\":\"tmp\"}],\"resourceRequirements\":[],\"ulimits\":[{\"hardLimit\":1024,\"name\":\"nofile\",\"softLimit\":1024}],\"vcpus\":1,\"volumes\":[{\"host\":{\"sourcePath\":\"/tmp\"},\"name\":\"tmp\"}]}" => "{\"command\":[\"ls\",\"-la\"],\"environment\":[{\"name\":\"VARNAME\",\"value\":\"VARVAL\"}],\"image\":\"busybox\",\"memory\":512,\"mountPoints\":[{\"containerPath\":\"/tmp\",\"readOnly\":false,\"sourceVolume\":\"tmp\"}],\"ulimits\":[{\"hardLimit\":1024,\"name\":\"nofile\",\"softLimit\":1024}],\"vcpus\":1,\"volumes\":[{\"host\":{\"sourcePath\":\"/tmp\"},\"name\":\"tmp\"}]}" (forces new resource)
          ... omitted for clarity ...
```

Similar to some fields in ECS `ContainerDefinitions`, the API will inject an empty JSON array at least in the `RequiredResources` field. Instead of burdening operators with exactly matching the canonical API JSON, we insert difference suppression for this case. We also suppress reordered `Environment` objects (by `Name`; similar to ECS), since it is likely feature request as well.

Output from acceptance testing:

```
--- PASS: TestAccAWSBatchJobDefinition_basic (14.45s)
--- PASS: TestAccAWSBatchJobDefinition_updateForcesNewResource (24.61s)
```

* resource/aws_batch_job_definition: Properly read name into Terraform state

Reference: #11407 (review)

To prevent future issues with resource import, when implemented.

Output from acceptance testing:

```
--- PASS: TestAccAWSBatchJobDefinition_basic (14.00s)
--- PASS: TestAccAWSBatchJobDefinition_updateForcesNewResource (27.64s)
```
@bflad bflad added this to the v2.48.0 milestone Feb 5, 2020
@bflad
Copy link
Contributor

bflad commented Feb 5, 2020

A potential fix for this has been merged and will release with version 2.48.0 of the Terraform AWS Provider, Thursday this week.

@ghost
Copy link

ghost commented Feb 7, 2020

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

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Mar 27, 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 Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug Addresses a defect in current functionality. service/batch Issues and PRs that pertain to the batch service.
Projects
None yet
Milestone
v2.48.0
2 participants
@bflad @mmshin