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.

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

r/aws_batch_job_definition: Suppress unnecessary environment variable diffs #21834

Merged

Conversation

ma2gedev
Copy link
Contributor

@ma2gedev ma2gedev commented Nov 18, 2021

Summary

This PR changes ordering AWS Batch job definition's environment variables when terraform-provider-aws reads job definition through AWS API and serializes to state. As the result, terraform's plan shows environment variables list without noisy diff.

Background

AWS Batch job definition's environment variables are showed as diff even if there is no change about it, because AWS API returns environment variables list without keeping order.

For example if job definition's memory is changed, terraform plan shows environment variables diff despite it is not changed. Like the following:

# example aws_batch_job_definition

resource "aws_batch_job_definition" "test" {
  name = "test_batch_env_var_list"
  type = "container"

  container_properties = <<CONTAINER_PROPERTIES
{
    "image": "busybox",
    "memory": 512,
    "vcpus": 1,
    "environment": [
        {"name": "VARNAME1", "value": "VARVAL1"},
        {"name": "VARNAME2", "value": "VARVAL2"},
        {"name": "VARNAME3", "value": "VARVAL3"},
        {"name": "VARNAME4", "value": "VARVAL4"}
    ]
}
CONTAINER_PROPERTIES
}

# got the following diff if memory was changed to 1024. Environment variables were not changed, but diff was showed.

      ~ container_properties  = jsonencode(
          ~ {
              - command              = [] -> null
              ~ environment          = [
                  ~ {
                      ~ name  = "VARNAME3" -> "VARNAME1"
                      ~ value = "VARVAL3" -> "VARVAL1"
                    },
                    {
                        name  = "VARNAME2"
                        value = "VARVAL2"
                    },
                  + {
                      + name  = "VARNAME3"
                      + value = "VARVAL3"
                    },
                    {
                        name  = "VARNAME4"
                        value = "VARVAL4"
                    },
                  - {
                      - name  = "VARNAME1"
                      - value = "VARVAL1"
                    },
                ]
              ~ memory               = 512 -> 1024
              - mountPoints          = [] -> null
              - resourceRequirements = [] -> null
              - secrets              = [] -> null
              - ulimits              = [] -> null
              - volumes              = [] -> null
                # (2 unchanged elements hidden)
            } # forces replacement
        )

Actually same problem existed in AWS ECS task definition, but it was solved with #11463 . That PR's approach was ordering environment variables list, therefore I follow the way to solve problem to keep code consistency.

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

Relates #11463
Closes #34947

Output from acceptance testing:

This change is passed unit tests, and worked in my project, but I could not do acceptance tests.

@github-actions github-actions bot added service/batch Issues and PRs that pertain to the batch service. size/XS Managed by automation to categorize the size of a PR. needs-triage Waiting for first response or review from a maintainer. and removed service/batch Issues and PRs that pertain to the batch service. labels Nov 18, 2021
@github-actions github-actions bot added the service/batch Issues and PRs that pertain to the batch service. label Nov 18, 2021
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome @ma2gedev 👋

It looks like this is your first Pull Request submission to the Terraform AWS Provider! If you haven’t already done so please make sure you have checked out our CONTRIBUTING guide and FAQ to make sure your contribution is adhering to best practice and has all the necessary elements in place for a successful approval.

Also take a look at our FAQ which details how we prioritize Pull Requests for inclusion.

Thanks again, and welcome to the community! 😃

@ma2gedev ma2gedev marked this pull request as ready for review November 18, 2021 15:20
@justinretzolk justinretzolk added bug Addresses a defect in current functionality. and removed needs-triage Waiting for first response or review from a maintainer. labels Nov 18, 2021
@rbreslow
Copy link

@justinretzolk Hey Justin. Any word on the triage for this issue? We're encountering it (just as we encountered the issue w/ ECS tasks in the past) and it creates a noisy diff each time something small in the job definition changes.

@justinretzolk
Copy link
Member

Hey @rbreslow 👋 Thank you for checking in on this, and apologies for taking so long to get back to you. Unfortunately, I'm not able to provide an estimate on when this will be merged/reviewed due to the potential of shifting priorities (we prioritize work by count of ":+1:" reactions, as well as a few other things). For more information on how we prioritize, check out our prioritization guide.

@james-mullen3
Copy link

Is there anyway I could assist with issue @justinretzolk? It is impacting us and making Terraform plans very hard to read for actual changes vs env var ordering issues.

@rbreslow
Copy link

Yeah, such a small diff, depressing hasn't been resolved and we're in 2024.

@justinretzolk
Copy link
Member

Hey all 👋 Thanks for checking in on this one. I suspect it's been a bit tougher to get votes on this one, because I imagine a lot of people ignore the noisy diff and move on. We have dedicated days for trying to get through smaller PRs like this, and I've got this PR on that list to try to get it reviewed a bit quicker.

@danb-csms
Copy link

This bug causes our team significant pain, any news on this being merged?

@ulfbayte
Copy link

Hello,

Any news on this bug ? It's still an issue for us, it make reading plan a real pain :(

@senseiops
Copy link

Hey all 👋 Thanks for checking in on this one. I suspect it's been a bit tougher to get votes on this one, because I imagine a lot of people ignore the noisy diff and move on. We have dedicated days for trying to get through smaller PRs like this, and I've got this PR on that list to try to get it reviewed a bit quicker.

Same here, the diffs are quite annoying.
Any news on the small PR todo list @justinretzolk ?
Thanks.

@ewbankkit ewbankkit requested a review from a team as a code owner October 9, 2024 17:12
Copy link
Contributor

@ewbankkit ewbankkit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀.

% make testacc TESTARGS='-run=TestAccBatchJobDefinition_' PKG=batch ACCTEST_PARALLELISM=3
make: Verifying source code with gofmt...
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go1.23.2 test ./internal/service/batch/... -v -count 1 -parallel 3  -run=TestAccBatchJobDefinition_ -timeout 360m
=== RUN   TestAccBatchJobDefinition_tags
=== PAUSE TestAccBatchJobDefinition_tags
=== RUN   TestAccBatchJobDefinition_tags_null
=== PAUSE TestAccBatchJobDefinition_tags_null
=== RUN   TestAccBatchJobDefinition_tags_EmptyMap
=== PAUSE TestAccBatchJobDefinition_tags_EmptyMap
=== RUN   TestAccBatchJobDefinition_tags_AddOnUpdate
=== PAUSE TestAccBatchJobDefinition_tags_AddOnUpdate
=== RUN   TestAccBatchJobDefinition_tags_EmptyTag_OnCreate
=== PAUSE TestAccBatchJobDefinition_tags_EmptyTag_OnCreate
=== RUN   TestAccBatchJobDefinition_tags_EmptyTag_OnUpdate_Add
=== PAUSE TestAccBatchJobDefinition_tags_EmptyTag_OnUpdate_Add
=== RUN   TestAccBatchJobDefinition_tags_EmptyTag_OnUpdate_Replace
=== PAUSE TestAccBatchJobDefinition_tags_EmptyTag_OnUpdate_Replace
=== RUN   TestAccBatchJobDefinition_tags_DefaultTags_providerOnly
=== PAUSE TestAccBatchJobDefinition_tags_DefaultTags_providerOnly
=== RUN   TestAccBatchJobDefinition_tags_DefaultTags_nonOverlapping
=== PAUSE TestAccBatchJobDefinition_tags_DefaultTags_nonOverlapping
=== RUN   TestAccBatchJobDefinition_tags_DefaultTags_overlapping
=== PAUSE TestAccBatchJobDefinition_tags_DefaultTags_overlapping
=== RUN   TestAccBatchJobDefinition_tags_DefaultTags_updateToProviderOnly
=== PAUSE TestAccBatchJobDefinition_tags_DefaultTags_updateToProviderOnly
=== RUN   TestAccBatchJobDefinition_tags_DefaultTags_updateToResourceOnly
=== PAUSE TestAccBatchJobDefinition_tags_DefaultTags_updateToResourceOnly
=== RUN   TestAccBatchJobDefinition_tags_DefaultTags_emptyResourceTag
=== PAUSE TestAccBatchJobDefinition_tags_DefaultTags_emptyResourceTag
=== RUN   TestAccBatchJobDefinition_tags_DefaultTags_emptyProviderOnlyTag
=== PAUSE TestAccBatchJobDefinition_tags_DefaultTags_emptyProviderOnlyTag
=== RUN   TestAccBatchJobDefinition_tags_DefaultTags_nullOverlappingResourceTag
=== PAUSE TestAccBatchJobDefinition_tags_DefaultTags_nullOverlappingResourceTag
=== RUN   TestAccBatchJobDefinition_tags_DefaultTags_nullNonOverlappingResourceTag
=== PAUSE TestAccBatchJobDefinition_tags_DefaultTags_nullNonOverlappingResourceTag
=== RUN   TestAccBatchJobDefinition_tags_ComputedTag_OnCreate
=== PAUSE TestAccBatchJobDefinition_tags_ComputedTag_OnCreate
=== RUN   TestAccBatchJobDefinition_tags_ComputedTag_OnUpdate_Add
=== PAUSE TestAccBatchJobDefinition_tags_ComputedTag_OnUpdate_Add
=== RUN   TestAccBatchJobDefinition_tags_ComputedTag_OnUpdate_Replace
=== PAUSE TestAccBatchJobDefinition_tags_ComputedTag_OnUpdate_Replace
=== RUN   TestAccBatchJobDefinition_basic
=== PAUSE TestAccBatchJobDefinition_basic
=== RUN   TestAccBatchJobDefinition_attributes
=== PAUSE TestAccBatchJobDefinition_attributes
=== RUN   TestAccBatchJobDefinition_disappears
=== PAUSE TestAccBatchJobDefinition_disappears
=== RUN   TestAccBatchJobDefinition_PlatformCapabilities_ec2
=== PAUSE TestAccBatchJobDefinition_PlatformCapabilities_ec2
=== RUN   TestAccBatchJobDefinition_PlatformCapabilitiesFargate_containerPropertiesDefaults
=== PAUSE TestAccBatchJobDefinition_PlatformCapabilitiesFargate_containerPropertiesDefaults
=== RUN   TestAccBatchJobDefinition_PlatformCapabilities_fargate
=== PAUSE TestAccBatchJobDefinition_PlatformCapabilities_fargate
=== RUN   TestAccBatchJobDefinition_ContainerProperties_advanced
=== PAUSE TestAccBatchJobDefinition_ContainerProperties_advanced
=== RUN   TestAccBatchJobDefinition_ContainerProperties_minorUpdate
=== PAUSE TestAccBatchJobDefinition_ContainerProperties_minorUpdate
=== RUN   TestAccBatchJobDefinition_propagateTags
=== PAUSE TestAccBatchJobDefinition_propagateTags
=== RUN   TestAccBatchJobDefinition_ContainerProperties_EmptyField
=== PAUSE TestAccBatchJobDefinition_ContainerProperties_EmptyField
=== RUN   TestAccBatchJobDefinition_NodeProperties_basic
=== PAUSE TestAccBatchJobDefinition_NodeProperties_basic
=== RUN   TestAccBatchJobDefinition_NodeProperties_advanced
=== PAUSE TestAccBatchJobDefinition_NodeProperties_advanced
=== RUN   TestAccBatchJobDefinition_EKSProperties_basic
=== PAUSE TestAccBatchJobDefinition_EKSProperties_basic
=== RUN   TestAccBatchJobDefinition_EKSProperties_update
=== PAUSE TestAccBatchJobDefinition_EKSProperties_update
=== RUN   TestAccBatchJobDefinition_EKSProperties_imagePullSecrets
=== PAUSE TestAccBatchJobDefinition_EKSProperties_imagePullSecrets
=== RUN   TestAccBatchJobDefinition_createTypeContainerWithNodeProperties
=== PAUSE TestAccBatchJobDefinition_createTypeContainerWithNodeProperties
=== RUN   TestAccBatchJobDefinition_createTypeMultiNodeWithContainerProperties
=== PAUSE TestAccBatchJobDefinition_createTypeMultiNodeWithContainerProperties
=== RUN   TestAccBatchJobDefinition_schedulingPriority
=== PAUSE TestAccBatchJobDefinition_schedulingPriority
=== RUN   TestAccBatchJobDefinition_emptyRetryStrategy
=== PAUSE TestAccBatchJobDefinition_emptyRetryStrategy
=== RUN   TestAccBatchJobDefinition_ECSProperties_update
=== PAUSE TestAccBatchJobDefinition_ECSProperties_update
=== CONT  TestAccBatchJobDefinition_tags
=== CONT  TestAccBatchJobDefinition_attributes
=== CONT  TestAccBatchJobDefinition_NodeProperties_advanced
--- PASS: TestAccBatchJobDefinition_NodeProperties_advanced (28.90s)
=== CONT  TestAccBatchJobDefinition_createTypeMultiNodeWithContainerProperties
--- PASS: TestAccBatchJobDefinition_createTypeMultiNodeWithContainerProperties (4.80s)
=== CONT  TestAccBatchJobDefinition_ECSProperties_update
--- PASS: TestAccBatchJobDefinition_attributes (47.05s)
=== CONT  TestAccBatchJobDefinition_emptyRetryStrategy
--- PASS: TestAccBatchJobDefinition_ECSProperties_update (25.85s)
=== CONT  TestAccBatchJobDefinition_schedulingPriority
--- PASS: TestAccBatchJobDefinition_emptyRetryStrategy (15.64s)
=== CONT  TestAccBatchJobDefinition_tags_DefaultTags_updateToProviderOnly
--- PASS: TestAccBatchJobDefinition_tags (63.86s)
=== CONT  TestAccBatchJobDefinition_basic
--- PASS: TestAccBatchJobDefinition_schedulingPriority (16.68s)
=== CONT  TestAccBatchJobDefinition_tags_ComputedTag_OnUpdate_Replace
--- PASS: TestAccBatchJobDefinition_basic (16.53s)
=== CONT  TestAccBatchJobDefinition_tags_ComputedTag_OnUpdate_Add
--- PASS: TestAccBatchJobDefinition_tags_DefaultTags_updateToProviderOnly (29.64s)
=== CONT  TestAccBatchJobDefinition_tags_ComputedTag_OnCreate
--- PASS: TestAccBatchJobDefinition_tags_ComputedTag_OnUpdate_Replace (36.96s)
=== CONT  TestAccBatchJobDefinition_tags_DefaultTags_emptyResourceTag
--- PASS: TestAccBatchJobDefinition_tags_ComputedTag_OnUpdate_Add (36.53s)
=== CONT  TestAccBatchJobDefinition_tags_DefaultTags_updateToResourceOnly
--- PASS: TestAccBatchJobDefinition_tags_ComputedTag_OnCreate (25.36s)
=== CONT  TestAccBatchJobDefinition_tags_DefaultTags_nullNonOverlappingResourceTag
--- PASS: TestAccBatchJobDefinition_tags_DefaultTags_emptyResourceTag (18.37s)
=== CONT  TestAccBatchJobDefinition_tags_EmptyTag_OnUpdate_Add
--- PASS: TestAccBatchJobDefinition_tags_DefaultTags_nullNonOverlappingResourceTag (18.24s)
=== CONT  TestAccBatchJobDefinition_tags_DefaultTags_overlapping
--- PASS: TestAccBatchJobDefinition_tags_DefaultTags_updateToResourceOnly (27.62s)
=== CONT  TestAccBatchJobDefinition_tags_DefaultTags_nonOverlapping
--- PASS: TestAccBatchJobDefinition_tags_EmptyTag_OnUpdate_Add (43.22s)
=== CONT  TestAccBatchJobDefinition_tags_DefaultTags_providerOnly
--- PASS: TestAccBatchJobDefinition_tags_DefaultTags_overlapping (48.17s)
=== CONT  TestAccBatchJobDefinition_tags_EmptyTag_OnUpdate_Replace
--- PASS: TestAccBatchJobDefinition_tags_DefaultTags_nonOverlapping (48.15s)
=== CONT  TestAccBatchJobDefinition_ContainerProperties_advanced
--- PASS: TestAccBatchJobDefinition_tags_EmptyTag_OnUpdate_Replace (28.97s)
=== CONT  TestAccBatchJobDefinition_NodeProperties_basic
=== CONT  TestAccBatchJobDefinition_ContainerProperties_EmptyField
--- PASS: TestAccBatchJobDefinition_NodeProperties_basic (18.20s)
--- PASS: TestAccBatchJobDefinition_tags_DefaultTags_providerOnly (62.73s)
=== CONT  TestAccBatchJobDefinition_propagateTags
--- PASS: TestAccBatchJobDefinition_ContainerProperties_EmptyField (16.53s)
=== CONT  TestAccBatchJobDefinition_ContainerProperties_minorUpdate
--- PASS: TestAccBatchJobDefinition_propagateTags (13.10s)
=== CONT  TestAccBatchJobDefinition_EKSProperties_imagePullSecrets
--- PASS: TestAccBatchJobDefinition_ContainerProperties_advanced (71.43s)
=== CONT  TestAccBatchJobDefinition_tags_DefaultTags_emptyProviderOnlyTag
--- PASS: TestAccBatchJobDefinition_EKSProperties_imagePullSecrets (16.18s)
=== CONT  TestAccBatchJobDefinition_createTypeContainerWithNodeProperties
--- PASS: TestAccBatchJobDefinition_createTypeContainerWithNodeProperties (4.65s)
=== CONT  TestAccBatchJobDefinition_tags_DefaultTags_nullOverlappingResourceTag
--- PASS: TestAccBatchJobDefinition_tags_DefaultTags_emptyProviderOnlyTag (18.04s)
=== CONT  TestAccBatchJobDefinition_PlatformCapabilitiesFargate_containerPropertiesDefaults
--- PASS: TestAccBatchJobDefinition_tags_DefaultTags_nullOverlappingResourceTag (18.31s)
=== CONT  TestAccBatchJobDefinition_PlatformCapabilities_fargate
--- PASS: TestAccBatchJobDefinition_PlatformCapabilitiesFargate_containerPropertiesDefaults (17.27s)
=== CONT  TestAccBatchJobDefinition_PlatformCapabilities_ec2
--- PASS: TestAccBatchJobDefinition_PlatformCapabilities_fargate (17.19s)
=== CONT  TestAccBatchJobDefinition_EKSProperties_update
--- PASS: TestAccBatchJobDefinition_PlatformCapabilities_ec2 (16.23s)
=== CONT  TestAccBatchJobDefinition_EKSProperties_basic
--- PASS: TestAccBatchJobDefinition_ContainerProperties_minorUpdate (78.00s)
=== CONT  TestAccBatchJobDefinition_tags_AddOnUpdate
--- PASS: TestAccBatchJobDefinition_EKSProperties_basic (16.37s)
=== CONT  TestAccBatchJobDefinition_disappears
--- PASS: TestAccBatchJobDefinition_EKSProperties_update (26.30s)
=== CONT  TestAccBatchJobDefinition_tags_EmptyTag_OnCreate
--- PASS: TestAccBatchJobDefinition_disappears (14.13s)
=== CONT  TestAccBatchJobDefinition_tags_EmptyMap
--- PASS: TestAccBatchJobDefinition_tags_AddOnUpdate (28.64s)
=== CONT  TestAccBatchJobDefinition_tags_null
--- PASS: TestAccBatchJobDefinition_tags_EmptyTag_OnCreate (32.29s)
--- PASS: TestAccBatchJobDefinition_tags_EmptyMap (22.32s)
--- PASS: TestAccBatchJobDefinition_tags_null (22.13s)
PASS
ok  	github.com/hashicorp/terraform-provider-aws/internal/service/batch	381.731s

@ewbankkit
Copy link
Contributor

@ma2gedev Thanks for the contribution 🎉 👏.

@ewbankkit ewbankkit merged commit fc125af into hashicorp:main Oct 9, 2024
31 checks passed
@github-actions github-actions bot added this to the v5.71.0 milestone Oct 9, 2024
@ma2gedev ma2gedev deleted the f-aws_batch_job_definition-env-var-ordering branch October 10, 2024 02:27
@ma2gedev
Copy link
Contributor Author

@ewbankkit Thank you very much 🎉

@danb-csms
Copy link

thank you for getting this in, will make a big difference to our team

@ulfbayte
Copy link

Thanks, it’s really appreciated !

Copy link

This functionality has been released in v5.72.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. Thank you!

@ma2gedev
Copy link
Contributor Author

@ewbankkit I just tried v5.72.1, but it didn't suppress environment variable diffs. This is because your commit b710358 does not contain the changes (52f6bfa) to internal/service/batch/job_definition.go . Could you incorporate the changes to internal/service/batch/job_definition.go?
Thank you in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Addresses a defect in current functionality. service/batch Issues and PRs that pertain to the batch service. size/XS Managed by automation to categorize the size of a PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: aws_batch_job_definition environment variables diff unclear
8 participants