Skip to content

Conversation

ymorimo
Copy link
Contributor

@ymorimo ymorimo commented Sep 17, 2020

https://scalar-labs.atlassian.net/browse/DLT-7024

  • Add azurerm_cosmosdb_account resource
  • Add an option enable_cosmosdb_service_endpoint to enable a service endpoint for Cosmos DB in a subnet that the kubernetes module creates

@ymorimo ymorimo self-assigned this Sep 17, 2020
@feeblefakie feeblefakie requested a review from yito88 September 17, 2020 07:09
@ymorimo ymorimo marked this pull request as draft September 17, 2020 07:47
@ymorimo ymorimo marked this pull request as ready for review September 17, 2020 08:10
Copy link
Collaborator

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

Overall looking good! Left some suggestions.

@@ -0,0 +1,23 @@
resource "azurerm_cosmosdb_account" "db" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@yito88 Can you take a look at this Cosmos DB configuration if it is OK?

# cluster_auto_scaling_max_count = "6"
}

# enable_cosmosdb_service_endpoint = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the variable name is a little too specific to what it actually does instead of what users want.
How about enable_cosmosdb or use_cosmosdb? (I think the latter sounds good to me since a user simply declares that he/she uses it.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

use_cosmosdb sounds good to me too. I'll update it.

Copy link
Contributor Author

@ymorimo ymorimo Sep 17, 2020

Choose a reason for hiding this comment

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

a44a004 and 837cbd7. Updated the description field to tell what the option does.

resource_group_name = local.network_name
offer_type = "Standard"

enable_automatic_failover = true
Copy link
Member

Choose a reason for hiding this comment

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

I think it is better to set enable_automatic_failover false for now.
First, It will not be enabled since Scalar DB doesn't support multiple regions now. (Strong consistency isn't supported with multiple regions.)
Though I'm not sure what happens when this failover, according to the document, the write region will be changed automatically. I guess it is possible to cause inconsistency for Scalar DB.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! Updated in 87ea970.

name = "${local.network_name}-cosmosdb"
location = local.region
resource_group_name = local.network_name
offer_type = "Standard"
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better to specify the kind?

kind = "GlobalDocumentDB"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in ffb11fc


output "cosmosdb_account_read_endpoints" {
value = azurerm_cosmosdb_account.db.read_endpoints
}
Copy link
Contributor

@tei-k tei-k Sep 18, 2020

Choose a reason for hiding this comment

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

Do you have a reason for not writing a description from below?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I just forgot to update the descriptions! Updated in 5ad6b0d. Thanks.

node_pool_subnet_id = var.kubernetes.node_pool_subnet_id
bastion_ip = var.network.bastion_ip
internal_domain = var.network.internal_domain
triggers = [var.network.bastion_provision_id]
Copy link
Contributor

Choose a reason for hiding this comment

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

network_dns network_id bastion_ip internal_domain triggers

If not using, I think we can delete its.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Removed unnecessary variables in 550394d.

Comment on lines 30 to 35
| cosmosdb_account_primary_master_key | n/a |
| cosmosdb_account_primary_readonly_master_key | n/a |
| cosmosdb_account_read_endpoints | n/a |
| cosmosdb_account_secondary_master_key | n/a |
| cosmosdb_account_secondary_readonly_master_key | n/a |
| cosmosdb_account_write_endpoints | n/a |
Copy link
Contributor

Choose a reason for hiding this comment

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

Please update the descriptions .


output "cosmosdb_account_connection_strings" {
value = azurerm_cosmosdb_account.db.connection_strings
description = "A list of connection strings available for this CosmosDB account."
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do you use this only here? not the?

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 was just a mistake. Thanks! Fixed.

Copy link
Contributor

@tei-k tei-k left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Collaborator

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM!

@feeblefakie
Copy link
Collaborator

@yito88 I will merge it but PTAL again !

@feeblefakie feeblefakie merged commit 3d9304e into master Sep 23, 2020
@feeblefakie feeblefakie deleted the cosmosdb-module branch September 23, 2020 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants