Skip to content

Commit

Permalink
resource/aws_batch_job_definition: Properly set container_properties …
Browse files Browse the repository at this point in the history 
…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)
```
  • Loading branch information
@bflad
bflad authored Feb 5, 2020
1 parent 167c8bb commit 0a8efb1
Show file tree
Hide file tree
Showing 4 changed files with 314 additions and 4 deletions.
76 changes: 76 additions & 0 deletions aws/internal/service/batch/equivalency/container_properties.go
View file
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 aws/internal/service/batch/equivalency/container_properties_test.go
View file
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)
}
})
}
}
35 changes: 32 additions & 3 deletions aws/resource_aws_batch_job_definition.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,12 @@ import (
"encoding/json"

"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"
"github.com/hashicorp/terraform-plugin-sdk/helper/schema"
"github.com/hashicorp/terraform-plugin-sdk/helper/structure"
"github.com/hashicorp/terraform-plugin-sdk/helper/validation"
"github.com/terraform-providers/terraform-provider-aws/aws/internal/service/batch/equivalency"
)

func resourceAwsBatchJobDefinition() *schema.Resource {
Expand All @@ -33,8 +35,12 @@ func resourceAwsBatchJobDefinition() *schema.Resource {
json, _ := structure.NormalizeJsonString(v)
return json
},
DiffSuppressFunc: suppressEquivalentJsonDiffs,
ValidateFunc: validateAwsBatchJobContainerProperties,
DiffSuppressFunc: func(k, old, new string, d *schema.ResourceData) bool {
equal, _ := equivalency.EquivalentBatchContainerPropertiesJSON(old, new)

return equal
},
ValidateFunc: validateAwsBatchJobContainerProperties,
},
"parameters": {
Type: schema.TypeMap,
Expand Down Expand Up @@ -142,7 +148,19 @@ func resourceAwsBatchJobDefinitionRead(d *schema.ResourceData, meta interface{})
return nil
}
d.Set("arn", job.JobDefinitionArn)
d.Set("container_properties", job.ContainerProperties)

containerProperties, err := flattenBatchContainerProperties(job.ContainerProperties)

if err != nil {
return fmt.Errorf("error converting Batch Container Properties to JSON: %s", err)
}

if err := d.Set("container_properties", containerProperties); err != nil {
return fmt.Errorf("error setting container_properties: %s", err)
}

d.Set("name", job.JobDefinitionName)

d.Set("parameters", aws.StringValueMap(job.Parameters))

if err := d.Set("retry_strategy", flattenBatchRetryStrategy(job.RetryStrategy)); err != nil {
Expand Down Expand Up @@ -215,6 +233,17 @@ func expandBatchJobContainerProperties(rawProps string) (*batch.ContainerPropert
return props, nil
}

// Convert batch.ContainerProperties object into its JSON representation
func flattenBatchContainerProperties(containerProperties *batch.ContainerProperties) (string, error) {
b, err := jsonutil.BuildJSON(containerProperties)

if err != nil {
return "", err
}

return string(b), nil
}

func expandJobDefinitionParameters(params map[string]interface{}) map[string]*string {
var jobParams = make(map[string]*string)
for k, v := range params {
Expand Down
4 changes: 3 additions & 1 deletion aws/resource_aws_batch_job_definition_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,16 +37,17 @@ func TestAccAWSBatchJobDefinition_basic(t *testing.T) {
MountPoints: []*batch.MountPoint{
{ContainerPath: aws.String("/tmp"), ReadOnly: aws.Bool(false), SourceVolume: aws.String("tmp")},
},
ResourceRequirements: []*batch.ResourceRequirement{},
Ulimits: []*batch.Ulimit{
{HardLimit: aws.Int64(int64(1024)), Name: aws.String("nofile"), SoftLimit: aws.Int64(int64(1024))},
},
Vcpus: aws.Int64(int64(1)),
Volumes: []*batch.Volume{
{
Host: &batch.Host{SourcePath: aws.String("/tmp")},
Name: aws.String("tmp"),
},
},
Vcpus: aws.Int64(int64(1)),
},
}
ri := acctest.RandInt()
Expand All @@ -60,6 +61,7 @@ func TestAccAWSBatchJobDefinition_basic(t *testing.T) {
Config: config,
Check: resource.ComposeTestCheckFunc(
testAccCheckBatchJobDefinitionExists("aws_batch_job_definition.test", &jd),
resource.TestCheckResourceAttr("aws_batch_job_definition.test", "name", fmt.Sprintf("tf_acctest_batch_job_definition_%d", ri)),
testAccCheckBatchJobDefinitionAttributes(&jd, &compare),
),
},
Expand Down

0 comments on commit 0a8efb1

Please sign in to comment.