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_account - primary_sql_connection_string, secondary_sql_connection_string, primary_readonly_sql_connection_string, secondary_readonly_sql_connection_string #17810

Merged
merged 10 commits into from
Oct 20, 2022

Conversation

lewis-od
Copy link
Contributor

Add individual connection string attributes to cosmosdb_account resource

Closes #15947

Copy link
Contributor

@tombuildsstuff tombuildsstuff left a comment

Choose a reason for hiding this comment

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

hey @lewis-od

Thanks for this PR - taking a look through on the whole this is looking pretty good but I think it could be worth adding sql to the property names since there's different interfaces available to CosmosDB (e.g. SQL / Mongo etc) depending on what's provisioned - WDYT?

Thanks!

internal/services/cosmos/cosmosdb_account_resource.go Outdated Show resolved Hide resolved
internal/services/cosmos/cosmosdb_account_resource.go Outdated Show resolved Hide resolved
internal/services/cosmos/cosmosdb_account_resource_test.go Outdated Show resolved Hide resolved
website/docs/d/cosmosdb_account.html.markdown Outdated Show resolved Hide resolved
@lewis-od
Copy link
Contributor Author

lewis-od commented Aug 5, 2022

hey @lewis-od

Thanks for this PR - taking a look through on the whole this is looking pretty good but I think it could be worth adding sql to the property names since there's different interfaces available to CosmosDB (e.g. SQL / Mongo etc) depending on what's provisioned - WDYT?

Thanks!

Hey @tombuildsstuff, thanks for the quick review!

Ah yeah good point - will update as suggested 😊

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 @lewis-od, but i think something odd is going on here as i am seeing a lot of failing tests:

------- Stdout: -------
=== RUN   TestAccCosmosDBAccount_basic_mongo_strong_without_capability
=== PAUSE TestAccCosmosDBAccount_basic_mongo_strong_without_capability
=== CONT  TestAccCosmosDBAccount_basic_mongo_strong_without_capability
    testcase.go:110: Step 1/2 error: Check failed: 1 error occurred:
        	* Check 1/1 error: Check 14/17 error: azurerm_cosmosdb_account.test: Attribute 'primary_connection_string' expected to be set
        
--- FAIL: TestAccCosmosDBAccount_basic_mongo_strong_without_capability (844.62s)
FAIL

@lewis-od
Copy link
Contributor Author

Thanks @lewis-od, but i think something odd is going on here as i am seeing a lot of failing tests:

------- Stdout: -------
=== RUN   TestAccCosmosDBAccount_basic_mongo_strong_without_capability
=== PAUSE TestAccCosmosDBAccount_basic_mongo_strong_without_capability
=== CONT  TestAccCosmosDBAccount_basic_mongo_strong_without_capability
    testcase.go:110: Step 1/2 error: Check failed: 1 error occurred:
        	* Check 1/1 error: Check 14/17 error: azurerm_cosmosdb_account.test: Attribute 'primary_connection_string' expected to be set
        
--- FAIL: TestAccCosmosDBAccount_basic_mongo_strong_without_capability (844.62s)
FAIL

Ah sorry yeah looks like there's 2 issues:

  • The property names that the tests are asserting on are wrong (e.g. primary_connection_string instead of primary_sql_connection_string)
  • All of the tests will be expecting the connection string properties to be present, but they won't be set if the kind is MongoDB

Will have a look at fixing these

@lewis-od
Copy link
Contributor Author

@katbyte okay hopefully those tests should pass now! I've not been able to run all of them, but have ran a couple of both the MongoDB and GlobalDocumentDB varieties and they're working for me

The tflint workflow was failing so I've ran make terrafmt, but it's edited a file I hadn't touched (website/docs/r/kusto_cluster_managed_private_endpoint.html.markdown). Not sure if you want that change in this PR or not?

@lewis-od lewis-od requested review from katbyte and removed request for tombuildsstuff August 19, 2022 11:05
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.

Sadly still seeing test failures:
image

@lewis-od lewis-od force-pushed the cdb-connection-strings-15947 branch from 354946f to 7a117d7 Compare October 14, 2022 10:47
@lewis-od
Copy link
Contributor Author

Sadly still seeing test failures: image

Sorry for the delay in getting back on this. Looks like I hadn't accounted for the legacy Parse kind of Cosmos account. I've made some changes and have ran the TestAccCosmosDBAccount_basic_* tests myself, and all seem to be passing now

@lewis-od lewis-od requested review from katbyte and tombuildsstuff and removed request for katbyte and tombuildsstuff October 14, 2022 11:11
@katbyte katbyte self-assigned this Oct 17, 2022
@katbyte katbyte changed the title Add connection string attributes to CosmosDB account azurerm_cosmosdb_account - primary_sql_connection_string, secondary_sql_connection_string, primary_readonly_sql_connection_string, secondary_readonly_sql_connection_string Oct 20, 2022
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 @lewis-od ! LGTM 🍄

@katbyte katbyte merged commit d01739e into hashicorp:main Oct 20, 2022
katbyte added a commit that referenced this pull request Oct 20, 2022
@github-actions github-actions bot added this to the v3.28.0 milestone Oct 20, 2022
@lewis-od lewis-od deleted the cdb-connection-strings-15947 branch October 21, 2022 10:03
@github-actions
Copy link

This functionality has been released in v3.28.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 Nov 24, 2022
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.

define a cosmos DB connection string into an Azure function app_settings
3 participants