-
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
azurerm_cosmos_db_account
: allow setting the Kind to MongoDB/GlobalDocumentDB
#299
Conversation
Fixes #290 ``` $ acctests azurerm TestAccAzureRMCosmosDBAccount_mongoDB === RUN TestAccAzureRMCosmosDBAccount_mongoDB --- PASS: TestAccAzureRMCosmosDBAccount_mongoDB (670.91s) PASS ok github.com/terraform-providers/terraform-provider-azurerm/azurerm 670.928s ```
@@ -215,6 +229,7 @@ func resourceArmCosmosDBAccountRead(d *schema.ResourceData, meta interface{}) er | |||
d.Set("name", resp.Name) | |||
d.Set("location", azureRMNormalizeLocation(*resp.Location)) | |||
d.Set("resource_group_name", resGroup) | |||
d.Set("kind", string(resp.Kind)) |
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.
I would have thought d.Set
can do the casting, but maybe not... 🤔
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.
we could, however I think it's worth being explicit that this value is coming from an external type?
Config: config, | ||
Check: resource.ComposeTestCheckFunc( | ||
testCheckAzureRMCosmosDBAccountExists(resourceName), | ||
resource.TestCheckResourceAttr(resourceName, "kind", "MongoDB"), |
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.
It would be nice to add a similar check to existing tests - just to verify that we expose the default kind.
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.
Done
"kind": { | ||
Type: schema.TypeString, | ||
Optional: true, | ||
ForceNew: 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.
As this is a ForceNew field, are you sure we don't need a state migration? Technically the old state will have empty kind
(or no kind
to be precise) and we're setting default here, so "" => "global-document-db"
would cause recreation, unless I'm mistaken?
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.
Just verified - seems good since there's a default value and this field was missing previously:
$ envchain azurerm terraform plan
Refreshing Terraform state in-memory prior to plan...
The refreshed state will be used to calculate this plan, but will not be
persisted to local or remote state storage.
azurerm_resource_group.test: Refreshing state... (ID: /subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/tharvey-dev)
azurerm_cosmosdb_account.test: Refreshing state... (ID: /subscriptions/00000000-0000-0000-0000-...ntDB/databaseAccounts/tharvey-cosmosdb)
No changes. Infrastructure is up-to-date.
This means that Terraform did not detect any differences between your
configuration and real physical resources that exist. As a result, Terraform
doesn't need to do anything.
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! |
Allow setting the
kind
field to eitherMongoDB
orGlobalDocumentDB
(which it's defaulted too for existing users)Fixes #290
New test passes - just running the lot: