-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Support autoscaling Cosmos DB databases and containers #7773
Conversation
|
||
func CheckForChangeFromAutoscaleAndManualThroughput(d *schema.ResourceData) error { | ||
if d.HasChange("throughput") && d.HasChange("autoscale_settings") { | ||
return fmt.Errorf("Switching between autoscale and manual provisioned throughput is not supported at this time.") |
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.
Yes. You can also switch between autoscale and standard (manual) provisioned throughput as needed. Currently, for all APIs, you can only use the Azure portal to do these operations.
Based on this and no clear documented API for toggling back and forth, I've decided to not allow this operation
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.
There are some documented endpoints for migrating between autoscale and manual throughput (e.g. https://docs.microsoft.com/en-us/rest/api/cosmos-db-resource-provider/2020-04-01/tableresources/migratetabletoautoscale) however I couldn't find SDK support, so this seems fine 👍
How is this PR going? 👍 |
Does this PR also apply for azurerm_cosmosdb_mongo_database or only sql? |
@simongottschlag this PR is complete and just awaiting review from an approver based on their priorities @nadworny this PR covers all Cosmos APIs that are supported by Terraform today |
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.
@marc-sensenich, thank you so much for submitting this PR. It's looking really good I just left a few minor comments that I would like your input on. Once the changes are made this will LGTM! 🚀
azurerm/helpers/validate/cosmos.go
Outdated
@@ -42,3 +42,24 @@ func CosmosThroughput(v interface{}, k string) (warnings []string, errors []erro | |||
|
|||
return warnings, errors | |||
} | |||
|
|||
func CosmosMaxThroughput(v interface{}, k string) (warnings []string, errors []error) { | |||
value := v.(int) |
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.
Can we validate the type here to avoid a panic if the wrong type is passed?
value := v.(int) | |
v, ok := i.(int) | |
if !ok { | |
errors = append(errors, fmt.Errorf("expected type of %q to be int", k)) | |
return | |
} |
azurerm/helpers/validate/cosmos.go
Outdated
@@ -42,3 +42,24 @@ func CosmosThroughput(v interface{}, k string) (warnings []string, errors []erro | |||
|
|||
return warnings, errors | |||
} | |||
|
|||
func CosmosMaxThroughput(v interface{}, k string) (warnings []string, errors []error) { |
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.
can we change v
to be i
to be consistent with the rest of the provider code?
func CosmosMaxThroughput(v interface{}, k string) (warnings []string, errors []error) { | |
func CosmosMaxThroughput(i interface{}, k string) (warnings []string, errors []error) { |
azurerm/helpers/validate/cosmos.go
Outdated
func CosmosMaxThroughput(v interface{}, k string) (warnings []string, errors []error) { | ||
value := v.(int) | ||
|
||
if value < 4000 { |
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.
Can we switch all of the value
variables to be v
as well?
if value < 4000 { | |
if v < 4000 { |
@WodansSon I've made the requested changes in 7684202. |
@WodansSon or @katbyte are there any further changes you'd like to see in this PR? |
Hey @tombuildsstuff it looks like this is tagged for |
Co-authored-by: Tom Bamford <[email protected]>
…ce.go Co-authored-by: Tom Bamford <[email protected]>
Co-authored-by: Tom Bamford <[email protected]>
Co-authored-by: Tom Bamford <[email protected]>
@manicminer I've addressed your most recent comments. I'll run the full test-suite for Cosmos DB this evening and update this PR with the results |
@manicminer Cosmos DB database and collection acceptance tests have been run from 324d259 and are passing as noted in the initial PR message |
Hi Marc, thanks for doing this great work! Does this mean we can already use this functionality in Azure? I was looking at the documentation and there the autoscale option was not (yet) added. |
@jonnekleijer this functionality will be available in the AzureRM provider in the yet to be released 2.28.0 version |
This has been released in version 2.28.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.28.0"
}
# ... other configuration ... |
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 👉 [email protected]. Thanks! |
Adds support for enabling autoscaling Cosmos DB databases and containers
Depends on fix for issue #7825 for testing
Depends on #7597 (merged in v2.20.0)
Closes #6164
Closes #7315
Example Usage
Test Status
Run with commit 324d259
Passing
Failing
These failing tests are related to #7825