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

Allow empty_dir volume shared across containers in container group #13374

Merged
merged 3 commits into from
Sep 23, 2021

Conversation

hterik
Copy link
Contributor

@hterik hterik commented Sep 16, 2021

fixes #11444

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

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

Thanks @hterik - overall this looks good but it seems all the tests have broken recently:

------ Stdout: -------
=== RUN   TestAccContainerGroup_SystemAssignedIdentity
=== PAUSE TestAccContainerGroup_SystemAssignedIdentity
=== CONT  TestAccContainerGroup_SystemAssignedIdentity
    testcase.go:88: Step 1/2 error: Error running apply: exit status 1
        
        Error: creating/updating container group "acctestcontainergroup-210916192259326918" (Resource Group "acctestRG-210916192259326918"): containerinstance.ContainerGroupsClient#CreateOrUpdate: Failure sending request: StatusCode=0 -- Original Error: Code="InaccessibleImage" Message="The image 'microsoft/aci-helloworld:latest' in container group 'acctestcontainergroup-210916192259326918' is not accessible. Please check the image and registry credential."
        
          with azurerm_container_group.test,
          on terraform_plugin_test.tf line 11, in resource "azurerm_container_group" "test":
          11: resource "azurerm_container_group" "test" {
        
--- FAIL: TestAccContainerGroup_SystemAssignedIdentity (65.81s)

could you update tests with a new image so we can test this and get it in? Also could we add a new test with an empty dir so we can ensure this stays fixed going forward? thanks!

@hterik
Copy link
Contributor Author

hterik commented Sep 17, 2021

Hi, I'm not sure what you mean by updating test with new image? Is this something that got broken by this change?

I can look into creating a new test using an empty_dir but will not be able to do it within a week i think, the acctest environment and giving it a az subscription is also new for me which will be some barrier, if any maintainer would like to continue on this you are more than welcome, otherwise i'll give it a shot next week.

btw, once this gets merged, how long until does it normally take for a change like this to become available in a release? Asking to know how invested i should get into workarounds or if i should just wait it out.

@github-actions github-actions bot added size/M and removed size/XS labels Sep 21, 2021
@hterik
Copy link
Contributor Author

hterik commented Sep 21, 2021

Please see f2ddce2 including a test for this change.
The test fails without the change and passes with it.

Note that the test actually doesn't fail if the test-file don't get picked up by the reader-image, since the framework only checks if the resource exists (check.That(data.ResourceName).ExistsInAzure), which it does even if the containers keep restarting. I couldn't find a way to test this any deeper with my limited knowledge in this area. At least it confirms that the container group can be created with this configuration.

I still could not run the old TestAccContainerGroup_emptyDirVolume test, neither with nor without this fix. It appears /bin/bash is missing from the seanmckenna/aci-hellofiles image which the test expects to use. Correct me if I'm wrong but I believe this is not affected by this change.

@catriona-m
Copy link
Member

Thanks for updating this @hterik, LGTM now 👍

@catriona-m catriona-m merged commit acaae93 into hashicorp:main Sep 23, 2021
catriona-m added a commit that referenced this pull request Sep 23, 2021
@github-actions
Copy link

This functionality has been released in v2.78.0 of the Terraform 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!

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

azurerm_container_group resource fails to create shared volume between containers in multi container group
3 participants