azurerm_container_registry - deprecating the georeplication_locations property in favour of the georeplications property #11200

ms-henglu · 2021-04-02T06:00:51Z

The test results are listed as follows.

=== RUN   TestAccContainerRegistry_geoReplication
=== PAUSE TestAccContainerRegistry_geoReplication
=== CONT  TestAccContainerRegistry_geoReplication
--- PASS: TestAccContainerRegistry_geoReplication (501.49s)

=== RUN   TestAccContainerRegistry_geoReplicationLocation
=== PAUSE TestAccContainerRegistry_geoReplicationLocation
=== CONT  TestAccContainerRegistry_geoReplicationLocation
--- PASS: TestAccContainerRegistry_geoReplicationLocation (519.45s)

=== RUN   TestAccContainerRegistry_geoReplicationSwitch
=== PAUSE TestAccContainerRegistry_geoReplicationSwitch
=== CONT  TestAccContainerRegistry_geoReplicationSwitch
--- PASS: TestAccContainerRegistry_geoReplicationSwitch (418.32s)

ArcturusZhang

Hi @ms-henglu thank you for opening this PR!

This basically looks good to me 👍 aside from some inline comments and some recommendations on test case enhancement, please take a look, thanks

ArcturusZhang · 2021-04-13T06:38:51Z

azurerm/internal/services/containers/container_registry_resource.go

+				Optional:      true,
+				Computed:      true, // TODO -- remove this when deprecation resolves
+				ConflictsWith: []string{"georeplication_locations"},
+				ConfigMode:    schema.SchemaConfigModeAttr,


Could we add some comments to describe the reason for this attribute here?
And also this also need a TODO -- remove in 3.0 comment.

ArcturusZhang · 2021-04-13T06:41:36Z

azurerm/internal/services/containers/container_registry_resource.go

 				Elem: &schema.Schema{
 					Type:         schema.TypeString,
 					ValidateFunc: validation.StringIsNotEmpty,
 				},
 				Set: location.HashCode,
 			},

+			"georeplications": {
+				Type:          schema.TypeList,


Do we also need this to be a Set considering that we are using Set for the attribute georeplication_locations which we are replacing with this new block? Or List could meet the requirement here, the Set for georeplication_locations is an overkill?
To determine this, would the service accept an input with two elements with the same location but different tags?
Or would the service always return the response in the same order as the request?

I think List is enough, since service always return the response in the same order as the request.

ref: https://github.com/microsoft/terraform-provider-azurerm/wiki/R-1006

Another reason to use list, is that there's an issue about optional-computed set, state won't be updated after applying, unless refresh.
hashicorp/terraform-plugin-sdk#618

If order doesn't matter this should be a set

I'll change it to set.

ArcturusZhang · 2021-04-13T06:50:05Z

azurerm/internal/services/containers/container_registry_resource.go

 	if geoReplicationLocations != nil && geoReplicationLocations.Len() > 0 {
 		// the ACR is being created so no previous geo-replication locations
-		oldGeoReplicationLocations := []interface{}{}
-		err = applyGeoReplicationLocations(d, meta, resourceGroup, name, oldGeoReplicationLocations, geoReplicationLocations.List())
+		var oldGeoReplicationLocations []*containerregistry.Replication
+		err = applyGeoReplicationLocations(d, meta, resourceGroup, name, oldGeoReplicationLocations, expandReplicationsFromLocations(geoReplicationLocations.List()))
+		if err != nil {
+			return fmt.Errorf("Error applying geo replications for Container Registry %q (Resource Group %q): %+v", name, resourceGroup, err)
+		}
+	}
+	// geo replications have been specified
+	if len(geoReplications) > 0 {
+		// the ACR is being created so no previous geo-replication locations
+		var oldGeoReplicationLocations []*containerregistry.Replication
+		err = applyGeoReplicationLocations(d, meta, resourceGroup, name, oldGeoReplicationLocations, expandReplications(geoReplications))
 		if err != nil {
 			return fmt.Errorf("Error applying geo replications for Container Registry %q (Resource Group %q): %+v", name, resourceGroup, err)
 		}


This part looks a bit redundant - could we combine these two if's by saying first determine the newGeoReplicationLocations and then call the applyGeoReplicationLocations to apply them? It helps to make things more clear here

ArcturusZhang · 2021-04-13T06:59:27Z

azurerm/internal/services/containers/container_registry_resource_test.go

 			Check: resource.ComposeTestCheckFunc(
 				check.That(data.ResourceName).ExistsInAzure(r),
 				check.That(data.ResourceName).Key("sku").HasValue(skuBasic),
-				check.That(data.ResourceName).Key("georeplication_locations.#").HasValue("0"),
+				check.That(data.ResourceName).Key("georeplications.#").HasValue("0"),
 			),
 		},
 	})


Is it possible that we could introduce a update case to update only the tags propertyin thegeoReplicationLocation? also could we add a case with multiple location entries with the corresponding update cases? Finally to make sure this would not break our existing customer - could we add a test case that ensures if we switch the config written using georeplication_locationsto a config written usinggeoreplications` will give us no-diff?

Thanks for the advice.
I updated the tests to test

tags

multiple locations

switch from georeplication_locations to georeplications

ArcturusZhang · 2021-04-13T07:03:52Z

website/docs/r/container_registry.html.markdown

@@ -58,6 +58,14 @@ The following arguments are supported:

 ~> **NOTE:** The `georeplication_locations` list cannot contain the location where the Container Registry exists.


COuld we also add the deprecation information to follow the Optional? Like this:

georeplication_locations - (Optional / Deprecated in favor of georeplications) A list of Azure locations where the container registry should be geo-replicated.

also can just remove it, but either works

website/docs/r/container_registry.html.markdown

ms-henglu · 2021-04-14T04:51:48Z

@ArcturusZhang hi, I have added a commit to add more tests and fix some issues. Please review again, thanks!

ms-henglu · 2021-04-16T04:05:48Z

@katbyte hi, I change the type to set, please check again, thanks!

ArcturusZhang · 2021-04-20T00:02:24Z

Hey @ms-henglu looks like we are having conflicts in this PR now, could you please resolve the conflict?

ms-henglu · 2021-04-20T02:17:39Z

@ArcturusZhang hi, thanks! I have resolved the conflicts by removing latest commit go mod vendor

katbyte

Aside from a couple minor comments this LGTM 👍

katbyte · 2021-04-20T04:30:11Z

azurerm/internal/services/containers/container_registry_resource_test.go

+}
+
+resource "azurerm_resource_group" "test" {
+  name     = "acctestRG-aks-%d"


this isn't aks?

Suggested change

name = "acctestRG-aks-%d"

name = "acctestRG-acr-%d"

katbyte · 2021-04-20T04:30:26Z

website/docs/r/container_registry.html.markdown

@@ -58,6 +58,14 @@ The following arguments are supported:

 ~> **NOTE:** The `georeplication_locations` list cannot contain the location where the Container Registry exists.


also can just remove it, but either works

katbyte · 2021-04-20T04:30:52Z

website/docs/r/container_registry.html.markdown

+
+* `location` - (Required) A location where the container registry should be geo-replicated.
+
+* `tags` - (Optional) A mapping of tags to assign to the container registry replication resource.


phrasing

Suggested change

* `tags` - (Optional) A mapping of tags to assign to the container registry replication resource.

* `tags` - (Optional) A mapping of tags to assign to this replication location.

katbyte · 2021-04-20T04:33:24Z

azurerm/internal/services/containers/container_registry_resource_test.go

+}
+
+resource "azurerm_resource_group" "test" {
+  name     = "acctestRG-aks-%d"


Suggested change

name = "acctestRG-aks-%d"

name = "acctestRG-acr-%d"

katbyte · 2021-04-20T04:33:28Z

azurerm/internal/services/containers/container_registry_resource_test.go

+}
+
+resource "azurerm_resource_group" "test" {
+  name     = "acctestRG-aks-%d"


katbyte · 2021-04-20T04:33:32Z

azurerm/internal/services/containers/container_registry_resource_test.go

+}
+
+resource "azurerm_resource_group" "test" {
+  name     = "acctestRG-aks-%d"


ms-henglu · 2021-04-20T05:06:51Z

@katbyte ,hi, I add one commit to fix it, please check again, thanks!

ghost · 2021-04-30T02:18:45Z

This has been released in version 2.57.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example:

provider "azurerm" {
    version = "~> 2.57.0"
}
# ... other configuration ...

github-actions · 2021-05-30T02:43:55Z

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.

ghost added size/XL documentation labels Apr 2, 2021

katbyte added service/container-registry enhancement labels Apr 6, 2021

ArcturusZhang suggested changes Apr 13, 2021

View reviewed changes

ArcturusZhang reviewed Apr 14, 2021

View reviewed changes

website/docs/r/container_registry.html.markdown Outdated Show resolved Hide resolved

ms-henglu force-pushed the branch-00000-add-resource-container-registry-replication-new branch 2 times, most recently from d952835 to c77fdad Compare April 14, 2021 03:41

ms-henglu closed this Apr 16, 2021

ms-henglu reopened this Apr 16, 2021

ms-henglu force-pushed the branch-00000-add-resource-container-registry-replication-new branch 2 times, most recently from efdf3f1 to 93e39ab Compare April 16, 2021 03:47

ghost added the dependencies label Apr 16, 2021

ms-henglu requested review from ArcturusZhang and katbyte April 16, 2021 04:05

katbyte added this to the v2.57.0 milestone Apr 19, 2021

ms-henglu added 3 commits April 20, 2021 09:32

add container registry georeplications property

2a82a67

update tests and fix a bug

599bfc2

change "georeplications" type from list to set

e08fb47

ms-henglu force-pushed the branch-00000-add-resource-container-registry-replication-new branch from 93e39ab to e08fb47 Compare April 20, 2021 01:36

katbyte approved these changes Apr 20, 2021

View reviewed changes

fix

ea35876

katbyte changed the title ~~add container registry georeplications property~~ azurerm_container_registry - deprecating the georeplication_locations property in favour of the georeplications property Apr 20, 2021

katbyte merged commit 7888c96 into hashicorp:master Apr 20, 2021

katbyte added a commit that referenced this pull request Apr 20, 2021

CHANGELOG.md for #11200

fde6977

magodo mentioned this pull request May 14, 2021

azurerm_container_registry - Support for zone_redundancy_enabled for both the ACR and each georeplications block #11706

Merged

github-actions bot locked as resolved and limited conversation to collaborators May 30, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

azurerm_container_registry - deprecating the georeplication_locations property in favour of the georeplications property #11200

azurerm_container_registry - deprecating the georeplication_locations property in favour of the georeplications property #11200

ms-henglu commented Apr 2, 2021 •

edited

Loading

ArcturusZhang left a comment

ArcturusZhang Apr 13, 2021

ArcturusZhang Apr 13, 2021

ms-henglu Apr 13, 2021

ms-henglu Apr 13, 2021

katbyte Apr 15, 2021

ms-henglu Apr 16, 2021

ArcturusZhang Apr 13, 2021

ArcturusZhang Apr 13, 2021

ms-henglu Apr 14, 2021

ArcturusZhang Apr 13, 2021

katbyte Apr 20, 2021

ms-henglu commented Apr 14, 2021

ms-henglu commented Apr 16, 2021

ArcturusZhang commented Apr 20, 2021

ms-henglu commented Apr 20, 2021

katbyte left a comment

katbyte Apr 20, 2021

katbyte Apr 20, 2021

katbyte Apr 20, 2021

katbyte Apr 20, 2021

katbyte Apr 20, 2021

katbyte Apr 20, 2021

ms-henglu commented Apr 20, 2021

ghost commented Apr 30, 2021

github-actions bot commented May 30, 2021

		@@ -58,6 +58,14 @@ The following arguments are supported:

		~> NOTE: The `georeplication_locations` list cannot contain the location where the Container Registry exists.


		* `location` - (Required) A location where the container registry should be geo-replicated.

		* `tags` - (Optional) A mapping of tags to assign to the container registry replication resource.

azurerm_container_registry - deprecating the georeplication_locations property in favour of the georeplications property #11200

azurerm_container_registry - deprecating the georeplication_locations property in favour of the georeplications property #11200

Conversation

ms-henglu commented Apr 2, 2021 • edited Loading

ArcturusZhang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-henglu commented Apr 14, 2021

ms-henglu commented Apr 16, 2021

ArcturusZhang commented Apr 20, 2021

ms-henglu commented Apr 20, 2021

katbyte left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-henglu commented Apr 20, 2021

ghost commented Apr 30, 2021

github-actions bot commented May 30, 2021

ms-henglu commented Apr 2, 2021 •

edited

Loading