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

feat: Adds new ip_addresses computed attribute in mongodbatlas_project resource and data sources #1850

Merged
merged 17 commits into from
Jan 19, 2024

Conversation

AgustinBettati
Copy link
Member

@AgustinBettati AgustinBettati commented Jan 17, 2024

Description

Link to any related issue(s): CLOUDP-215106

Includes new computed attribute ip_addresses in project resource and data sources.

Resulting terraform state for a project with no clusters

{
    "mode": "data",
    "type": "mongodbatlas_project",
    "provider": "provider[\"registry.terraform.io/mongodb/mongodbatlas\"]",
    "instances": [
        {
            "schema_version": 0,
            "attributes": {
                "cluster_count": 0,
                "created": "2023-10-24T09:22:24Z",
                "id": "65378ccff055c147c22c7a45",
                "ip_addresses": {
                    "services": {
                        "clusters": []
                    }
                },
                ...
            }
        }
    ]
}

Resulting terraform state for a project with clusters

(IP values obfuscated manually)

{
    "mode": "data",
    "type": "mongodbatlas_project",
    "name": "test",
    "provider": "provider[\"registry.terraform.io/mongodb/mongodbatlas\"]",
    "instances": [
        {
            "schema_version": 0,
            "attributes": {
                "cluster_count": 1,
                "created": "2023-10-10T09:07:21Z",
                "id": "65251446ae5f3f6ec7968b13",
                "ip_addresses": {
                    "services": {
                        "clusters": [
                            {
                                "cluster_name": "Cluster0",
                                "inbound": [
                                    "13.53.*****",
                                    "13.50.*****",
                                    "13.48.*****"
                                ],
                                "outbound": [
                                    "13.53.*****",
                                    "13.50.*****",
                                    "13.48.*****"
                                ]
                            }
                        ]
                    }
                }
                ...
            }
        }
    ]
}

Type of change:

  • Bug fix (non-breaking change which fixes an issue). Please, add the "bug" label to the PR.
  • New feature (non-breaking change which adds functionality). Please, add the "enhancement" label to the PR.
  • Breaking change (fix or feature that would cause existing functionality to not work as expected). Please, add the "breaking change" label to the PR.
  • This change requires a documentation update
  • Documentation fix/enhancement

Required Checklist:

  • I have signed the MongoDB CLA
  • I have read the contribution guidelines
  • I have checked that this change does not generate any credentials and that they are NOT accidentally logged anywhere.
  • I have added tests that prove my fix is effective or that my feature works per HashiCorp requirements
  • I have added any necessary documentation (if appropriate)
  • I have run make fmt and formatted my code
  • If changes include deprecations or removals, I defined an isolated PR with a relevant title as it will be used in the auto-generated changelog.

Further comments

@AgustinBettati AgustinBettati changed the title Cloudp 215106 feat: Adds new ip_addresses computed attributed in mongodbatlas_project resource and data sources Jan 17, 2024
@AgustinBettati AgustinBettati changed the title feat: Adds new ip_addresses computed attributed in mongodbatlas_project resource and data sources feat: Adds new ip_addresses computed attribute in mongodbatlas_project resource and data sources Jan 17, 2024
@@ -6,12 +6,18 @@ filename: "{{ .InterfaceName | snakecase }}.go"
mockname: "{{.InterfaceName}}"

packages:
? github.com/mongodb/terraform-provider-mongodbatlas/internal/service/searchdeployment
Copy link
Member Author

Choose a reason for hiding this comment

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

mockery config file was not working as expected, only the last package (in this case advancedcluster) was used for generating mocks.

Comment on lines +89 to +91
* `teams` - Returns all teams to which the authenticated user has access in the project. See [Teams](#teams).
* `limits` - The limits for the specified project. See [Limits](#limits).
* `ip_addresses` - IP addresses in a project categorized by services. See [IP Addresses](#ip-addresses).
Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusting format of defining nested schema in other section, generates consistency with project resource documentation and how tfplugindocs also defines nested schemas.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nice!

@@ -137,12 +138,40 @@ func (d *projectDS) Schema(ctx context.Context, req datasource.SchemaRequest, re
},
},
},
"ip_addresses": schema.SingleNestedAttribute{
Copy link
Member

Choose a reason for hiding this comment

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

can something be used from project_ip_access_list resource/ds or they're different concepts?

Copy link
Member Author

@AgustinBettati AgustinBettati Jan 18, 2024

Choose a reason for hiding this comment

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

They seem to be different concepts. From testing I can see that it provides the IP address of each cluster node. This scope document gives some more details as to users that would leverage these ip addresses

@@ -134,6 +134,34 @@ func (d *ProjectsDS) Schema(ctx context.Context, req datasource.SchemaRequest, r
},
},
},
"ip_addresses": schema.SingleNestedAttribute{
Copy link
Member

Choose a reason for hiding this comment

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

can some part of the schema be reused from the RS or it's hard because here all are Computed?

Copy link
Member Author

Choose a reason for hiding this comment

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

The main limitation is that they are different schema types, terraform-plugin-framework/datasource/schema and terraform-plugin-framework/resource/schema respectively.

@@ -137,12 +138,40 @@ func (d *projectDS) Schema(ctx context.Context, req datasource.SchemaRequest, re
},
},
},
"ip_addresses": schema.SingleNestedAttribute{
Attributes: map[string]schema.Attribute{
"services": schema.SingleNestedAttribute{
Copy link
Member

Choose a reason for hiding this comment

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

what is services adding as opposed to not having it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Here I am sticking to the structure defined in the API. From the scope doc:

We will design the API schema in a way that's extensible (similar to the way SRE is designing theirs) so we can just add the bare minimum for now and future projects can add more.

I believe sticking to this structure will avoid breaking changes in the future as more services are added. Also synced with Zuhair on this in the jira comments.

"services": schema.SingleNestedAttribute{
Attributes: map[string]schema.Attribute{
"clusters": schema.ListNestedAttribute{
NestedObject: schema.NestedAttributeObject{
Copy link
Member

Choose a reason for hiding this comment

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

it seems like IP addresses are associated to clusters in the project? wouldn't it make more sense then that IP addresses are returned in the clustest/adv_cluster resource instead of project? or in addition to be returned in project?

Copy link
Member Author

Choose a reason for hiding this comment

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

This a fair point. The main reason I see for including in the project resource is due to the endpoint being tagged in the project section (https://www.mongodb.com/docs/atlas/reference/api-resources-spec/v2/#tag/Projects/operation/returnAllIPAddresses).

{
TeamID: types.StringValue("teamId"),
RoleNames: roleList,
},
}
teamsTFSet, _ = types.SetValueFrom(context.Background(), project.TfTeamObjectType, []project.TFTeamModel{
Copy link
Member

@lantoli lantoli Jan 18, 2024

Choose a reason for hiding this comment

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

shouldn't we check errors or it's ok because it's in tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

not concerned for creating the test value, however adjusted model functions to return diags and check in the unit tests that no errors are encountered.

@AgustinBettati AgustinBettati marked this pull request as ready for review January 19, 2024 11:10
@AgustinBettati AgustinBettati requested a review from a team as a code owner January 19, 2024 11:10
@AgustinBettati AgustinBettati merged commit 5b34f46 into master Jan 19, 2024
37 checks passed
@AgustinBettati AgustinBettati deleted the CLOUDP-215106 branch January 19, 2024 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants