Merge pull request #29820 from hashicorp/b-batch-empty-var

resource/aws_batch_job_description: Ignore diffs on `container_properties` environment variables with an empty value

.changelog/29820.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+resource/aws_batch_job_definition: Prevents perpetual diff when container properties environment variable has empty value.
+```

internal/acctest/acctest.go

@@ -585,6 +585,47 @@ func CheckResourceAttrEquivalentJSON(resourceName, attributeName, expectedJSON s
 	}
 }
 
+func CheckResourceAttrJMES(name, key, jmesPath, value string) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		is, err := PrimaryInstanceState(s, name)
+		if err != nil {
+			return err
+		}
+
+		attr, ok := is.Attributes[key]
+		if !ok {
+			return fmt.Errorf("%s: Attribute %q not set", name, key)
+		}
+
+		var jsonData any
+		err = json.Unmarshal([]byte(attr), &jsonData)
+		if err != nil {
+			return fmt.Errorf("%s: Expected attribute %q to be JSON: %w", name, key, err)
+		}
+
+		result, err := jmespath.Search(jmesPath, jsonData)
+		if err != nil {
+			return fmt.Errorf("Invalid JMESPath %q: %w", jmesPath, err)
+		}
+
+		var v string
+		switch x := result.(type) {
+		case string:
+			v = x
+		case float64:
+			v = strconv.FormatFloat(x, 'f', -1, 64)
+		default:
+			return fmt.Errorf(`%[1]s: Attribute %[2]q, JMESPath %[3]q got "%#[4]v" (%[4]T)`, name, key, jmesPath, result)
+		}
+
+		if v != value {
+			return fmt.Errorf("%s: Attribute %q, JMESPath %q expected %#v, got %#v", name, key, jmesPath, value, v)
+		}
+
+		return nil
+	}
+}
+
 func CheckResourceAttrJMESPair(nameFirst, keyFirst, jmesPath, nameSecond, keySecond string) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		first, err := PrimaryInstanceState(s, nameFirst)
@@ -613,9 +654,14 @@ func CheckResourceAttrJMESPair(nameFirst, keyFirst, jmesPath, nameSecond, keySec
 			return fmt.Errorf("Invalid JMESPath %q: %w", jmesPath, err)
 		}
 
-		value, ok := result.(string)
-		if !ok {
-			return fmt.Errorf("%s: Attribute %q, JMESPath %q, expected single string, got %#v", nameFirst, keyFirst, jmesPath, result)
+		var value string
+		switch x := result.(type) {
+		case string:
+			value = x
+		case float64:
+			value = strconv.FormatFloat(x, 'f', -1, 64)
+		default:
+			return fmt.Errorf(`%[1]s: Attribute %[2]q, JMESPath %[3]q got "%#[4]v" (%[4]T)`, nameFirst, keyFirst, jmesPath, result)
 		}
 
 		vSecond, okSecond := second.Attributes[keySecond]

internal/errs/diag.go

@@ -42,6 +42,13 @@ func NewAttributeErrorDiagnostic(path cty.Path, summary, detail string) diag.Dia
 	)
 }
 
+func NewAttributeWarningDiagnostic(path cty.Path, summary, detail string) diag.Diagnostic {
+	return withPath(
+		NewWarningDiagnostic(summary, detail),
+		path,
+	)
+}
+
 func NewErrorDiagnostic(summary, detail string) diag.Diagnostic {
 	return diag.Diagnostic{
 		Severity: diag.Error,
@@ -50,6 +57,14 @@ func NewErrorDiagnostic(summary, detail string) diag.Diagnostic {
 	}
 }
 
+func NewWarningDiagnostic(summary, detail string) diag.Diagnostic {
+	return diag.Diagnostic{
+		Severity: diag.Warning,
+		Summary:  summary,
+		Detail:   detail,
+	}
+}
+
 func FromAttributeError(path cty.Path, err error) diag.Diagnostic {
 	return withPath(
 		NewErrorDiagnostic(err.Error(), ""),

internal/service/batch/container_properties.go

@@ -9,6 +9,7 @@ import (
 	"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"
+	tfslices "github.com/hashicorp/terraform-provider-aws/internal/slices"
 )
 
 type containerProperties batch.ContainerProperties
@@ -24,6 +25,14 @@ func (cp *containerProperties) Reduce() error {
 		cp.Command = nil
 	}
 
+	// Remove environment variables with empty values
+	cp.Environment = tfslices.Filter(cp.Environment, func(kvp *batch.KeyValuePair) bool {
+		if kvp == nil {
+			return false
+		}
+		return aws.StringValue(kvp.Value) != ""
+	})
+
 	// Prevent difference of API response that adds an empty array when not configured during the request
 	if len(cp.Environment) == 0 {
 		cp.Environment = nil

internal/service/batch/container_properties_test.go

@@ -9,21 +9,18 @@ import (
 func TestEquivalentContainerPropertiesJSON(t *testing.T) {
 	t.Parallel()
 
-	testCases := []struct {
-		Name              string
+	testCases := map[string]struct {
 		ApiJson           string
 		ConfigurationJson string
 		ExpectEquivalent  bool
 		ExpectError       bool
 	}{
-		{
-			Name:              "empty",
+		"empty": {
 			ApiJson:           ``,
 			ConfigurationJson: ``,
 			ExpectEquivalent:  true,
 		},
-		{
-			Name: "empty ResourceRequirements",
+		"empty ResourceRequirements": {
 			ApiJson: `
 {
 	"command": ["ls", "-la"],
@@ -99,8 +96,7 @@ func TestEquivalentContainerPropertiesJSON(t *testing.T) {
 `,
 			ExpectEquivalent: true,
 		},
-		{
-			Name: "reordered Environment",
+		"reordered Environment": {
 			ApiJson: `
 {
 	"command": ["ls", "-la"],
@@ -185,8 +181,7 @@ func TestEquivalentContainerPropertiesJSON(t *testing.T) {
 `,
 			ExpectEquivalent: true,
 		},
-		{
-			Name: "empty environment, mountPoints, ulimits, and volumes",
+		"empty environment, mountPoints, ulimits, and volumes": {
 			//lintignore:AWSAT005
 			ApiJson: `
 {
@@ -214,8 +209,7 @@ func TestEquivalentContainerPropertiesJSON(t *testing.T) {
 `,
 			ExpectEquivalent: true,
 		},
-		{
-			Name: "empty command, logConfiguration.secretOptions, mountPoints, resourceRequirements, secrets, ulimits, volumes",
+		"empty command, logConfiguration.secretOptions, mountPoints, resourceRequirements, secrets, ulimits, volumes": {
 			//lintignore:AWSAT003,AWSAT005
 			ApiJson: `
 {
@@ -256,8 +250,7 @@ func TestEquivalentContainerPropertiesJSON(t *testing.T) {
 `,
 			ExpectEquivalent: true,
 		},
-		{
-			Name: "no fargatePlatformConfiguration",
+		"no fargatePlatformConfiguration": {
 			//lintignore:AWSAT003,AWSAT005
 			ApiJson: `
 {
@@ -295,8 +288,7 @@ func TestEquivalentContainerPropertiesJSON(t *testing.T) {
 `,
 			ExpectEquivalent: true,
 		},
-		{
-			Name: "empty linuxParameters.devices, linuxParameters.tmpfs, logConfiguration.options",
+		"empty linuxParameters.devices, linuxParameters.tmpfs, logConfiguration.options": {
 			//lintignore:AWSAT003,AWSAT005
 			ApiJson: `
 {
@@ -334,8 +326,7 @@ func TestEquivalentContainerPropertiesJSON(t *testing.T) {
 `,
 			ExpectEquivalent: true,
 		},
-		{
-			Name: "empty linuxParameters.devices.permissions, linuxParameters.tmpfs.mountOptions",
+		"empty linuxParameters.devices.permissions, linuxParameters.tmpfs.mountOptions": {
 			//lintignore:AWSAT003,AWSAT005
 			ApiJson: `
 {
@@ -382,11 +373,84 @@ func TestEquivalentContainerPropertiesJSON(t *testing.T) {
 `,
 			ExpectEquivalent: true,
 		},
+		"empty environment variables": {
+			//lintignore:AWSAT005
+			ApiJson: `
+{
+	"image": "example:image",
+	"vcpus": 8,
+	"memory": 2048,
+	"command": ["start.py", "Ref::S3bucket", "Ref::S3key"],
+	"environment": [
+		{
+			"name": "VALUE",
+			"value": "test"
+		}
+	],
+	"jobRoleArn": "arn:aws:iam::123456789012:role/example",
+	"volumes": [],
+	"mountPoints": [],
+	"ulimits": [],
+	"resourceRequirements": []
+}`,
+			//lintignore:AWSAT005
+			ConfigurationJson: `
+{
+	"command": ["start.py", "Ref::S3bucket", "Ref::S3key"],
+	"image": "example:image",
+	"memory": 2048,
+	"vcpus": 8,
+	"environment": [
+		{
+			"name": "EMPTY",
+			"value": ""
+		},
+		{
+			"name": "VALUE",
+			"value": "test"
+		}
+	],
+	"jobRoleArn": "arn:aws:iam::123456789012:role/example"
+}`,
+			ExpectEquivalent: true,
+		},
+		"empty environment variable": {
+			//lintignore:AWSAT005
+			ApiJson: `
+{
+	"image": "example:image",
+	"vcpus": 8,
+	"memory": 2048,
+	"command": ["start.py", "Ref::S3bucket", "Ref::S3key"],
+	"environment": [],
+	"jobRoleArn": "arn:aws:iam::123456789012:role/example",
+	"volumes": [],
+	"mountPoints": [],
+	"ulimits": [],
+	"resourceRequirements": []
+}`,
+			//lintignore:AWSAT005
+			ConfigurationJson: `
+{
+	"command": ["start.py", "Ref::S3bucket", "Ref::S3key"],
+	"image": "example:image",
+	"memory": 2048,
+	"vcpus": 8,
+	"environment": [
+		{
+			"name": "EMPTY",
+			"value": ""
+		}
+	],
+	"jobRoleArn": "arn:aws:iam::123456789012:role/example"
+}`,
+			ExpectEquivalent: true,
+		},
 	}
 
-	for _, testCase := range testCases {
+	for name, testCase := range testCases {
 		testCase := testCase
-		t.Run(testCase.Name, func(t *testing.T) {
+		t.Run(name, func(t *testing.T) {
 			t.Parallel()
 
 			got, err := tfbatch.EquivalentContainerPropertiesJSON(testCase.ConfigurationJson, testCase.ApiJson)

internal/service/batch/job_definition.go

@@ -11,11 +11,13 @@ import (
 	"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/go-cty/cty"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/structure"
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
 	"github.com/hashicorp/terraform-provider-aws/internal/conns"
+	"github.com/hashicorp/terraform-provider-aws/internal/errs"
 	"github.com/hashicorp/terraform-provider-aws/internal/errs/sdkdiag"
 	"github.com/hashicorp/terraform-provider-aws/internal/flex"
 	tftags "github.com/hashicorp/terraform-provider-aws/internal/tags"
@@ -35,11 +37,9 @@ func ResourceJobDefinition() *schema.Resource {
 		},
 
 		Schema: map[string]*schema.Schema{
-			"name": {
-				Type:         schema.TypeString,
-				Required:     true,
-				ForceNew:     true,
-				ValidateFunc: validName,
+			"arn": {
+				Type:     schema.TypeString,
+				Computed: true,
 			},
 			"container_properties": {
 				Type:     schema.TypeString,
@@ -56,6 +56,12 @@ func ResourceJobDefinition() *schema.Resource {
 				},
 				ValidateFunc: validJobContainerProperties,
 			},
+			"name": {
+				Type:         schema.TypeString,
+				Required:     true,
+				ForceNew:     true,
+				ValidateFunc: validName,
+			},
 			"parameters": {
 				Type:     schema.TypeMap,
 				Optional: true,
@@ -71,6 +77,12 @@ func ResourceJobDefinition() *schema.Resource {
 					ValidateFunc: validation.StringInSlice(batch.PlatformCapability_Values(), false),
 				},
 			},
+			"propagate_tags": {
+				Type:     schema.TypeBool,
+				Optional: true,
+				ForceNew: true,
+				Default:  false,
+			},
 			"retry_strategy": {
 				Type:     schema.TypeList,
 				Optional: true,
@@ -134,14 +146,12 @@ func ResourceJobDefinition() *schema.Resource {
 					},
 				},
 			},
+			"revision": {
+				Type:     schema.TypeInt,
+				Computed: true,
+			},
 			"tags":     tftags.TagsSchema(),
 			"tags_all": tftags.TagsSchemaComputed(),
-			"propagate_tags": {
-				Type:     schema.TypeBool,
-				Optional: true,
-				ForceNew: true,
-				Default:  false,
-			},
 			"timeout": {
 				Type:     schema.TypeList,
 				Optional: true,
@@ -164,14 +174,6 @@ func ResourceJobDefinition() *schema.Resource {
 				ForceNew:     true,
 				ValidateFunc: validation.StringInSlice([]string{batch.JobDefinitionTypeContainer}, true),
 			},
-			"revision": {
-				Type:     schema.TypeInt,
-				Computed: true,
-			},
-			"arn": {
-				Type:     schema.TypeString,
-				Computed: true,
-			},
 		},
 
 		CustomizeDiff: verify.SetTagsDiff,
@@ -197,6 +199,16 @@ func resourceJobDefinitionCreate(ctx context.Context, d *schema.ResourceData, me
 			return sdkdiag.AppendErrorf(diags, "creating Batch Job Definition (%s): %s", name, err)
 		}
 
+		for _, env := range props.Environment {
+			if aws.StringValue(env.Value) == "" {
+				diags = append(diags, errs.NewAttributeWarningDiagnostic(
+					cty.GetAttrPath("container_properties"),
+					"Ignoring environment variable",
+					fmt.Sprintf("The environment variable %q has an empty value, which is ignored by the Batch service", aws.StringValue(env.Name))),
+				)
+			}
+		}
+
 		input.ContainerProperties = props
 	}