feat: Add privateEndpointHostames field to Data Federation resource#4358
feat: Add privateEndpointHostames field to Data Federation resource#4358marcabreracast wants to merge 21 commits intomasterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds exposure of Data Federation private endpoint hostname information in the Terraform provider’s federated database instance resource.
Changes:
- Adds a new computed
private_endpoint_hostnamesattribute to the federated database instance resource schema. - Populates the attribute during Read and Import using a new flattener for the Atlas SDK response type.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
internal/service/federateddatabaseinstance/resource_federated_database_instance.go
Outdated
Show resolved
Hide resolved
internal/service/federateddatabaseinstance/resource_federated_database_instance.go
Outdated
Show resolved
Hide resolved
internal/service/federateddatabaseinstance/resource_federated_database_instance.go
Outdated
Show resolved
Hide resolved
internal/service/federateddatabaseinstance/resource_federated_database_instance.go
Show resolved
Hide resolved
internal/service/federateddatabaseinstance/resource_federated_database_instance.go
Show resolved
Hide resolved
|
This PR has gone 7 days without any activity and meets the project’s definition of "stale". This will be auto-closed if there is no new activity over the next 7 days. If the issue is still relevant and active, you can simply comment with a "bump" to keep it open, or add the label "not_stale". Thanks for keeping our repository healthy! |
|
APIx bot: a message has been sent to Docs Slack channel |
🤖 Augment PR SummarySummary: Adds support for exposing private endpoint hostnames on Atlas Data Federation resources and data sources. Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
corryroot
left a comment
There was a problem hiding this comment.
LGTM! I commented with some copy nits.
|
|
||
| * `id` - The Terraform's unique identifier used internally for state management. | ||
| * `hostnames` - The list of hostnames assigned to the Federated Database Instance. Each string in the array is a hostname assigned to the Federated Database Instance. | ||
| * `private_endpoint_hostnames` - The list of private endpoint hostnames assigned to the Federated Database Instance. |
There was a problem hiding this comment.
| * `private_endpoint_hostnames` - The list of private endpoint hostnames assigned to the Federated Database Instance. | |
| * `private_endpoint_hostnames` - List of private endpoint hostnames assigned to the Federated Database Instance. |
| * `id` - The Terraform's unique identifier used internally for state management. | ||
| * `hostnames` - The list of hostnames assigned to the Federated Database Instance. Each string in the array is a hostname assigned to the Federated Database Instance. | ||
| * `private_endpoint_hostnames` - The list of private endpoint hostnames assigned to the Federated Database Instance. | ||
| * `private_endpoint_hostnames.#.hostname` - Human-readable label identifying the hostname. |
There was a problem hiding this comment.
| * `private_endpoint_hostnames.#.hostname` - Human-readable label identifying the hostname. | |
| * `private_endpoint_hostnames.#.hostname` - Human-readable label that identifies the host. |
| * `hostnames` - The list of hostnames assigned to the Federated Database Instance. Each string in the array is a hostname assigned to the Federated Database Instance. | ||
| * `private_endpoint_hostnames` - The list of private endpoint hostnames assigned to the Federated Database Instance. | ||
| * `private_endpoint_hostnames.#.hostname` - Human-readable label identifying the hostname. | ||
| * `private_endpoint_hostnames.#.private_endpoint` - Human-readable label identifying the private endpoint. |
There was a problem hiding this comment.
| * `private_endpoint_hostnames.#.private_endpoint` - Human-readable label identifying the private endpoint. | |
| * `private_endpoint_hostnames.#.private_endpoint` - Human-readable label that identifies the private endpoint. |
| * `private_endpoint_hostnames` - The list of private endpoint hostnames assigned to the Federated Database Instance. | ||
| * `private_endpoint_hostnames.#.hostname` - Human-readable label identifying the hostname. | ||
| * `private_endpoint_hostnames.#.private_endpoint` - Human-readable label identifying the private endpoint. |
| * `private_endpoint_hostnames` - The list of private endpoint hostnames assigned to the Federated Database Instance. | ||
| * `private_endpoint_hostnames.#.hostname` - Human-readable label identifying the hostname. | ||
| * `private_endpoint_hostnames.#.private_endpoint` - Human-readable label identifying the private endpoint. |
| "storage_stores.0.read_preference.0.tag_sets.#": "2", | ||
| "storage_stores.0.read_preference.0.tag_sets.0.tags.#": "2", | ||
| "storage_databases.0.collections.0.data_sources.0.database": "sample_airbnb", | ||
| "private_endpoint_hostnames.#": "0", |
There was a problem hiding this comment.
Do we have any test where this is set?
There was a problem hiding this comment.
Agree, first double check on which scenario it is populated and from there see feasibility of capturing within a test
There was a problem hiding this comment.
Added a test in a435f94 to fully create a private endpoint from scratch. Note that because the creation takes some time, I had to take a similar approach to what we're doing in encryptionatrest resource test, where we wait for the value to be populated.
| return diag.FromErr(fmt.Errorf(errorFederatedDatabaseInstanceSetting, "hostnames", name, err)) | ||
| } | ||
|
|
||
| if err := d.Set("private_endpoint_hostnames", flattenPrivateEndpointHostnames(dataFederationInstance.GetPrivateEndpointHostnames())); err != nil { |
There was a problem hiding this comment.
not sure if the API always returns privateEndpointHostnames = [] or omits the field.
Using the GetXXX will not differentiate the two cases but I think this is ok here since it is a computed attribute it makes it simpler for the consumers, so they never have to worry about null value and can always assume a []
EspenAlbert
left a comment
There was a problem hiding this comment.
LGTM. But ideally we should have a test where it is not empty
| `, federatedInstanceName, projectName, orgID) | ||
| } | ||
|
|
||
| func TestAccFederatedDatabaseInstance_withPrivateEndpoint(t *testing.T) { |
There was a problem hiding this comment.
[nit] Move test above all the internal functions
| region = "us-east-1" | ||
| } | ||
|
|
||
| resource "aws_vpc" "test" { |
There was a problem hiding this comment.
q: Is this the only place were we create aws_vpc* resources? Seems like a good opportunity to refactor?
There was a problem hiding this comment.
We also do it in resource_private_endpoint_regional_mode_test.go. However looking at the config used there, it differs meaningfully from what we're doing in this resource. I'd say that it's a refactor where the gain would be minimal as the boiler plate we could extract is very minor.
|
|
||
| func configWithPrivateEndpoint(projectID, name string) string { | ||
| return fmt.Sprintf(` | ||
| provider "aws" { |
There was a problem hiding this comment.
Why do we need this? Isn't it part of ExternalProviders?
There was a problem hiding this comment.
Not needed, added it because I wanted to make the region specific but as it's our default anyway, removed it in 7f39bec
| }, | ||
| { | ||
| PreConfig: waitForStatusUpdate, | ||
| RefreshState: true, |
There was a problem hiding this comment.
Why is this needed? (Add comment)
| Config: configWithPrivateEndpoint(projectID, name), | ||
| }, | ||
| { | ||
| PreConfig: waitForStatusUpdate, |
There was a problem hiding this comment.
Q: Is this something we should also document? That you need a sleep resource for the private_endpoint_hostnames to be populated?
Seems like a "gotcha" that users can run into.
There was a problem hiding this comment.
Yeah makes sense, added a note to the docs in 2f36c2d
There was a problem hiding this comment.
Looking into this further, a time_sleep resource would not help here since it cannot trigger a re-read of a resource that has already been applied to my understanding. A terraform apply -refresh-only after the initial apply will update state with the populated hostnames.
Rephrased the note in 76d38cc, let me know if it makes sense.
| } | ||
|
|
||
| func TestAccFederatedDatabaseInstance_withPrivateEndpoint(t *testing.T) { | ||
| var ( |
There was a problem hiding this comment.
Great job on adding this!
Maybe it is worth also adding it to the examples/ directory?
There was a problem hiding this comment.
We could add it yes, however where would we want it to go? We already have examples for Data Federation Private Link in examples/mongodbatlas_privatelink_endpoint/aws/data-federation-online-archive, and also for examples/mongodbatlas_federated_database_instance/aws
Description
This PR adds the computed attribute
private_endpoint_hostnamestomongodbatlas_federated_database_instanceresource, and corresponding data sourcesmongodbatlas_federated_database_instanceandmongodbatlas_federated_database_instances.Link to any related issue(s): CLOUDP-391245
Type of change:
Required Checklist:
Further comments