resource/aws_batch_job_definition: Properly set container_properties and name into Terraform state and fix basic test

[bflad]

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" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Reference: #9954
Closes #11038

Release note for CHANGELOG:

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

Regarding the previous _basic test failure of:

--- 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"
            }]
        }

The API added a new ResourceRequirements field which was not accounted for in the comparison object yet.

Regarding the container_properties changes, 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 of 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 due to how the API canonicalizes the field:

--- 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 ResourceRequirements 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)

[gdavison]

LGTM 🚀

--- PASS: TestAccAWSBatchJobDefinition_basic (7.16s)
--- PASS: TestAccAWSBatchJobDefinition_updateForcesNewResource (12.17s)

[ghost]

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]

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!