From c77fdad523fa33edf5e1349925f8a2fc51999aa4 Mon Sep 17 00:00:00 2001 From: ms-henglu Date: Wed, 14 Apr 2021 11:33:07 +0800 Subject: [PATCH] update tests and fix a bug --- .../containers/container_registry_resource.go | 74 +++------- .../container_registry_resource_test.go | 137 +++++++++++++++--- .../docs/r/container_registry.html.markdown | 2 +- 3 files changed, 138 insertions(+), 75 deletions(-) diff --git a/azurerm/internal/services/containers/container_registry_resource.go b/azurerm/internal/services/containers/container_registry_resource.go index 5cd6c1d11cc5..79f4191bc5b0 100644 --- a/azurerm/internal/services/containers/container_registry_resource.go +++ b/azurerm/internal/services/containers/container_registry_resource.go @@ -90,7 +90,7 @@ func resourceContainerRegistry() *schema.Resource { Optional: true, Computed: true, // TODO -- remove this when deprecation resolves ConflictsWith: []string{"georeplication_locations"}, - ConfigMode: schema.SchemaConfigModeAttr, + ConfigMode: schema.SchemaConfigModeAttr, // TODO -- remove in 3.0, because this property is optional and computed, it has to be declared as empty array to remove existed values Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "location": { @@ -364,20 +364,16 @@ func resourceContainerRegistryCreate(d *schema.ResourceData, meta interface{}) e return fmt.Errorf("Error waiting for creation of Container Registry %q (Resource Group %q): %+v", name, resourceGroup, err) } - // locations have been specified for geo-replication + // the ACR is being created so no previous geo-replication locations + var oldGeoReplicationLocations, newGeoReplicationLocations []*containerregistry.Replication if geoReplicationLocations != nil && geoReplicationLocations.Len() > 0 { - // the ACR is being created so no previous geo-replication locations - 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) - } + newGeoReplicationLocations = expandReplicationsFromLocations(geoReplicationLocations.List()) + } else { + newGeoReplicationLocations = expandReplications(geoReplications) } // 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 len(newGeoReplicationLocations) > 0 { + err = applyGeoReplicationLocations(d, meta, resourceGroup, name, oldGeoReplicationLocations, newGeoReplicationLocations) if err != nil { return fmt.Errorf("Error applying geo replications for Container Registry %q (Resource Group %q): %+v", name, resourceGroup, err) } @@ -530,65 +526,41 @@ func applyContainerRegistrySku(d *schema.ResourceData, meta interface{}, sku str return nil } -func applyGeoReplicationLocations(d *schema.ResourceData, meta interface{}, resourceGroup string, name string, oldGeoReplicationLocations []*containerregistry.Replication, newGeoReplicationLocations []*containerregistry.Replication) error { +func applyGeoReplicationLocations(d *schema.ResourceData, meta interface{}, resourceGroup string, name string, oldGeoReplications []*containerregistry.Replication, newGeoReplications []*containerregistry.Replication) error { replicationClient := meta.(*clients.Client).Containers.ReplicationsClient ctx, cancel := timeouts.ForCreateUpdate(meta.(*clients.Client).StopContext, d) defer cancel() log.Printf("[INFO] preparing to apply geo-replications for Container Registry.") - createReplications := make(map[string]*containerregistry.Replication) - - // loop on the new location values - for _, nl := range newGeoReplicationLocations { - if nl == nil { + // delete previously deployed locations + for _, replication := range oldGeoReplications { + if replication == nil { continue } - newLocation := azure.NormalizeLocation(*nl.Location) - createReplications[newLocation] = nl // the replication needs to be created - } + oldLocation := azure.NormalizeLocation(*replication.Location) - // loop on the old location values - for _, ol := range oldGeoReplicationLocations { - if ol == nil { - continue - } - // oldLocation was created from a previous deployment - oldLocation := azure.NormalizeLocation(*ol.Location) - - delete(createReplications, oldLocation) // the location do not need to be created, it already exists - } - - // create new geo-replication locations - for locationToCreate, replication := range createReplications { - future, err := replicationClient.Create(ctx, resourceGroup, name, locationToCreate, *replication) + future, err := replicationClient.Delete(ctx, resourceGroup, name, oldLocation) if err != nil { - return fmt.Errorf("Error creating Container Registry Replication %q (Resource Group %q, Location %q): %+v", name, resourceGroup, locationToCreate, err) + return fmt.Errorf("Error deleting Container Registry Replication %q (Resource Group %q, Location %q): %+v", name, resourceGroup, oldLocation, err) } - if err = future.WaitForCompletionRef(ctx, replicationClient.Client); err != nil { - return fmt.Errorf("Error waiting for creation of Container Registry Replication %q (Resource Group %q, Location %q): %+v", name, resourceGroup, locationToCreate, err) + return fmt.Errorf("Error waiting for deletion of Container Registry Replication %q (Resource Group %q, Location %q): %+v", name, resourceGroup, oldLocation, err) } } - // loop on the list of previously deployed locations - for _, ol := range oldGeoReplicationLocations { - if ol == nil { - continue - } - oldLocation := azure.NormalizeLocation(*ol.Location) - // if the old location is still in the list of locations, then continue - if _, ok := createReplications[oldLocation]; ok { + // create new geo-replication locations + for _, replication := range newGeoReplications { + if replication == nil { continue } - - // the old location is not in the list of locations, delete it - future, err := replicationClient.Delete(ctx, resourceGroup, name, oldLocation) + locationToCreate := azure.NormalizeLocation(*replication.Location) + future, err := replicationClient.Create(ctx, resourceGroup, name, locationToCreate, *replication) if err != nil { - return fmt.Errorf("Error deleting Container Registry Replication %q (Resource Group %q, Location %q): %+v", name, resourceGroup, oldLocation, err) + return fmt.Errorf("Error creating Container Registry Replication %q (Resource Group %q, Location %q): %+v", name, resourceGroup, locationToCreate, err) } if err = future.WaitForCompletionRef(ctx, replicationClient.Client); err != nil { - return fmt.Errorf("Error waiting for deletion of Container Registry Replication %q (Resource Group %q, Location %q): %+v", name, resourceGroup, oldLocation, err) + return fmt.Errorf("Error waiting for creation of Container Registry Replication %q (Resource Group %q, Location %q): %+v", name, resourceGroup, locationToCreate, err) } } diff --git a/azurerm/internal/services/containers/container_registry_resource_test.go b/azurerm/internal/services/containers/container_registry_resource_test.go index b9179dbccab4..ec3e55066cff 100644 --- a/azurerm/internal/services/containers/container_registry_resource_test.go +++ b/azurerm/internal/services/containers/container_registry_resource_test.go @@ -217,7 +217,7 @@ func TestAccContainerRegistry_geoReplicationLocation(t *testing.T) { data.ResourceTest(t, r, []resource.TestStep{ // first config creates an ACR with locations { - Config: r.geoReplicationLocation(data, skuPremium, []string{secondaryLocation}), + Config: r.geoReplicationLocation(data, []string{secondaryLocation}), Check: resource.ComposeTestCheckFunc( check.That(data.ResourceName).ExistsInAzure(r), check.That(data.ResourceName).Key("sku").HasValue(skuPremium), @@ -227,7 +227,7 @@ func TestAccContainerRegistry_geoReplicationLocation(t *testing.T) { }, // second config updates the ACR with updated locations { - Config: r.geoReplicationLocation(data, skuPremium, []string{ternaryLocation}), + Config: r.geoReplicationLocation(data, []string{ternaryLocation}), Check: resource.ComposeTestCheckFunc( check.That(data.ResourceName).ExistsInAzure(r), check.That(data.ResourceName).Key("sku").HasValue(skuPremium), @@ -235,17 +235,27 @@ func TestAccContainerRegistry_geoReplicationLocation(t *testing.T) { check.That(data.ResourceName).Key("georeplication_locations.0").HasValue(ternaryLocation), ), }, + // third config updates the ACR with updated locations + { + Config: r.geoReplicationLocation(data, []string{secondaryLocation, ternaryLocation}), + Check: resource.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + check.That(data.ResourceName).Key("sku").HasValue(skuPremium), + check.That(data.ResourceName).Key("georeplication_locations.#").HasValue("2"), + ), + }, + data.ImportStep(), // For compatibility, downgrade from Premium to Basic should remove all replications first, but it's unnecessary. Once georeplication_locations is deprecated, this can be done in single update. - // third config updates the ACR with no location. + // fourth config updates the ACR with no location. { - Config: r.geoReplicationUpdateWithNoLocation(data, skuPremium), + Config: r.geoReplicationUpdateWithNoLocation(data), Check: resource.ComposeTestCheckFunc( check.That(data.ResourceName).ExistsInAzure(r), check.That(data.ResourceName).Key("sku").HasValue(skuPremium), check.That(data.ResourceName).Key("georeplication_locations.#").HasValue("0"), ), }, - // fourth config updates the SKU to basic. + // fifth config updates the SKU to basic. { Config: r.geoReplicationUpdateWithNoLocation_basic(data), Check: resource.ComposeTestCheckFunc( @@ -270,30 +280,48 @@ func TestAccContainerRegistry_geoReplication(t *testing.T) { data.ResourceTest(t, r, []resource.TestStep{ // first config creates an ACR with locations { - Config: r.geoReplication(data, skuPremium, secondaryLocation), + Config: r.geoReplication(data, secondaryLocation), Check: resource.ComposeTestCheckFunc( check.That(data.ResourceName).ExistsInAzure(r), check.That(data.ResourceName).Key("sku").HasValue(skuPremium), check.That(data.ResourceName).Key("georeplications.#").HasValue("1"), check.That(data.ResourceName).Key("georeplications.0.location").HasValue(secondaryLocation), + check.That(data.ResourceName).Key("georeplications.0.tags.%").HasValue("1"), + check.That(data.ResourceName).Key("georeplications.0.tags.Environment").HasValue("Production"), ), }, data.ImportStep(), // second config updates the ACR with updated locations { - Config: r.geoReplication(data, skuPremium, ternaryLocation), + Config: r.geoReplication(data, ternaryLocation), Check: resource.ComposeTestCheckFunc( check.That(data.ResourceName).ExistsInAzure(r), check.That(data.ResourceName).Key("sku").HasValue(skuPremium), check.That(data.ResourceName).Key("georeplications.#").HasValue("1"), check.That(data.ResourceName).Key("georeplications.0.location").HasValue(ternaryLocation), + check.That(data.ResourceName).Key("georeplications.0.tags.%").HasValue("1"), + check.That(data.ResourceName).Key("georeplications.0.tags.Environment").HasValue("Production"), + ), + }, + data.ImportStep(), + // third config updates the ACR with updated locations + { + Config: r.geoReplicationMultipleLocations(data, secondaryLocation, ternaryLocation), + Check: resource.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + check.That(data.ResourceName).Key("sku").HasValue(skuPremium), + check.That(data.ResourceName).Key("georeplications.#").HasValue("2"), + check.That(data.ResourceName).Key("georeplications.0.location").HasValue(secondaryLocation), + check.That(data.ResourceName).Key("georeplications.0.tags.%").HasValue("0"), + check.That(data.ResourceName).Key("georeplications.1.location").HasValue(ternaryLocation), + check.That(data.ResourceName).Key("georeplications.1.tags.%").HasValue("0"), ), }, data.ImportStep(), // For compatibility, downgrade from Premium to Basic should remove all replications first, but it's unnecessary. Once georeplication_locations is deprecated, this can be done in single update. - // third config updates the ACR with no location + // fourth config updates the ACR with no location { - Config: r.geoReplicationUpdateWithNoReplication(data, skuPremium), + Config: r.geoReplicationUpdateWithNoReplication(data), Check: resource.ComposeTestCheckFunc( check.That(data.ResourceName).ExistsInAzure(r), check.That(data.ResourceName).Key("sku").HasValue(skuPremium), @@ -301,7 +329,7 @@ func TestAccContainerRegistry_geoReplication(t *testing.T) { ), }, data.ImportStep(), - // fourth config updates the SKU to basic. + // fifth config updates the SKU to basic. { Config: r.geoReplicationUpdateWithNoReplication_basic(data), Check: resource.ComposeTestCheckFunc( @@ -313,6 +341,43 @@ func TestAccContainerRegistry_geoReplication(t *testing.T) { }) } +func TestAccContainerRegistry_geoReplicationSwitch(t *testing.T) { + data := acceptance.BuildTestData(t, "azurerm_container_registry", "test") + r := ContainerRegistryResource{} + + skuPremium := "Premium" + + secondaryLocation := location.Normalize(data.Locations.Secondary) + ternaryLocation := location.Normalize(data.Locations.Ternary) + + data.ResourceTest(t, r, []resource.TestStep{ + // first config creates an ACR using georeplication_locations + { + Config: r.geoReplicationLocation(data, []string{secondaryLocation, ternaryLocation}), + Check: resource.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + check.That(data.ResourceName).Key("sku").HasValue(skuPremium), + check.That(data.ResourceName).Key("georeplication_locations.#").HasValue("2"), + ), + }, + data.ImportStep(), + // second config updates the ACR using georeplications + { + Config: r.geoReplicationMultipleLocations(data, secondaryLocation, ternaryLocation), + Check: resource.ComposeTestCheckFunc( + check.That(data.ResourceName).ExistsInAzure(r), + check.That(data.ResourceName).Key("sku").HasValue(skuPremium), + check.That(data.ResourceName).Key("georeplications.#").HasValue("2"), + check.That(data.ResourceName).Key("georeplications.0.location").HasValue(secondaryLocation), + check.That(data.ResourceName).Key("georeplications.0.tags.%").HasValue("0"), + check.That(data.ResourceName).Key("georeplications.1.location").HasValue(ternaryLocation), + check.That(data.ResourceName).Key("georeplications.1.tags.%").HasValue("0"), + ), + }, + data.ImportStep(), + }) +} + func TestAccContainerRegistry_networkAccessProfileIp(t *testing.T) { data := acceptance.BuildTestData(t, "azurerm_container_registry", "test") r := ContainerRegistryResource{} @@ -552,7 +617,7 @@ resource "azurerm_container_registry" "test" { `, data.RandomInteger, data.Locations.Primary, data.RandomInteger) } -func (ContainerRegistryResource) geoReplicationLocation(data acceptance.TestData, sku string, replicationRegions []string) string { +func (ContainerRegistryResource) geoReplicationLocation(data acceptance.TestData, replicationRegions []string) string { regions := make([]string, 0) for _, region := range replicationRegions { // ensure they're quoted @@ -572,13 +637,13 @@ resource "azurerm_container_registry" "test" { name = "testacccr%d" resource_group_name = azurerm_resource_group.test.name location = azurerm_resource_group.test.location - sku = "%s" + sku = "Premium" georeplication_locations = [%s] } -`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, sku, strings.Join(regions, ",")) +`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, strings.Join(regions, ",")) } -func (ContainerRegistryResource) geoReplication(data acceptance.TestData, sku string, replication string) string { +func (ContainerRegistryResource) geoReplication(data acceptance.TestData, replication string) string { return fmt.Sprintf(` provider "azurerm" { features {} @@ -593,18 +658,44 @@ resource "azurerm_container_registry" "test" { name = "testacccr%d" resource_group_name = azurerm_resource_group.test.name location = azurerm_resource_group.test.location - sku = "%s" + sku = "Premium" georeplications { - location = %s + location = "%s" tags = { Environment = "Production" } } } -`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, sku, fmt.Sprintf("%q", replication)) +`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, replication) +} + +func (ContainerRegistryResource) geoReplicationMultipleLocations(data acceptance.TestData, primaryLocation string, secondaryLocation string) string { + return fmt.Sprintf(` +provider "azurerm" { + features {} +} + +resource "azurerm_resource_group" "test" { + name = "acctestRG-aks-%d" + location = "%s" +} + +resource "azurerm_container_registry" "test" { + name = "testacccr%d" + resource_group_name = azurerm_resource_group.test.name + location = azurerm_resource_group.test.location + sku = "Premium" + georeplications { + location = "%s" + } + georeplications { + location = "%s" + } +} +`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, primaryLocation, secondaryLocation) } -func (ContainerRegistryResource) geoReplicationUpdateWithNoLocation(data acceptance.TestData, sku string) string { +func (ContainerRegistryResource) geoReplicationUpdateWithNoLocation(data acceptance.TestData) string { return fmt.Sprintf(` provider "azurerm" { features {} @@ -619,13 +710,13 @@ resource "azurerm_container_registry" "test" { name = "testacccr%d" resource_group_name = azurerm_resource_group.test.name location = azurerm_resource_group.test.location - sku = "%s" + sku = "Premium" georeplication_locations = [] } -`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, sku) +`, data.RandomInteger, data.Locations.Primary, data.RandomInteger) } -func (ContainerRegistryResource) geoReplicationUpdateWithNoReplication(data acceptance.TestData, sku string) string { +func (ContainerRegistryResource) geoReplicationUpdateWithNoReplication(data acceptance.TestData) string { return fmt.Sprintf(` provider "azurerm" { features {} @@ -640,10 +731,10 @@ resource "azurerm_container_registry" "test" { name = "testacccr%d" resource_group_name = azurerm_resource_group.test.name location = azurerm_resource_group.test.location - sku = "%s" + sku = "Premium" georeplications = [] } -`, data.RandomInteger, data.Locations.Primary, data.RandomInteger, sku) +`, data.RandomInteger, data.Locations.Primary, data.RandomInteger) } func (ContainerRegistryResource) geoReplicationUpdateWithNoLocation_basic(data acceptance.TestData) string { return fmt.Sprintf(` diff --git a/website/docs/r/container_registry.html.markdown b/website/docs/r/container_registry.html.markdown index ddb2dcf65ca4..18dc34232616 100644 --- a/website/docs/r/container_registry.html.markdown +++ b/website/docs/r/container_registry.html.markdown @@ -52,7 +52,7 @@ The following arguments are supported: * `tags` - (Optional) A mapping of tags to assign to the resource. -* `georeplication_locations` - (Optional) A list of Azure locations where the container registry should be geo-replicated. +* `georeplication_locations` - (Optional / **Deprecated in favor of `georeplications`**) A list of Azure locations where the container registry should be geo-replicated. ~> **NOTE:** The `georeplication_locations` is only supported on new resources with the `Premium` SKU.