Updates and bug fixes to azurerm_cosmosdb_account #1055

katbyte · 2018-04-02T15:33:38Z

reviewed and improved azurerm_cosmosdb_account:

Added new field enable_automatic_failover
Now exports endpoint, read_enpoints and write_endpoints
failover_policy has been renamed to geo_locations to better match the API & portal terms
replication locations can now be renamed and have their failover_priority change (except for the primary location)
properties can now be changed at the same time replication locations are added
replication location ID's can now be set via the prefix attribute customizing their endpoints
consistency_policy is now a list so its attributes can be referenced
added additional input validation
now supports ConsistentPrefix consistency level

Also added more comprehensive tests and an cosmos-db example

…locations

tombuildsstuff

a few (mostly minor) comments - but this is looking good :)

tombuildsstuff · 2018-04-05T09:53:35Z

azurerm/resource_arm_cosmos_db_account.go

 						},

 						"max_staleness_prefix": {
 							Type:         schema.TypeInt,
 							Optional:     true,
 							Default:      100,
-							ValidateFunc: validation.IntBetween(1, 2147483647),
+							ValidateFunc: validation.IntBetween(10, 1000000),


is there some context for this value change? the older values came from either the Portal or the API Docs, but IIRC where there are different values for DocumentDB and CosmosDB?

I'm not sure where it came from! 2147483647 is the maximum value the API returns.

tombuildsstuff · 2018-04-05T15:46:01Z

azurerm/resource_arm_cosmos_db_account.go

-				Required: true,
+				Type:          schema.TypeSet,
+				Optional:      true,
+				Deprecated:    "This field has been renamed to 'geo_location' to better match the SDK/API",


I'd suggest this becomes "This field has been renamed to 'geo_location' to match Azure's usage"

tombuildsstuff · 2018-04-05T15:48:11Z

azurerm/resource_arm_cosmos_db_account.go

+		}
+	} else {
+		//could be a CustomizeDiff?, but this is temporary
+		return fmt.Errorf("Neither `geo_location` or `failover_policy` is set for CosmosDB Account '%s'", name)


I think this is fine providing it's called out in the docs

tombuildsstuff · 2018-04-05T16:03:36Z

azurerm/resource_arm_cosmos_db_account.go

+		flattenAndSetAzureRmCosmosDBAccountFailoverPolicy(d, resp.FailoverPolicies)
+	} else {
+		//if failover policy isn't default to using geo_location
+		if err := flattenAndSetAzureRmCosmosDBAccountGeoLocations(d, resp); err != nil {


can we make this flatten and have the Set handled here? Unfortunately tags is a bad example that should be refactored at some point tbh

tombuildsstuff · 2018-04-05T16:03:37Z

azurerm/resource_arm_cosmos_db_account.go

 	flattenAndSetAzureRmCosmosDBAccountConsistencyPolicy(d, resp.ConsistencyPolicy)
-	flattenAndSetAzureRmCosmosDBAccountFailoverPolicy(d, resp.FailoverPolicies)
+	if _, ok := d.GetOk("failover_policy"); ok {
+		flattenAndSetAzureRmCosmosDBAccountFailoverPolicy(d, resp.FailoverPolicies)


can we make this flatten and have the Set handled here? Unfortunately tags is a bad example that should be refactored at some point tbh

tombuildsstuff · 2018-04-05T16:22:52Z

azurerm/resource_arm_cosmos_db_account.go


-	buf.WriteString(fmt.Sprintf("%s-%d-%d", consistencyLevel, maxInterval, maxStalenessPrefix))
+	buf.WriteString(fmt.Sprintf("-%s-%d", location, priority))


if we're changing the hash value I think we'll need a state migration here?

Good spot, the - got in there by accident.

tombuildsstuff · 2018-04-05T16:30:01Z

azurerm/resource_arm_cosmos_db_account.go

@@ -65,8 +73,13 @@ func resourceArmCosmosDBAccount() *schema.Resource {
 				Optional: true,
 			},

+			"enable_automatic_failover": {
+				Type:     schema.TypeBool,
+				Optional: true,


since this'll be false if it's not specified, should we default this to a value or make it required?

When created via the portal its not required and seems to default to false.

tombuildsstuff · 2018-04-05T16:30:41Z

website/docs/r/cosmosdb_account.html.markdown

-* `max_staleness_prefix` - (Optional) When used with the Bounded Staleness consistency level, this value represents the number of stale requests tolerated. Accepted range for this value is 1 – 2,147,483,647. Defaults to `100`. Required when `consistency_level` is set to `BoundedStaleness`.
+* `consistency_level` - (Required) The Consistency Level to use for this CosmosDB Account - can be either `BoundedStaleness`, `Eventual`, `Session`, `Strong` or `ConsistentPrefix`.
+* `max_interval_in_seconds` - (Optional) When used with the Bounded Staleness consistency level, this value represents the time amount of staleness (in seconds) tolerated. Accepted range for this value is 5 - 86400 (1 day). Defaults to `5`. Required when `consistency_level` is set to `BoundedStaleness`.
+* `max_staleness_prefix` - (Optional) When used with the Bounded Staleness consistency level, this value represents the number of stale requests tolerated. Accepted range for this value is 10 – 1000000. Defaults to `100`. Required when `consistency_level` is set to `BoundedStaleness`.


minor can we quote these values?

tombuildsstuff · 2018-04-05T16:31:55Z

website/docs/r/cosmosdb_account.html.markdown

 * `location` - (Required) The name of the Azure region to host replicated data.
-* `priority` - (Required) The failover priority of the region. A failover priority of 0 indicates a write region. The maximum value for a failover priority = (total number of regions - 1). Failover priority values must be unique for each of the regions in which the database account exists.
+* `priority` - (Required) The failover priority of the region. A failover priority of 0 indicates a write region. The maximum value for a failover priority = (total number of regions - 1). Failover priority values must be unique for each of the regions in which the database account exists. Changing this causes the location to be re-provisioned and cannot be changed for the location with failover priority 0.


cannot be changed for the location with failover priority 0. may make sense as an info box? e.g. -> **NOTE:** cannot be changed for the location with failover priority 0.

tombuildsstuff · 2018-04-05T16:32:32Z

website/docs/r/cosmosdb_account.html.markdown

+
+* `read_endpoints` - A list of read endpoints available for this CosmosDB account.
+
+* `write_endpoints` - A list of write endpoints available for this CosmosDB account.


if there's only one of these should we make this a string?

The API returns an array of locations, not sure if there will ever be more then one however.

…dates

tombuildsstuff

I've left a couple of minor things (which I'll push a commit to fix) - but this otherwise LGTM 👍

Given this includes a deprecation, let's merge this once 1.3.3 is released?

tombuildsstuff · 2018-04-09T18:02:48Z

azurerm/resource_arm_cosmos_db_account.go

 	if err != nil {
-		return err
+		return fmt.Errorf("Error creating/updating CosmosDB Account %q (Resource Group %q): %+v", name, resourceGroup, err)


minor technically this is only creating, but it's fine

tombuildsstuff · 2018-04-09T18:09:33Z

azurerm/resource_arm_cosmos_db_account.go

-	if location := resp.Location; location != nil {
-		d.Set("location", azureRMNormalizeLocation(*location))
-	}
+	d.Set("location", azureRMNormalizeLocation(*resp.Location))


can we revert this?

…_location on read, resulted in endless plans

katbyte · 2018-04-17T02:23:20Z

Thanks!

Running tests against the proper changeset revealed some regressions: flattenAndSetTags() was lost in the merge and if we set both failover_policy and geo_location all tests fail due to never ending plans. Reverted both these and re-ran the acceptance tests.

katbyte · 2018-04-17T08:45:09Z

Tests pass:

tombuildsstuff · 2018-04-17T09:54:14Z

azurerm/resource_arm_cosmos_db_account.go

+		d.Set("failover_policy", flattenAzureRmCosmosDBAccountFailoverPolicy(resp.FailoverPolicies))
+	} else {
+		//if failover policy isn't default to using geo_location
+		d.Set("geo_location", flattenAzureRmCosmosDBAccountGeoLocations(d, resp))


I'll push a commit to raise the errors if there's a failure here - this otherwise LGTM 👍

tombuildsstuff · 2018-04-17T09:54:32Z

azurerm/resource_arm_cosmos_db_account.go

+		d.Set("failover_policy", flattenAzureRmCosmosDBAccountFailoverPolicy(resp.FailoverPolicies))
+	} else {
+		//if failover policy isn't default to using geo_location
+		d.Set("geo_location", flattenAzureRmCosmosDBAccountGeoLocations(d, resp))
 	}


I'll push a commit to raise the errors if there's a failure here - this otherwise LGTM 👍

…iess

tombuildsstuff

LGTM 👍

ghost · 2020-03-31T15:09:33Z

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. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

katbyte added 4 commits March 23, 2018 00:06

Intial commit of updates to azurerm_cosmosdb_account resource

bac8d43

Updates to properly wait for adding/removing locations to be compleated

acbabe6

Updated cosmosdb_account resource to rename/reprioritize non-primary …

8e89585

…locations

Fixed failing azurerm_cosmosdb_account import tests

070d726

katbyte mentioned this pull request Apr 2, 2018

New Data Source: azurerm_cosmosdb_account #1056

Merged

katbyte added bug enhancement service/cosmosdb labels Apr 2, 2018

katbyte added this to the 1.3.2 milestone Apr 2, 2018

azurerm_cosmosdb_account minor updates and fixes

4c6693b

katbyte requested a review from tombuildsstuff April 2, 2018 21:45

Fixed broken azurerm_cosmosdb_account import test

1723eeb

tombuildsstuff modified the milestones: 1.3.2, 1.3.3 Apr 4, 2018

Small change to azurerm_cosmosdb_account tests

0e7e265

tombuildsstuff requested changes Apr 5, 2018

View reviewed changes

katbyte and others added 3 commits April 5, 2018 16:14

Updated cosmosdb_account with requested changes.

1b176db

Merge remote-tracking branch 'origin/master' into cosmosdb_account_up…

95c2851

…dates

resGroup -> resourceGroup to fix the build

f3c0153

tombuildsstuff approved these changes Apr 16, 2018

View reviewed changes

tombuildsstuff and others added 3 commits April 16, 2018 19:16

Fixing some minor nits

435ad0c

Reverted flattenAndSetTags merge regression

3c666fd

azurerm_cosmosdb_account: revert setting both failover_policy and geo…

058aefd

…_location on read, resulted in endless plans

katbyte modified the milestones: 1.3.3, 1.4.0 Apr 17, 2018

tombuildsstuff reviewed Apr 17, 2018

View reviewed changes

Raising errors picked up when flattening geo_locations/failover_polic…

036e934

…iess

tombuildsstuff approved these changes Apr 17, 2018

View reviewed changes

tombuildsstuff modified the milestones: 1.4.0, Temp/To Be Sorted Apr 17, 2018

tombuildsstuff merged commit 8f474d4 into master Apr 17, 2018

tombuildsstuff deleted the cosmosdb_account_updates branch April 17, 2018 14:12

tombuildsstuff added a commit that referenced this pull request Apr 17, 2018

Updating to include #1055

9c73030

mikhailshilkov mentioned this pull request Oct 10, 2019

In-place update of Cosmos DB geo_location #4315

Closed

hashicorp locked and limited conversation to collaborators Mar 31, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updates and bug fixes to azurerm_cosmosdb_account #1055

Updates and bug fixes to azurerm_cosmosdb_account #1055

katbyte commented Apr 2, 2018 •

edited

tombuildsstuff left a comment

tombuildsstuff Apr 5, 2018

katbyte Apr 5, 2018

tombuildsstuff Apr 5, 2018

tombuildsstuff Apr 5, 2018

tombuildsstuff Apr 5, 2018

tombuildsstuff Apr 5, 2018

tombuildsstuff Apr 5, 2018

katbyte Apr 5, 2018

tombuildsstuff Apr 5, 2018

katbyte Apr 5, 2018

tombuildsstuff Apr 5, 2018

tombuildsstuff Apr 5, 2018

tombuildsstuff Apr 5, 2018

katbyte Apr 5, 2018

tombuildsstuff left a comment

tombuildsstuff Apr 9, 2018

tombuildsstuff Apr 9, 2018

katbyte commented Apr 17, 2018

katbyte commented Apr 17, 2018

tombuildsstuff Apr 17, 2018

tombuildsstuff Apr 17, 2018

tombuildsstuff left a comment

ghost commented Mar 31, 2020


		buf.WriteString(fmt.Sprintf("%s-%d-%d", consistencyLevel, maxInterval, maxStalenessPrefix))
		buf.WriteString(fmt.Sprintf("-%s-%d", location, priority))


		* `read_endpoints` - A list of read endpoints available for this CosmosDB account.

		* `write_endpoints` - A list of write endpoints available for this CosmosDB account.

Updates and bug fixes to azurerm_cosmosdb_account #1055

Updates and bug fixes to azurerm_cosmosdb_account #1055

Conversation

katbyte commented Apr 2, 2018 • edited

tombuildsstuff 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

katbyte commented Apr 17, 2018

katbyte commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tombuildsstuff left a comment

Choose a reason for hiding this comment

ghost commented Mar 31, 2020

katbyte commented Apr 2, 2018 •

edited