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

Updated API version of container_group from 2021-03-01 to 2021-10-01 #17785

Merged
merged 6 commits into from
Oct 21, 2022

Conversation

dkuzmenok
Copy link
Contributor

This PR updates API, used by container_group from 2021-03-01 to 2021-10-01.

API changes: https://docs.microsoft.com/en-us/azure/templates/microsoft.containerinstance/change-log/containergroups#2021-10-01

Included changes:

  • Removed network_profile_id (deprecated since 2021-07-01).
  • Added image_registry_credentials.user_assigned_identity_id (added in 2021-07-01).
  • Added subnets (added in 2021-07-01).
  • Added zones (added in 2021-09-01).
  • Added dns_name_label_reuse_policy (added in 2021-10-01).

Notes:

Fixes #17371.

Tests:


TF_ACC=1 go test -v -timeout 300000s -run ^TestAccContainerGroup_ github.com/hashicorp/terraform-provider-azurerm/internal/services/containers/
...
=== CONT  TestAccContainerGroup_SystemAssignedIdentity
=== CONT  TestAccContainerGroup_gitRepoVolume
=== CONT  TestAccContainerGroup_imageRegistryCredentials
=== CONT  TestAccContainerGroup_linuxBasicTagsUpdate
--- PASS: TestAccContainerGroup_imageRegistryCredentials (192.04s)
=== CONT  TestAccContainerGroup_withPrivateEmpty
--- PASS: TestAccContainerGroup_SystemAssignedIdentity (194.49s)
=== CONT  TestAccContainerGroup_windowsComplete
--- PASS: TestAccContainerGroup_gitRepoVolume (208.86s)
=== CONT  TestAccContainerGroup_windowsBasic
--- PASS: TestAccContainerGroup_linuxBasicTagsUpdate (233.73s)
=== CONT  TestAccContainerGroup_SystemAssignedIdentityVirtualNetwork
--- PASS: TestAccContainerGroup_withPrivateEmpty (146.22s)
=== CONT  TestAccContainerGroup_virtualNetworkParallel
--- PASS: TestAccContainerGroup_SystemAssignedIdentityVirtualNetwork (247.13s)
=== CONT  TestAccContainerGroup_virtualNetwork
--- PASS: TestAccContainerGroup_virtualNetworkParallel (252.23s)
=== CONT  TestAccContainerGroup_linuxComplete
--- PASS: TestAccContainerGroup_windowsBasic (462.40s)
=== CONT  TestAccContainerGroup_exposedPort
--- PASS: TestAccContainerGroup_virtualNetwork (310.90s)
=== CONT  TestAccContainerGroup_exposedPortUpdate
--- PASS: TestAccContainerGroup_exposedPort (146.25s)
=== CONT  TestAccContainerGroup_linuxBasicUpdate
--- PASS: TestAccContainerGroup_exposedPortUpdate (212.74s)
=== CONT  TestAccContainerGroup_requiresImport
--- PASS: TestAccContainerGroup_linuxBasicUpdate (203.53s)
=== CONT  TestAccContainerGroup_SystemAssignedIdentityWithZones
--- PASS: TestAccContainerGroup_windowsComplete (910.18s)
=== CONT  TestAccContainerGroup_multipleAssignedIdentities
--- PASS: TestAccContainerGroup_requiresImport (140.12s)
=== CONT  TestAccContainerGroup_UserAssignedIdentityWithVirtualNetwork
--- PASS: TestAccContainerGroup_SystemAssignedIdentityWithZones (129.69s)
=== CONT  TestAccContainerGroup_UserAssignedIdentity
--- PASS: TestAccContainerGroup_multipleAssignedIdentities (160.36s)
=== CONT  TestAccContainerGroup_logTypeUnset
--- PASS: TestAccContainerGroup_UserAssignedIdentity (143.85s)
=== CONT  TestAccContainerGroup_linuxBasic
--- PASS: TestAccContainerGroup_logTypeUnset (173.14s)
=== CONT  TestAccContainerGroup_imageRegistryCredentialsUpdate
--- PASS: TestAccContainerGroup_UserAssignedIdentityWithVirtualNetwork (331.17s)
=== CONT  TestAccContainerGroup_ProbeExec
--- PASS: TestAccContainerGroup_linuxBasic (188.85s)
=== CONT  TestAccContainerGroup_SystemAssignedIdentityNoNetwork
--- PASS: TestAccContainerGroup_ProbeExec (135.10s)
=== CONT  TestAccContainerGroup_withInitContainer
--- PASS: TestAccContainerGroup_SystemAssignedIdentityNoNetwork (136.86s)
=== CONT  TestAccContainerGroup_encryption
--- PASS: TestAccContainerGroup_imageRegistryCredentialsUpdate (217.63s)
=== CONT  TestAccContainerGroup_secretVolume
--- PASS: TestAccContainerGroup_withInitContainer (125.43s)
=== CONT  TestAccContainerGroup_emptyDirVolumeShared
--- PASS: TestAccContainerGroup_secretVolume (130.87s)
=== CONT  TestAccContainerGroup_emptyDirVolumeSharedWithInitContainer
--- PASS: TestAccContainerGroup_emptyDirVolumeShared (134.25s)
=== CONT  TestAccContainerGroup_ProbeHttpGet
--- PASS: TestAccContainerGroup_linuxComplete (1320.01s)
=== CONT  TestAccContainerGroup_emptyDirVolume
--- PASS: TestAccContainerGroup_emptyDirVolumeSharedWithInitContainer (128.24s)
--- PASS: TestAccContainerGroup_encryption (350.81s)
--- PASS: TestAccContainerGroup_ProbeHttpGet (137.60s)
--- PASS: TestAccContainerGroup_emptyDirVolume (136.39s)
PASS
ok      github.com/hashicorp/terraform-provider-azurerm/internal/services/containers    2046.927s

Copy link
Collaborator

@magodo magodo left a comment

Choose a reason for hiding this comment

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

Thanks for this PR!
I've taken a look through and left some comments inline, but this is mostly looking good to me 👍

@dkuzmenok dkuzmenok force-pushed the container-group-upgrade branch from 9e95d7c to 8bc3b62 Compare August 12, 2022 08:05
@dkuzmenok
Copy link
Contributor Author

@magodo I have resolved all the points, raised by you.
Removed tests' modifications and now they look good.
Tests TestAccContainerGroup_ are all passed.

@dkuzmenok
Copy link
Contributor Author

@magodo There was a failing check, coming from main branch (not related to this PR). I have fixed it in #18219, so this one should be fine and ready.

@magodo
Copy link
Collaborator

magodo commented Sep 5, 2022

@tombuildsstuff This PR has removed network_profile_id (similar as #17926), shall we move it to the v4 list?

@asbjorn-wiik
Copy link

@tombuildsstuff This PR has removed network_profile_id (similar as #17926), shall we move it to the v4 list?
Seems like #17926 was already merged.
Is there any reason for waiting for v4, is it the breaking-change label?
If v4 is the target release, what is the timeframe for that (ballpark)?
Our current setup with azurerm (3.23.0) provider has issues when changing ACI config and Terraform has to do a destroy and create. That fails for ACI on the private network.

@magodo
Copy link
Collaborator

magodo commented Sep 22, 2022

@asbjorn-wiik That PR is then reverted in #18239.

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 for the pr @dkuzmenok - however we cannot just remove properties like this outside of a breaking change. Is it possible to just deprecate the network profile id property for now and remove it in 4.0?

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.

We also have some test failures:
image

@dkuzmenok
Copy link
Contributor Author

We also have some test failures: image

@katbyte I would fix the first one, but the second one is not linked to by changes, just showing that "K80" GPU SKU is not available in EUS2 location.
I did expect to have PrimaryLocation being set to "North Europe", where this SKU is available - https://learn.microsoft.com/en-us/azure/container-instances/container-instances-gpu . Not sure what locations are used for running tests in your env.

@dkuzmenok dkuzmenok force-pushed the container-group-upgrade branch from 2f1a47f to 0b88748 Compare October 17, 2022 11:09
@dkuzmenok
Copy link
Contributor Author

@katbyte I've updated this one. All tests now complete successfully.
There is a Deprecated enabled over network_profile_id.

@dkuzmenok dkuzmenok requested review from katbyte and removed request for magodo October 17, 2022 11:53
@katbyte katbyte self-assigned this Oct 17, 2022
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 @dkuzmenok ! looks good aside from a couple comments

internal/services/containers/container_group_resource.go Outdated Show resolved Hide resolved
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 @dkuzmenok - LGTM 🍄

@katbyte katbyte merged commit c2fceae into hashicorp:main Oct 21, 2022
@github-actions github-actions bot added this to the v3.28.0 milestone Oct 21, 2022
katbyte added a commit that referenced this pull request Oct 21, 2022
@github-actions
Copy link

This functionality has been released in v3.28.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 Nov 24, 2022
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 wiht subnet (network profile is being deprecated)
4 participants