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

Support for spatial_index in azurerm_cosmosdb_sql_container #11625

Merged
merged 11 commits into from
Jun 11, 2021

Conversation

yupwei68
Copy link
Contributor

@yupwei68 yupwei68 commented May 8, 2021

Fix #8817

--- PASS: TestAccCosmosDbSqlContainer_indexing_policy (1559.79s)

@yupwei68 yupwei68 changed the title Wyp cosmos sql container Support for spatial_index in azurerm_cosmosdb_sql_container May 8, 2021
@katbyte katbyte added this to the v2.59.0 milestone May 9, 2021
Comment on lines +74 to +87
spatialTypes := []documentdb.SpatialType{
documentdb.SpatialTypeLineString,
documentdb.SpatialTypeMultiPolygon,
documentdb.SpatialTypePoint,
documentdb.SpatialTypePolygon,
}

for _, i := range input {
indexPair := i.(map[string]interface{})
indexes = append(indexes, documentdb.SpatialSpec{
Types: &spatialTypes,
Path: utils.String(indexPair["path"].(string)),
})
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we expose this in schema instead of applying each spatial index to all types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The service will return all types no matter what types we try to update. But at least one type is necessary to be set, because missing types will lead to bad request.

Copy link
Collaborator

Choose a reason for hiding this comment

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

i'm not sure why that doesn't mean we shouldn't expose the four types for people to individually set?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means, for example, we update types = ["String"], and the service returns ["String", "MultiPolygon", "Point", "Polygon"], shall we ignore the difference for the customers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It means that the service has set all types for you. The service team has confirmed that it's by design. I'd like to suggest not to expose types to the clients?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's by design by service. Here is the service's reply:
https://gist.github.com/yupwei68/6c3ecffe6aeec33d945210e70bb3513a#gistcomment-3735507

Copy link
Collaborator

Choose a reason for hiding this comment

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

my point is would someone ever want to set a different spatial index path for different types? it type1 is path1 and type2 is path2 ect or will there always only ever be one index path for all types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There will always be index paths for all types, according to the service team. The user can't specify the specific types for their paths.

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok so to be clear, there is a single path that is shared by all types regardless and its not possible to assign different paths to different types? i assume the portal only has a "path" property then and hides away types from the user? if this is all the case maybe it make sense to document htis in the property so users know the path is applied to all types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The portal has not supported this yet. I have updated the document and I have marked the types computed.

azurerm/internal/services/cosmos/common/schema.go Outdated Show resolved Hide resolved
yupwei68 added 2 commits May 10, 2021 09:54
@yupwei68
Copy link
Contributor Author

Thanks kt for your comments. Please continue reviewing.
=== RUN TestAccCosmosDbSqlContainer_indexing_policy
=== PAUSE TestAccCosmosDbSqlContainer_indexing_policy
=== CONT TestAccCosmosDbSqlContainer_indexing_policy
--- PASS: TestAccCosmosDbSqlContainer_indexing_policy (1618.76s)

@ghost ghost removed the waiting-response label May 10, 2021
@yupwei68 yupwei68 requested a review from katbyte May 10, 2021 05:56
@katbyte katbyte modified the milestones: v2.59.0, v2.60.0 May 14, 2021
yupwei68 added 2 commits May 17, 2021 15:51
@yupwei68
Copy link
Contributor Author

Thanks kt for your comment. I've added a comment and some description in the documents for this problem. Please continue reviewing.

@ghost ghost removed the waiting-response label May 17, 2021
@katbyte katbyte modified the milestones: v2.60.0, v2.61.0 May 20, 2021
yupwei68 added 2 commits May 27, 2021 15:51
@tombuildsstuff tombuildsstuff modified the milestones: v2.61.0, v2.62.0 May 27, 2021
@katbyte
Copy link
Collaborator

katbyte commented Jun 3, 2021

@yupwei68 - looking for confirmation about this: #11625 (comment)

@katbyte katbyte removed this from the v2.62.0 milestone Jun 4, 2021
@katbyte katbyte added this to the v2.63.0 milestone Jun 4, 2021
@yupwei68
Copy link
Contributor Author

yupwei68 commented Jun 4, 2021

Thanks kt. Sorry for the missing. Please continue.

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 👍 just needt he merge conflict sorted

@yupwei68
Copy link
Contributor Author

Thanks kt, I've resolved the conflicts.

@katbyte katbyte modified the milestones: v2.63.0, v2.64.0 Jun 11, 2021
@katbyte katbyte merged commit 0356282 into hashicorp:master Jun 11, 2021
katbyte added a commit that referenced this pull request Jun 11, 2021
@yupwei68 yupwei68 deleted the wyp-cosmos-sql-container branch June 11, 2021 03:10
@github-actions
Copy link

This functionality has been released in v2.64.0 of the Terraform Provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template. Thank you!

@github-actions
Copy link

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 19, 2021
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.

Support for spatial indexes in azurerm_cosmosdb_sql_container indexing policy
3 participants