feat: Adding Route Aggregation Policy Resource and Data Source by srushti-patl · Pull Request #850 · equinix/terraform-provider-equinix

srushti-patl · 2025-02-04T06:45:40Z

Added the following using Terraform Plugin Framework:

equinix_fabric_route_aggregation_ resource
equinix_fabric_route_aggregation_data source
equinix_fabric_route_aggregations_data source
Included automated testing and docs

codecov-commenter · 2025-02-04T07:14:59Z

Codecov Report

Attention: Patch coverage is 49.93954% with 414 lines in your changes missing coverage. Please review.

Project coverage is 65.09%. Comparing base (291f58a) to head (4de8b90).
Report is 13 commits behind head on main.

Files with missing lines	Patch %	Lines
...rnal/resources/fabric/routeaggregation/resource.go	18.18%	159 Missing and 3 partials ⚠️
...ic/routeaggregation/datasource_all_aggregations.go	10.41%	86 Missing ⚠️
...ternal/resources/fabric/routeaggregation/models.go	0.00%	86 Missing ⚠️
...ernal/resources/fabric/routeaggregation/sweeper.go	0.00%	58 Missing ⚠️
...uteaggregation/datasource_by_routeAggregationId.go	31.25%	22 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #850       +/-   ##
===========================================
+ Coverage   34.53%   65.09%   +30.55%     
===========================================
  Files         192      199        +7     
  Lines       25223    26050      +827     
===========================================
+ Hits         8711    16957     +8246     
+ Misses      16365     8267     -8098     
- Partials      147      826      +679

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…-DataSource

thogarty

First review completed. See comments.

thogarty · 2025-02-21T00:48:24Z

+			continue
+		}
+
+		_, resp, err := client.RouteAggregationsApi.GetRouteAggregationByUuid(ctx, rs.Primary.ID).Execute()


You're making two api calls in this method when 1 will do. You can check various responses separately with the return values from one call.

Also, did you verify that a 400 is going to be a success on deletion? I've put that in streaming because of a design decision that could be a defect. I'm curious if route aggreation has the same.

Initially during the development, I had confirmed it with Qiong, 400 was a success criteria for Route Aggregation deletion.Recently they have updated it to 200, I have updated the code

thogarty · 2025-02-21T00:49:33Z

+			}
+
+			// Handle specific API error messages
+			var apiErr *fabricv4.GenericOpenAPIError


My intuition is telling me we have a better pattern for retrieving the API error code and doing a check. This seems very large for what it's achieving.

thogarty · 2025-02-21T00:51:11Z

+		return
+	}
+
+	resp.Diagnostics.Append(diags...)


This check doesn't seem to have any value. It's not checking any new diags values and is doing the same as the following statement. Can be removed.

thogarty · 2025-02-21T00:51:47Z

+		Op:    "replace",
+		Path:  "/name",
+		Value: map[string]interface{}{"": newName},
+	},


nit: could keep it on the same line

thogarty · 2025-02-21T00:56:40Z

+	return diags
+}
+
+func parseRouteAggregation(ctx context.Context, routeAggregation *fabricv4.RouteAggregationsData,


We're removing this extra function because it's not being used repeatedly in a way that is effective. See #844 (comment)

thogarty · 2025-02-21T00:58:16Z

+					resource.TestCheckResourceAttr("data.equinix_fabric_route_aggregations.data_ras", "pagination.limit", "2"),
+					resource.TestCheckResourceAttr("data.equinix_fabric_route_aggregations.data_ras", "pagination.offset", "1"),
+				),
+				ExpectNonEmptyPlan: false,


This statement actually is already the default so it can be omitted. It's only when we set this value to true that we're giving the test permission to see changes post apply.

thogarty · 2025-02-21T00:58:33Z

+					resource.TestCheckResourceAttr("equinix_fabric_route_aggregation.test", "type", "BGP_IPv4_PREFIX_AGGREGATION"),
+					resource.TestCheckResourceAttr("equinix_fabric_route_aggregation.test", "description", "Test Route Aggregation"),
+				),
+				ExpectNonEmptyPlan: false,


Same as datasources_test comment.

thogarty · 2025-02-21T00:59:36Z

 ---

-# Equinix Provider
+# terraform-provider-equinix Provider


There's an issue with the make docs run here for some reason. This shouldn't be a change that occurs.

thogarty · 2025-02-21T01:01:45Z

+---
+
+# equinix_fabric_route_aggregation (Resource)
+


Missing the description in the resource schema that would appear as the header in this doc.

…equest

thogarty

Changes requested.

thogarty · 2025-02-24T23:55:47Z

+		return
+	}
+
+	if diags.HasError() {


This whole statement needed to be removed. Not just the line before it.

thogarty · 2025-02-24T23:57:57Z

+			string(fabricv4.ROUTEAGGREGATIONSTATE_DEPROVISIONING),
+		},
+		Target: []string{
+			string(fabricv4.ROUTEAGGREGATIONSTATE_DEPROVISIONED),


deletedMarker was removed here during your update. Needs to be present.

thogarty · 2025-02-25T00:06:10Z

+	m.RouteAggregationID = types.StringValue(routeAggregation.GetUuid())
+	m.ID = types.StringValue(routeAggregation.GetUuid())
+
+	if routeAggregation != nil {


Redo models. Leverage struct embedding principles to leverage a single line parse statement.

thogarty · 2025-02-25T00:07:40Z

+	client := r.Meta.NewFabricClientForFramework(ctx, req.ProviderMeta)
+
+	createRequest, diags := buildCreateRequest(ctx, plan)
+	if diags.HasError() {


Needs to append to resp.Diagnostics before returning.

thogarty

LGTM. Ship it.

github-actions · 2025-02-28T18:42:13Z

This PR is included in version 3.3.0 🎉

feat: Adding Route Aggregation Policy Resource and Data Source

d47d1a5

srushti-patl requested review from a team as code owners February 4, 2025 06:45

srushti-patl temporarily deployed to internal February 4, 2025 06:45 — with GitHub Actions Inactive

fix: adding route aggregation model file

b1edf8d

srushti-patl temporarily deployed to internal February 4, 2025 07:13 — with GitHub Actions Inactive

fix: Fixing lint errors

d7cc83c

srushti-patl temporarily deployed to internal February 4, 2025 20:32 — with GitHub Actions Inactive

fix: Updating datasource test file

c248a7e

srushti-patl temporarily deployed to internal February 6, 2025 23:32 — with GitHub Actions Inactive

fix: Updating datasource test file

fde74f1

srushti-patl temporarily deployed to internal February 6, 2025 23:37 — with GitHub Actions Inactive

Merge branch 'main' into CXF-106515-Route-Aggregation-Policy-Resource…

83c7a56

…-DataSource

srushti-patl temporarily deployed to internal February 6, 2025 23:39 — with GitHub Actions Inactive

thogarty added the area/resources/fabric Issues related to Fabric and ECX APIs label Feb 20, 2025

thogarty requested changes Feb 21, 2025

View reviewed changes

srushti-patl added 2 commits February 21, 2025 14:42

fix: Adding patch for Route Aggregation and Route Aggregation Patch R…

292bce6

…equest

fix: Updating PR comments and Resolving merge conflicts

4fd4942

srushti-patl temporarily deployed to internal February 24, 2025 20:07 — with GitHub Actions Inactive

fix: resolving merge conflicts

73faeda

srushti-patl temporarily deployed to internal February 24, 2025 22:31 — with GitHub Actions Inactive

fix: Fixing lint erros

a38363c

srushti-patl temporarily deployed to internal February 24, 2025 23:40 — with GitHub Actions Inactive

srushti-patl requested a review from thogarty February 24, 2025 23:49

thogarty requested changes Feb 25, 2025

View reviewed changes

fix: Updating PR comments

4de8b90

srushti-patl temporarily deployed to internal February 25, 2025 01:02 — with GitHub Actions Inactive

srushti-patl requested a review from thogarty February 25, 2025 01:03

thogarty approved these changes Feb 25, 2025

View reviewed changes

srushti-patl merged commit 90a8e38 into main Feb 25, 2025

srushti-patl deleted the CXF-106515-Route-Aggregation-Policy-Resource-DataSource branch February 25, 2025 01:13

Conversation

srushti-patl commented Feb 4, 2025

Uh oh!

codecov-commenter commented Feb 4, 2025 • edited by codecov Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thogarty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srushti-patl Feb 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thogarty left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thogarty left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Feb 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov-commenter commented Feb 4, 2025 •

edited by codecov Bot

Loading

srushti-patl Feb 24, 2025 •

edited

Loading