Assign EfsVolumeConfiguration when present

[deanshelton913]

EFS never assigned?

I could be totally wrong, but I think this statement was copypasta from the if statement before.

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 OR Closes #0000

Release note for CHANGELOG:

Output from acceptance testing:

$ make testacc TEST=./aws TESTARGS='-run=TestAccAWSEcsTaskDefinition_withEFSVolumeMinimal'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSEcsTaskDefinition_withEFSVolumeMinimal -timeout 120m
=== RUN   TestAccAWSEcsTaskDefinition_withEFSVolumeMinimal
=== PAUSE TestAccAWSEcsTaskDefinition_withEFSVolumeMinimal
=== CONT  TestAccAWSEcsTaskDefinition_withEFSVolumeMinimal
--- PASS: TestAccAWSEcsTaskDefinition_withEFSVolumeMinimal (26.98s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       28.360s
...

[DrFaust92]

definitely a copypasta error from my side.
can you run the acceptance tests for the efs test case? i can help if needed

[deanshelton913]

Sure, i can give it a try, before that tho what do you expect will it cost to run acceptance? Not too concerned, just want to know: Pennies? Dollars? Gold bars? :P
I presume i should be running TestAccAWSEcsTaskDefinition_withEFSVolumeMinimal ?

Can you shortcut me and give me the acceptance test command you would expect for this?

Also: Wondering how this got past spec, as that test seems to actually assert that the efs volume is properly attached, no?

[DrFaust92]

@deanshelton913, as it creates and destroys and ecs task def + efs file system. pennies in the worst case?

this is how i run this:

I'm guessing this means that DockerVolumeConfiguration is never actually nil so it always "works" and tries to fetch the EFSConfig.

i'll investigate this as well. but your fix is needed either way

[deanshelton913]

Your test command is now showing up for me in your comment above, here is what i just ran:

make testacc TEST=./aws TESTARGS='-run=TestAccAWSEcsTaskDefinition_withEFSVolumeMinimal'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSEcsTaskDefinition_withEFSVolumeMinimal -timeout 120m
=== RUN   TestAccAWSEcsTaskDefinition_withEFSVolumeMinimal
=== PAUSE TestAccAWSEcsTaskDefinition_withEFSVolumeMinimal
=== CONT  TestAccAWSEcsTaskDefinition_withEFSVolumeMinimal
--- PASS: TestAccAWSEcsTaskDefinition_withEFSVolumeMinimal (26.98s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       28.360s

[deanshelton913]

Thats odd... the test fixture testAccAWSEcsTaskDefinitionWithEFSVolumeMinimal shows no assignment of DockerVolumeConfiguration for the setup for that test. I am not sure of the mechanics here, but just looking at the test, i would expect that value to be nil during this test run... I can try and debug aswell

[deanshelton913]

Confirmed using the ol' "Find the fmt.Print() statement in the bedsheet" technique. The DockerVolumeConfiguration value is indeed nil as expected... My attention would now be on the assertion, and how it is coming back as "all's well"

[deanshelton913]

If we merge this in, how soon will we see it in production?

[deanshelton913]

@DrFaust92 Any word on this?

[DrFaust92]

@deanshelton913, (unfortunately) i am not maintainer so i cant help speeding this up.
other than that i cant understand how this actually works without your change, but ive already seen some weird behaviour with AWS API before...

[DrFaust92]

@deanshelton913 , i have found the issue with this! its related to the fact that i didnt address hash diff for state.

its being addressed in #12751 i needed to use your fix in my PR for it to work, but i didnt bother to rebase it on top of your branch over a single line.

[deanshelton913]

Schweet. Glad it all worked out. I put a 👍 on your PR.
If you would have included my commit, i could claim i helped... but knowing that i contributed is all that matters.

My job is done here. 🦸‍♂️

I suppose we can close this PR.

[bflad]

Merging this in with #12571, thanks so much @deanshelton913 🚀

Output from acceptance testing:

--- PASS: TestAccAWSEcsTaskDefinition_arrays (14.80s)
--- PASS: TestAccAWSEcsTaskDefinition_basic (20.00s)
--- PASS: TestAccAWSEcsTaskDefinition_changeVolumesForcesNewResource (19.79s)
--- PASS: TestAccAWSEcsTaskDefinition_constraint (14.97s)
--- PASS: TestAccAWSEcsTaskDefinition_ExecutionRole (20.43s)
--- PASS: TestAccAWSEcsTaskDefinition_Fargate (19.62s)
--- PASS: TestAccAWSEcsTaskDefinition_Inactive (19.78s)
--- PASS: TestAccAWSEcsTaskDefinition_inferenceAccelerator (29.51s)
--- PASS: TestAccAWSEcsTaskDefinition_ProxyConfiguration (35.61s)
--- PASS: TestAccAWSEcsTaskDefinition_Tags (31.04s)
--- PASS: TestAccAWSEcsTaskDefinition_withDockerVolume (14.90s)
--- PASS: TestAccAWSEcsTaskDefinition_withDockerVolumeMinimalConfig (14.80s)
--- PASS: TestAccAWSEcsTaskDefinition_withEcsService (73.57s)
--- PASS: TestAccAWSEcsTaskDefinition_withEFSVolume (21.90s)
--- PASS: TestAccAWSEcsTaskDefinition_withEFSVolumeMinimal (21.71s)
--- PASS: TestAccAWSEcsTaskDefinition_withIPCMode (37.86s)
--- PASS: TestAccAWSEcsTaskDefinition_withNetworkMode (16.56s)
--- PASS: TestAccAWSEcsTaskDefinition_withPidMode (28.57s)
--- PASS: TestAccAWSEcsTaskDefinition_withScratchVolume (13.22s)
--- PASS: TestAccAWSEcsTaskDefinition_withTaskRoleArn (20.50s)
--- PASS: TestAccAWSEcsTaskDefinition_withTaskScopedDockerVolume (13.30s)

[ghost]

This has been released in version 2.64.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!