Skip to content
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_cosmosdb_mongo_collection - correctly read the _id index back when mongo 3.6 #8690

Merged
merged 8 commits into from
Dec 30, 2020

Conversation

yupwei68
Copy link
Contributor

Fix #8660

=== RUN TestAccAzureRMCosmosDbMongoCollection_basic
=== PAUSE TestAccAzureRMCosmosDbMongoCollection_basic
=== CONT TestAccAzureRMCosmosDbMongoCollection_basic
--- PASS: TestAccAzureRMCosmosDbMongoCollection_basic (1811.27s)
=== RUN TestAccAzureRMCosmosDbMongoCollection_complete
=== PAUSE TestAccAzureRMCosmosDbMongoCollection_complete
=== CONT TestAccAzureRMCosmosDbMongoCollection_complete
--- PASS: TestAccAzureRMCosmosDbMongoCollection_complete (1819.16s)
=== RUN TestAccAzureRMCosmosDbMongoCollection_update
=== PAUSE TestAccAzureRMCosmosDbMongoCollection_update
=== CONT TestAccAzureRMCosmosDbMongoCollection_update
--- PASS: TestAccAzureRMCosmosDbMongoCollection_update (2097.46s)
=== RUN TestAccAzureRMCosmosDbMongoCollection_throughput
=== PAUSE TestAccAzureRMCosmosDbMongoCollection_throughput
=== CONT TestAccAzureRMCosmosDbMongoCollection_throughput
--- PASS: TestAccAzureRMCosmosDbMongoCollection_throughput (2052.64s)
=== RUN TestAccAzureRMCosmosDbMongoCollection_withIndex
=== PAUSE TestAccAzureRMCosmosDbMongoCollection_withIndex
=== CONT TestAccAzureRMCosmosDbMongoCollection_withIndex
--- PASS: TestAccAzureRMCosmosDbMongoCollection_withIndex (1824.25s)
=== RUN TestAccAzureRMCosmosDbMongoCollection_autoscale
=== PAUSE TestAccAzureRMCosmosDbMongoCollection_autoscale
=== CONT TestAccAzureRMCosmosDbMongoCollection_autoscale
--- PASS: TestAccAzureRMCosmosDbMongoCollection_autoscale (2234.07s)
=== RUN TestAccAzureRMCosmosDbMongoCollection_ver36
=== PAUSE TestAccAzureRMCosmosDbMongoCollection_ver36
=== CONT TestAccAzureRMCosmosDbMongoCollection_ver36
--- PASS: TestAccAzureRMCosmosDbMongoCollection_ver36 (1685.88s)

@ghost ghost added the size/M label Sep 30, 2020
@yupwei68 yupwei68 closed this Sep 30, 2020
@yupwei68 yupwei68 reopened this Sep 30, 2020
Copy link
Member

@jackofallops jackofallops left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @yupwei68
Thanks for this PR. I'm afraid I think this needs some more thought. The linked issue, and indeed the older related issue #8144, indicate that the _id index should always be present if custom indexes are specified after the breaking change in behaviour for the API. Native MongoDB creates this, but CosmosDB Mongo does not.

In a 3.6 Collection the _id index is returned by the API (I've not tried the 3.2 version, but I suspect it's there also)

{
  "id": "/subscriptions/xxx/resourceGroups/xxx/providers/Microsoft.DocumentDB/databaseAccounts/dev-ca/apis/mongodb/databases/dev-cosmosmongo/collections/dev-cosmos-collection",
  "type": "Microsoft.DocumentDB/databaseAccounts/apis/databases/collections",
  "name": "dev-cosmos-collection",
  "properties": {
    "id": "dev-cosmos-collection",
    "_rid": "CsMXAJWjA5o=",
    "_etag": "\"00003304-0000-0d00-0000-5f9956260000\"",
    "_ts": 1603884582,
    "indexes": [
      {
        "key": {
          "keys": [
            "_id"
          ]
        },
        "options": {}
      },
      {
        "key": {
          "keys": [
            "_ts"
          ]
        },
...

The service defaults to 3.2 when created by the API (The portal defaults to 3.6). This is changed by sending the EnableMongo value in the capabilities list on the account. (this is definitely not obvious), so your test doesn't exercise the correct path I'm afraid.

In my comments on 8144 for potential solutions, there were a couple approaches we could take. We document that the user needs to specify the _id index when creating a collection, or we add it in the code if indexes are specified but that one is missing. Ideally the service shouldn't need us to do any of this. Native MongoDB creates that index automatically when a new collection is created, so the ideal situation would be that the service team match Native behaviour that users would expect from a MongoDB Service.

@yupwei68
Copy link
Contributor Author

Thanks @jackofallops for more context and information. Then I think the problem exists in that we have set the index "_id" to system_index. I've done the corresponding changes. All tests have passed. Please continue reviewing.

=== RUN TestAccAzureRMCosmosDbMongoCollection_basic
=== PAUSE TestAccAzureRMCosmosDbMongoCollection_basic
=== CONT TestAccAzureRMCosmosDbMongoCollection_basic
--- PASS: TestAccAzureRMCosmosDbMongoCollection_basic (1637.67s)
=== RUN TestAccAzureRMCosmosDbMongoCollection_complete
=== PAUSE TestAccAzureRMCosmosDbMongoCollection_complete
=== CONT TestAccAzureRMCosmosDbMongoCollection_complete
--- PASS: TestAccAzureRMCosmosDbMongoCollection_complete (1643.92s)
=== RUN TestAccAzureRMCosmosDbMongoCollection_update
=== PAUSE TestAccAzureRMCosmosDbMongoCollection_update
=== CONT TestAccAzureRMCosmosDbMongoCollection_update
--- PASS: TestAccAzureRMCosmosDbMongoCollection_update (1953.35s)
=== RUN TestAccAzureRMCosmosDbMongoCollection_throughput
=== PAUSE TestAccAzureRMCosmosDbMongoCollection_throughput
=== CONT TestAccAzureRMCosmosDbMongoCollection_throughput
--- PASS: TestAccAzureRMCosmosDbMongoCollection_throughput (2160.28s)
=== RUN TestAccAzureRMCosmosDbMongoCollection_withIndex
=== PAUSE TestAccAzureRMCosmosDbMongoCollection_withIndex
=== CONT TestAccAzureRMCosmosDbMongoCollection_withIndex
--- PASS: TestAccAzureRMCosmosDbMongoCollection_withIndex (1639.39s)
=== RUN TestAccAzureRMCosmosDbMongoCollection_autoscale
=== PAUSE TestAccAzureRMCosmosDbMongoCollection_autoscale
=== CONT TestAccAzureRMCosmosDbMongoCollection_autoscale
--- PASS: TestAccAzureRMCosmosDbMongoCollection_autoscale (2013.61s)
=== RUN TestAccAzureRMCosmosDbMongoCollection_ver36
=== PAUSE TestAccAzureRMCosmosDbMongoCollection_ver36
=== CONT TestAccAzureRMCosmosDbMongoCollection_ver36
--- PASS: TestAccAzureRMCosmosDbMongoCollection_ver36 (1787.49s)

@ghost ghost removed the waiting-response label Oct 29, 2020
@LaurentLesle
Copy link
Contributor

@yupwei68 @jackofallops do you think you can complete this PR soon? Looks like an issue pending on this PR #8660

Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @yupwei68 - couple comments but otherwise looks good

@katbyte katbyte added this to the v2.42.0 milestone Dec 27, 2020
@yupwei68
Copy link
Contributor Author

@katbyte Thanks for your comments. Please continue reviewing.

@katbyte katbyte changed the title azurerm_cosmosdb_mongo_collection plan is not empty when _id is added in keys in index azurerm_cosmosdb_mongo_collection - correctly read the _id index back when mongo 3.6 Dec 30, 2020
Copy link
Collaborator

@katbyte katbyte left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @yupwei68 - LGTM 👍

@katbyte katbyte merged commit 6bd58af into hashicorp:master Dec 30, 2020
katbyte added a commit that referenced this pull request Dec 30, 2020
@ghost
Copy link

ghost commented Jan 8, 2021

This has been released in version 2.42.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.42.0"
}
# ... other configuration ...

@ghost
Copy link

ghost commented Jan 29, 2021

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!

@ghost ghost locked as resolved and limited conversation to collaborators Jan 29, 2021
@yupwei68 yupwei68 deleted the wyp-cos-co36 branch June 7, 2021 06:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terraform wants to update collection index every time
5 participants