-
Notifications
You must be signed in to change notification settings - Fork 9.2k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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) ```
- Loading branch information
Showing
3 changed files
with
309 additions
and
3 deletions.
There are no files selected for viewing
76 changes: 76 additions & 0 deletions
76
aws/internal/service/batch/equivalency/container_properties.go
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| package equivalency | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "encoding/json" | ||
| "log" | ||
| "sort" | ||
|
|
||
| "github.com/aws/aws-sdk-go/aws" | ||
| "github.com/aws/aws-sdk-go/private/protocol/json/jsonutil" | ||
| "github.com/aws/aws-sdk-go/service/batch" | ||
| ) | ||
|
|
||
| type containerProperties batch.ContainerProperties | ||
|
|
||
| func (cp *containerProperties) Reduce() error { | ||
| // Deal with Environment objects which may be re-ordered in the API | ||
| sort.Slice(cp.Environment, func(i, j int) bool { | ||
| return aws.StringValue(cp.Environment[i].Name) < aws.StringValue(cp.Environment[j].Name) | ||
| }) | ||
|
|
||
| if len(cp.ResourceRequirements) == 0 { | ||
| cp.ResourceRequirements = nil | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| // EquivalentBatchContainerPropertiesJSON determines equality between two Batch ContainerProperties JSON strings | ||
| func EquivalentBatchContainerPropertiesJSON(str1, str2 string) (bool, error) { | ||
| if str1 == "" { | ||
| str1 = "{}" | ||
| } | ||
|
|
||
| if str2 == "" { | ||
| str2 = "{}" | ||
| } | ||
|
|
||
| var cp1, cp2 containerProperties | ||
|
|
||
| if err := json.Unmarshal([]byte(str1), &cp1); err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| if err := cp1.Reduce(); err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| canonicalJson1, err := jsonutil.BuildJSON(cp1) | ||
|
|
||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| if err := json.Unmarshal([]byte(str2), &cp2); err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| if err := cp2.Reduce(); err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| canonicalJson2, err := jsonutil.BuildJSON(cp2) | ||
|
|
||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| equal := bytes.Equal(canonicalJson1, canonicalJson2) | ||
|
|
||
| if !equal { | ||
| log.Printf("[DEBUG] Canonical Batch Container Properties JSON are not equal.\nFirst: %s\nSecond: %s\n", canonicalJson1, canonicalJson2) | ||
| } | ||
|
|
||
| return equal, nil | ||
| } |
203 changes: 203 additions & 0 deletions
203
aws/internal/service/batch/equivalency/container_properties_test.go
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,203 @@ | ||
| package equivalency | ||
|
|
||
| import ( | ||
| "testing" | ||
| ) | ||
|
|
||
| func TestEquivalentBatchContainerPropertiesJSON(t *testing.T) { | ||
| testCases := []struct { | ||
| Name string | ||
| ApiJson string | ||
| ConfigurationJson string | ||
| ExpectEquivalent bool | ||
| ExpectError bool | ||
| }{ | ||
| { | ||
| Name: "empty", | ||
| ApiJson: ``, | ||
| ConfigurationJson: ``, | ||
| ExpectEquivalent: true, | ||
| }, | ||
| { | ||
| Name: "empty ResourceRequirements", | ||
| ApiJson: ` | ||
| { | ||
| "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" | ||
| } | ||
| ] | ||
| } | ||
| `, | ||
| ConfigurationJson: ` | ||
| { | ||
| "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" | ||
| } | ||
| ] | ||
| } | ||
| `, | ||
| ExpectEquivalent: true, | ||
| }, | ||
| { | ||
| Name: "reordered Environment", | ||
| ApiJson: ` | ||
| { | ||
| "command": ["ls", "-la"], | ||
| "environment": [ | ||
| { | ||
| "name": "VARNAME1", | ||
| "value": "VARVAL1" | ||
| }, | ||
| { | ||
| "name": "VARNAME2", | ||
| "value": "VARVAL2" | ||
| } | ||
| ], | ||
| "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" | ||
| } | ||
| ] | ||
| } | ||
| `, | ||
| ConfigurationJson: ` | ||
| { | ||
| "command": ["ls", "-la"], | ||
| "environment": [ | ||
| { | ||
| "name": "VARNAME2", | ||
| "value": "VARVAL2" | ||
| }, | ||
| { | ||
| "name": "VARNAME1", | ||
| "value": "VARVAL1" | ||
| } | ||
| ], | ||
| "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" | ||
| } | ||
| ] | ||
| } | ||
| `, | ||
| ExpectEquivalent: true, | ||
| }, | ||
| } | ||
|
|
||
| for _, testCase := range testCases { | ||
| t.Run(testCase.Name, func(t *testing.T) { | ||
| got, err := EquivalentBatchContainerPropertiesJSON(testCase.ConfigurationJson, testCase.ApiJson) | ||
|
|
||
| if err != nil && !testCase.ExpectError { | ||
| t.Errorf("got unexpected error: %s", err) | ||
| } | ||
|
|
||
| if err == nil && testCase.ExpectError { | ||
| t.Errorf("expected error, but received none") | ||
| } | ||
|
|
||
| if got != testCase.ExpectEquivalent { | ||
| t.Errorf("got %t, expected %t", got, testCase.ExpectEquivalent) | ||
| } | ||
| }) | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters