-
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
cosmosdb_mongo_collection - support index
& system_index
#6426
Conversation
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 @neil-yechenwei,
it looks like you're not setting the index back on read? thence why you are having to do the import ignore. We should reading these back and ignoring the system ones?
@katbyte , after tested previously, I found some system keys can be updated and some system keys cannot be updated. So I think maybe we cannot simply ignore all system keys, right? After checked azure cli and powershell for this resource, seems they also didn't ignore system keys but expose all keys to user. So I think we can align with them. Does it make sense? |
@neil-yechenwei - however we do it, maybe separate |
@katbyte , because some system indexes can be updated. So if I move them to computed index_system property, it would cause diff on index property. So seems I cannot do this. After double confirmed with service owner, seems only system indexes "_id" and "DocumentDBDefaultIndex" cannot be updated, other system indexes can be updated. So I ignore them. See my updated code. |
@neil-yechenwei, we should always be reading in the values the user can set. weather we seperate user settable and system into |
@katbyte , I've updated code to move user settable indexes which include user customized index and settable system index into property "index" and move system indexes which are not settable into a computed "system_index" |
index
& system_index
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 for the changes @neil-yechenwei,
Just one minor comment inline otherwise this LGTM!
}, | ||
}, | ||
|
||
"system_index": { |
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 think this should be system_indexes
as it'll never be used as seperate blocks?
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.
updated
@katbyte , thanks for your comments. I've updated code. |
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 @neil-yechenwei! LGTM now 👍
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 for pushing those changes - I've taken a look and left some comments inline - but if we can fix those up then this otherwise LGTM
Thanks!
@@ -264,7 +303,16 @@ func resourceArmCosmosDbMongoCollectionRead(d *schema.ResourceData, meta interfa | |||
} | |||
|
|||
if props.Indexes != nil { |
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 need to ensure these fields are always set, so we can remove the if statement here, since the flatten
function should handle these being nil
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.
updated
for _, i := range *indexes { | ||
if key := i.Key; key != nil { | ||
var ttlInner int32 | ||
func flattenCosmosMongoCollectionIndex(input *[]documentdb.MongoIndex) (*[]map[string]interface{}, *[]map[string]interface{}, *int32) { |
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.
this method needs to account for input
being nil here
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.
updated
systemIndex := map[string]interface{}{} | ||
|
||
if v.Key != nil && v.Key.Keys != nil { | ||
key := (*v.Key.Keys)[0] |
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's a crash here if there's 0 items in this
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.
updated
|
||
* `keys` - The list of system keys which are not settable for each Cosmos DB Mongo Collection. | ||
|
||
* `unique` - The unique status of the index. |
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 add a better description here?
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.
updated
@tombuildsstuff , thanks for your comments. I've updated code. |
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 @neil-yechenwei! LGTM 👍
This has been released in version 2.7.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.7.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! |
Fix the issue of #4460
Fixes #4460