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_postgresql_cluster: add server names information t… #25240

Merged
merged 2 commits into from
Mar 15, 2024

Conversation

reastyn
Copy link
Contributor

@reastyn reastyn commented Mar 13, 2024

I have tried to deploy CosmosDB Postgres, but with some of the updates they added a random hexadecimal string to coordinator domain (from c-acctestcluster240313222245905652.postgres.cosmos.azure.com -> c-acctestcluster240313222245905652.efyg2q5iqomwly.postgres.cosmos.azure.com). When I wanted to use this domain to be passed to other modules I couldn't as I can derive the domain from the service name. Therefore I just propagated the server_names property to the attributes, which means it can be used as for example a output.

Testing Logs/Evidence

I added the attribute to the basic test as seen in internal/services/cosmos/cosmosdb_postgresql_cluster_resource_test.go and successfully ran test locally.

Change Log

  • azurerm_cosmosdb_postgresql_cluster - add fully qualified domain name to attributes

This is a (please select all that apply):

  • Bug Fix
  • New Feature (ie adding a service, resource, or data source)
  • Enhancement
  • Breaking Change

@reastyn
Copy link
Contributor Author

reastyn commented Mar 14, 2024

I also just noticed, that this stackoverflow mentions this problem
https://stackoverflow.com/questions/76579820/output-azure-cosmos-db-for-postgresql-connection-string-with-terraform

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks @reastyn. I left some comments around consistency, would you mind taking a look at those? Once they're addressed we can take another look through.

@@ -636,3 +659,19 @@ func flattenMaintenanceWindow(input *clusters.MaintenanceWindow) []MaintenanceWi
},
}
}

func formatServerNames(input *[]clusters.ServerNameItem) []ServerNameItem {
Copy link
Member

Choose a reason for hiding this comment

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

Minor but for consistency could we rename this to

Suggested change
func formatServerNames(input *[]clusters.ServerNameItem) []ServerNameItem {
func flattenServerNames(input *[]clusters.ServerNameItem) []ServerNameItem {

Comment on lines 664 to 668
if input == nil {
return nil
}

var output []ServerNameItem
Copy link
Member

Choose a reason for hiding this comment

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

We should be setting an empty value into state instead of nil here

Suggested change
if input == nil {
return nil
}
var output []ServerNameItem
var output []ServerNameItem
if input == nil {
return output
}

@@ -304,6 +310,22 @@ func (r CosmosDbPostgreSQLClusterResource) Attributes() map[string]*pluginsdk.Sc
Type: pluginsdk.TypeString,
Computed: true,
},
"server_names": {
Copy link
Member

Choose a reason for hiding this comment

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

We can shorten this to servers, _names is implicit and one of the attributes in the block is already called name

Suggested change
"server_names": {
"servers": {

@@ -99,6 +99,8 @@ In addition to the Arguments listed above - the following Attributes are exporte

* `earliest_restore_time` - The earliest restore point time (ISO8601 format) for the Azure Cosmos DB for PostgreSQL Cluster.

* `server_names` - The name of the servers and its fully qualified domain name
Copy link
Member

Choose a reason for hiding this comment

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

Since this is a block it should be

Suggested change
* `server_names` - The name of the servers and its fully qualified domain name
* `servers` - A `servers` block as defined below.
---
A `servers` block exports the following:
* `fqdn` - The Fully Qualified Domain Name of the server.
* `name` - The name of the server.

@reastyn
Copy link
Contributor Author

reastyn commented Mar 15, 2024

Thanks a lot for taking a look, the issues you mentioned should be resolved

Copy link
Member

@stephybun stephybun left a comment

Choose a reason for hiding this comment

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

Thanks @reastyn LGTM 👍

@stephybun stephybun merged commit eddb603 into hashicorp:main Mar 15, 2024
33 checks passed
@github-actions github-actions bot added this to the v3.97.0 milestone Mar 15, 2024
stephybun added a commit that referenced this pull request Mar 18, 2024
@NemSimpraga
Copy link

NemSimpraga commented Mar 18, 2024

Thank you @reastyn ! I am currently doing a hack/patch to get this very very basic property via azapi , which I do NOT like! I mean it baffles me how this resource could've even been created without that attribute out of all! Deploying a DB and not being able to query it's endpoint? Crazy!

data "azapi_resource" "db" {
  name                   = azurerm_cosmosdb_postgresql_cluster.db.name
  parent_id              = azurerm_resource_group.main.id
  type                   = "Microsoft.DBforPostgreSQL/serverGroupsv2@2023-03-02-preview"
  response_export_values = ["properties.serverNames"]
}

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 Apr 18, 2024
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