-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Upgrade Cosmos version from 2020-04-01-preview
to 2021-01-15
#10926
Conversation
2020-04-01-preview
to 2021-01-15
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @yupwei68 - i've left some comments inline. Also we have a test failure:
------- Stdout: -------
=== RUN TestAccCosmosDbMongoCollection_autoscale
=== PAUSE TestAccCosmosDbMongoCollection_autoscale
=== CONT TestAccCosmosDbMongoCollection_autoscale
testing_new_import_state.go:249: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
(map[string]string) (len=1) {
(string) (len=10) "throughput": (string) (len=3) "500"
}
(map[string]string) (len=1) {
(string) (len=10) "throughput": (string) (len=3) "400"
}
--- FAIL: TestAccCosmosDbMongoCollection_autoscale (1235.42s)
FAIL
@@ -123,7 +123,7 @@ func CosmosDbIndexingPolicySchema() *schema.Schema { | |||
ValidateFunc: validation.StringInSlice([]string{ | |||
string(documentdb.Consistent), | |||
string(documentdb.None), | |||
}, false), | |||
}, true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are we making this case insensitive? the SDK values should match what we get from the API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In preview version, the value is "consistent" and "none", but in current version, the value is "Consistent" and "None". To resolve the breaking change, I set it to be case insensitive?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of making this case insensitive can we just correct the casing to the SDK values on read?
also - could we open an issue on the SDK or somewhere to track this discrepancy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The case has been change when the service has released 2020-04-01 stable version. The old 2020-04-01-preview version has been deleted.
Since preview version permits breaking changes, shall I not open the issue?
azurerm/internal/services/cosmos/cosmosdb_account_data_source_test.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/cosmos/cosmosdb_gremlin_graph_resource.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.
Hi @katbyte Thanks for comments. I've run this test for three times locally. It all passed. I think it's a random hiccup. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, just a few small things left to address and this'll be good to merge
@@ -123,7 +123,7 @@ func CosmosDbIndexingPolicySchema() *schema.Schema { | |||
ValidateFunc: validation.StringInSlice([]string{ | |||
string(documentdb.Consistent), | |||
string(documentdb.None), | |||
}, false), | |||
}, true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of making this case insensitive can we just correct the casing to the SDK values on read?
also - could we open an issue on the SDK or somewhere to track this discrepancy?
azurerm/internal/services/cosmos/cosmosdb_gremlin_graph_resource.go
Outdated
Show resolved
Hide resolved
azurerm/internal/services/cosmos/cosmosdb_gremlin_graph_resource.go
Outdated
Show resolved
Hide resolved
Thanks kt for your comments. I've changed all these. Please continue reviewing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @yupwei68 - aside from one comment inline (add todo 3.0 to those constants) and these two test failures this looks ready to merge!
------- Stdout: -------
=== RUN TestAccCosmosDbSqlStoredProcedure_basic
=== PAUSE TestAccCosmosDbSqlStoredProcedure_basic
=== CONT TestAccCosmosDbSqlStoredProcedure_basic
testing.go:620: Step 1/2 error: Error running pre-apply refresh:
Error: Missing required argument
on config293530581/terraform_plugin_test.tf line 35, in resource "azurerm_cosmosdb_sql_container" "test":
35: resource "azurerm_cosmosdb_sql_container" "test" {
The argument "partition_key_path" is required, but no definition was found.
--- FAIL: TestAccCosmosDbSqlStoredProcedure_basic (6.26s)
FAIL
------- Stderr: -------
2021/04/22 04:53:16 [DEBUG] not using binary driver name, it's no longer needed
2021/04/22 04:53:16 [DEBUG] not using binary driver name, it's no longer needed
------- Stdout: -------
=== RUN TestAccCosmosDbSqlStoredProcedure_update
=== PAUSE TestAccCosmosDbSqlStoredProcedure_update
=== CONT TestAccCosmosDbSqlStoredProcedure_update
testing.go:620: Step 1/4 error: Error running pre-apply refresh:
Error: Missing required argument
on config415328271/terraform_plugin_test.tf line 35, in resource "azurerm_cosmosdb_sql_container" "test":
35: resource "azurerm_cosmosdb_sql_container" "test" {
The argument "partition_key_path" is required, but no definition was found.
--- FAIL: TestAccCosmosDbSqlStoredProcedure_update (6.02s)
FAIL
------- Stderr: -------
2021/04/22 04:53:21 [DEBUG] not using binary driver name, it's no longer needed
2021/04/22 04:53:21 [DEBUG] not using binary driver name, it's no longer needed
Hi kt, I've updated the description of the PR to list all changes and breaking changes. |
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 ... |
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. |
All tests have passed
changes:
indexing_mode
andorder
in resourcesazurerm_cosmosdb_sql_container
andazurerm_cosmosdb_gremlin_graph
is defined and returned in lower case instead of hybrid case. We keep the original hybrid cases instead of breaking changes.breaking changes:
azurerm_cosmosdb_account
tests. Thus,_id
index is required inazurerm_cosmosdb_mongo_collection
.azurerm_cosmosdb_gremlin_graph
andazurerm_cosmosdb_sql_container
.