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

New Resource azurerm_cosmosdb_gremlin_graph #5301

Merged
merged 8 commits into from
Jan 8, 2020
Merged

New Resource azurerm_cosmosdb_gremlin_graph #5301

merged 8 commits into from
Jan 8, 2020

Conversation

SebRosander
Copy link
Contributor

Fixes last parts of #3692 & #4970

Tests:
=== RUN TestAccAzureRMCosmosDbGremlinGraph_basic
=== PAUSE TestAccAzureRMCosmosDbGremlinGraph_basic
=== CONT TestAccAzureRMCosmosDbGremlinGraph_basic
--- PASS: TestAccAzureRMCosmosDbGremlinGraph_basic (1264.20s)
PASS

=== RUN TestAccAzureRMCosmosDbGremlinGraph_customConflictResolutionPolicy
=== PAUSE TestAccAzureRMCosmosDbGremlinGraph_customConflictResolutionPolicy
=== CONT TestAccAzureRMCosmosDbGremlinGraph_customConflictResolutionPolicy
--- PASS: TestAccAzureRMCosmosDbGremlinGraph_customConflictResolutionPolicy (1286.11s)
PASS

=== RUN TestAccAzureRMCosmosDbGremlinGraph_indexPolicy
=== PAUSE TestAccAzureRMCosmosDbGremlinGraph_indexPolicy
=== CONT TestAccAzureRMCosmosDbGremlinGraph_indexPolicy
--- PASS: TestAccAzureRMCosmosDbGremlinGraph_indexPolicy (1250.10s)
PASS

=== RUN TestAccAzureRMCosmosDbGremlinGraph_complete
=== PAUSE TestAccAzureRMCosmosDbGremlinGraph_complete
=== CONT TestAccAzureRMCosmosDbGremlinGraph_complete
--- PASS: TestAccAzureRMCosmosDbGremlinGraph_complete (1413.13s)
PASS

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 for the PR @SebRosander, this is off to a great start. I've left some minor comments that once addressed this should be good to merge.

"indexing_mode": {
Type: schema.TypeString,
Required: true,
DiffSuppressFunc: suppress.CaseDifference,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we remove this and make it case sensitive?

We typically don't make things case insensitive unless there an API bug, in which case we open an issue on the SDK and link to it in a comment here

Suggested change
DiffSuppressFunc: suppress.CaseDifference,

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like an API bug. Had a short discussion with @tombuildsstuff on Slack. Issue can be found here: Azure/azure-sdk-for-go#6603

Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we add a comment here linking to the issue??

@SebRosander SebRosander requested a review from katbyte January 7, 2020 16:57
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 @SebRosander, just a couple more comments to address and this should be good to merge!

"indexing_mode": {
Type: schema.TypeString,
Required: true,
DiffSuppressFunc: suppress.CaseDifference,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we add a comment here linking to the issue??

} else {
id, err := azure.CosmosGetIDFromResponse(existing.Response)
if err != nil {
return fmt.Errorf("Error generationg import ID for Cosmos Gremlin Graph '%s' (Account: %s, Database: %s)", name, account, database)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("Error generationg import ID for Cosmos Gremlin Graph '%s' (Account: %s, Database: %s)", name, account, database)
return fmt.Errorf("Error getting import ID for Cosmos Gremlin Graph '%s' (Account: %s, Database: %s)", name, account, database)


An `index_policy` block supports the following:

* `automatic` - (Optional) Indicates if the indexing policy is automatic. Defaults to true
Copy link
Collaborator

Choose a reason for hiding this comment

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

we quote booleans

Suggested change
* `automatic` - (Optional) Indicates if the indexing policy is automatic. Defaults to true
* `automatic` - (Optional) Indicates if the indexing policy is automatic. Defaults to `true`.


* `automatic` - (Optional) Indicates if the indexing policy is automatic. Defaults to true

* `indexing_mode` - (Required) Indicates the indexing mode. Possible values include: 'Consistent', 'Lazy', 'None'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wrong quotes have been used

Suggested change
* `indexing_mode` - (Required) Indicates the indexing mode. Possible values include: 'Consistent', 'Lazy', 'None'
* `indexing_mode` - (Required) Indicates the indexing mode. Possible values include: `Consistent`, `Lazy`, `None`.


The following attributes are exported:

* `id` - the Cosmos DB Gremlin Graph ID.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* `id` - the Cosmos DB Gremlin Graph ID.
* `id` - The Cosmos DB Gremlin Graph ID.


* `indexing_mode` - (Required) Indicates the indexing mode. Possible values include: 'Consistent', 'Lazy', 'None'

* `included_paths` - (Optional) List of paths to include in the indexing. Required if `indexing_mode` is 'Consistent' or 'Lazy'.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we fix the rest of thes emis quoted blocks

Suggested change
* `included_paths` - (Optional) List of paths to include in the indexing. Required if `indexing_mode` is 'Consistent' or 'Lazy'.
* `included_paths` - (Optional) List of paths to include in the indexing. Required if `indexing_mode` is `Consistent` or `Lazy`.

@katbyte katbyte added this to the v1.41.0 milestone Jan 7, 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 for the revisions @SebRosander! LGTM now 👍

@ghost
Copy link

ghost commented Jan 16, 2020

This has been released in version 1.41.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 = "~> 1.41.0"
}
# ... other configuration ...

@ghost ghost removed the waiting-response label Jan 16, 2020
@ghost
Copy link

ghost commented Feb 8, 2020

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 and limited conversation to collaborators Feb 8, 2020
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.

3 participants