refactor: fabric connection and fabric port schema updates #548

thogarty · 2024-01-31T07:15:19Z

Consolidated+Migrated Fabric Connection Schema to Fabric Connection Resource
Refactored Common Schema Elements to internal Package
Consolidated+Migrated Fabric Port Schema to Fabric Port Resource
Updated Service Profile, Routing Protocol, and Cloud Router Schema to use Common elements
Updated Out of Date Documentation to match current changes
Moved mapping helpers to their corresponding resource + common mapping helpers to internal package

codecov-commenter · 2024-01-31T07:17:42Z

Codecov Report

Attention: 467 lines in your changes are missing coverage. Please review.

Comparison is base (a60c71f) 44.71% compared to head (1ff8a47) 44.37%.
Report is 27 commits behind head on main.

Files	Patch %	Lines
...ternal/resources/metal/project_ssh_key/resource.go	0.00%	131 Missing ⚠️
internal/resources/metal/ssh_key/resource.go	0.00%	130 Missing ⚠️
...rnal/resources/metal/project_ssh_key/datasource.go	0.00%	61 Missing ⚠️
...nternal/resources/metal/ssh_key/resource_schema.go	0.00%	45 Missing ⚠️
internal/resources/metal/project_ssh_key/models.go	0.00%	20 Missing ⚠️
equinix/resource_fabric_port.go	95.00%	12 Missing and 6 partials ⚠️
internal/resources/metal/metal_connection/speed.go	0.00%	16 Missing ⚠️
...resources/metal/project_ssh_key/resource_schema.go	0.00%	10 Missing ⚠️
internal/resources/metal/ssh_key/models.go	0.00%	9 Missing ⚠️
equinix/fabric_mapping_helper.go	62.50%	6 Missing ⚠️
... and 5 more

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #548      +/-   ##
==========================================
- Coverage   44.71%   44.37%   -0.35%     
==========================================
  Files          98       98              
  Lines       19888    18804    -1084     
==========================================
- Hits         8893     8344     -549     
+ Misses      10961    10378     -583     
- Partials       34       82      +48

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

srushti-patl

LGTM!

displague · 2024-02-01T01:03:00Z

docs/data-sources/equinix_fabric_cloud_router.md

@@ -25,15 +25,16 @@ data "equinix_fabric_cloud_router" "cloud_router_data_name" {

 ### Optional

+- `project` (Block Set) Project information (see [below for nested schema](#nestedblock--project))


This implies:

data "equinix_fabric_cloud_router" "cloud_router_data_name" { project { project_id = "<uuid_of_project>" # or href = "<href_of_project>" } }

How does a FCR lookup by project work? Is there a one FCR resource per project limitation? Does the datasource error if there are more than one FCR in the selected project? Is that a reasonable launch filter that will need to be augmented with additional filters in the future (name, location, feature based?)

Looking a little closer, it appears that project is not used when looking up FCR resources, only the uuid is used.

I think what is intended is that the datasource would have uuid required (which it is not) and project would be computed (as it is) but not optional.

Great catch. Agree completely. Will update.

displague · 2024-02-01T01:27:30Z

equinix/resource_fabric_connection.go

@@ -18,6 +19,574 @@ import (
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 )

+func FabricConnectionResourceSchema() map[string]*schema.Schema {


This was previously a private in the package: createFabricConnectionResourceSchema. This (lowercase naming) is preferable since we don't want or expect outside Go packages to depend on this function. Removing exported functions at a later date is a breaking change.

In the future migration to internal/resources/fabric/connection/ (perhaps internal/services/fabric/connection/) the function could be exported and shared between other packages in the Terraform provider without including the function in the public signature of the github.com/equinix/terraform-provider-equinix/equinix package. That package will minimally expose the Provider (which Terraform dependent integrations like Pulumi and Crossplane have utilized at some point or another).

Removing exported functions at a later date is a breaking change

because this project was released at 1.0.0 in the past.

https://0ver.org/ 🙂

Copy that. Heard and will fix here and take that review forward into further Provider changes.

displague · 2024-02-01T01:32:51Z

equinix/resource_fabric_service_profile.go

+		"integration_id": {
+			Type:        schema.TypeString,
+			Computed:    true,
+			Description: "Integration id",


Curious what this field is used for. The description doesn't provide more information than the name.

Same comment about bandwidth_from_api (and several fields in the adjacent helpers where the description is a s/_/ / transformation of the name.

For this one, and some of the others, the unfortunate answer is that I don't think they actually are useful to the terraform users. They are apart of the Fabric API schema, but are not commonly (if ever) used by a customer to do anything. Difficult to get more information on them but I will double check.

For things like that I would actually like to remove them, but there would be a breaking change as you referenced because we've published v1.0.0 already.

Nevermind, I found the details needed to provide a better description. Link here for reference: https://developer.equinix.com/docs?page=/dev-docs/fabric/overview

Updating to reflect it.

displague · 2024-02-01T01:36:12Z

docs/data-sources/equinix_fabric_connection.md

@@ -62,15 +63,16 @@ Read-Only:

 - `account` (Set of Object) (see [below for nested schema](#nestedobjatt--a_side--access_point--account))
 - `authentication_key` (String)
- **Deprecated** `gateway` Use `router` instead (Set of Object) (see [below for nested schema](#nestedobjatt--a_side--access_point--router))
- `router` CloudRouter; Replaces `gateway` attribute (Set of Object) (see [below for nested schema](#nestedobjatt--a_side--access_point--router))
+- `gateway` (Set of Object) (see [below for nested schema](#nestedobjatt--a_side--access_point--gateway))


Curious how we lost the deprecation on this attribute. Users should continue to be encouraged to read from the router rather than the gateway.

Ah yes, I had fixed that in the resource, but forgot the data_source. Will update. Good comment.

ctreatma · 2024-02-01T15:18:40Z

internal/schema/set_map.go

@@ -20,9 +20,6 @@ func SetMap(d *schema.ResourceData, m map[string]interface{}) error {
 		if f, ok := v.(setFn); ok {
 			err = f(d, key)
 		} else {
-			if key == "router" {


ctreatma · 2024-02-01T15:18:52Z

internal/schema/resourcedata.go

@@ -2,7 +2,7 @@ package schema

 import "reflect"

-// resourceDataProvider provies interface to schema.ResourceData
+// resourceDataProvider proxies interface to schema.ResourceData


ctreatma · 2024-02-01T15:25:45Z

internal/schema/fabric_common_mapping_helpers.go

+	return order
+}
+
+func OrderToTerra(order *v4.Order) *schema.Set {


Since you're moving these fabric_common_ files anyway, I recommend creating a separate directory/package for them, perhaps:

internal/fabric/schema/mapping.go

internal/fabric/schema/schemas.go

Having it in a separate package helps clarify that this OrderToTerra, for example, is only useful in the context of Fabric integrations and should probably not be modified by other teams.

Yea, good point. Will make that change.

thogarty · 2024-02-01T23:13:00Z

@ctreatma and @displague, responded to feedback. Should be good to go for a follow up review. Appreciate the comments 👍

Additionally, anything that we're missing in here that you would like to have updated. I would like to push that to a fast follow if it can wait a little bit. We're trying to do the refactor first just to get Fabric in order so that we can update attribute behaviors+descriptions in a much lower cognitive overhead environment. Doing the untangling with the precision of improving the missing details has been challenging.

… gateway to terra state

…a package name

ctreatma

👍 on the changes to the common files

srushti-patl

LGTM

thogarty requested review from a team as code owners January 31, 2024 07:15

thogarty had a problem deploying to internal January 31, 2024 07:15 — with GitHub Actions Error

thogarty temporarily deployed to internal January 31, 2024 07:15 — with GitHub Actions Inactive

thogarty temporarily deployed to internal February 1, 2024 00:08 — with GitHub Actions Inactive

thogarty force-pushed the CXF-83370-schema-updates-port-connection branch from aaf22da to d389876 Compare February 1, 2024 00:08

thogarty temporarily deployed to internal February 1, 2024 00:08 — with GitHub Actions Inactive

thogarty temporarily deployed to internal February 1, 2024 00:22 — with GitHub Actions Inactive

thogarty temporarily deployed to internal February 1, 2024 00:27 — with GitHub Actions Inactive

thogarty temporarily deployed to internal February 1, 2024 00:40 — with GitHub Actions Inactive

srushti-patl previously approved these changes Feb 1, 2024

View reviewed changes

thogarty requested a review from a team February 1, 2024 00:49

displague reviewed Feb 1, 2024

View reviewed changes

thogarty dismissed srushti-patl’s stale review via 5047e1f February 1, 2024 01:26

thogarty temporarily deployed to internal February 1, 2024 01:26 — with GitHub Actions Inactive

displague reviewed Feb 1, 2024

View reviewed changes

ctreatma reviewed Feb 1, 2024

View reviewed changes

thogarty temporarily deployed to internal February 1, 2024 23:08 — with GitHub Actions Inactive

thogarty force-pushed the CXF-83370-schema-updates-port-connection branch from 1b2bc81 to 126c37a Compare February 1, 2024 23:10

thogarty temporarily deployed to internal February 1, 2024 23:10 — with GitHub Actions Inactive

thogarty temporarily deployed to internal February 2, 2024 21:39 — with GitHub Actions Inactive

thogarty added 14 commits February 2, 2024 16:21

Refactor Connection Schemas

b59754a

Refactor Port and Connection Schemas + mapping helpers

135eadb

Refactor Service Profile schema and connection schema

39c6db8

Update computed attributes for port and connection

22caa53

Modify Ports attribute behaviors

90b9fce

Update data_sources schema docs after refactor

0af08e2

Update resource docs after schema refactor

9ebf97b

Remove beta notices from fabric resources/data_sources

be6fe0f

Update small doc updates that were getting overwritten and setting of…

0a30aa7

… gateway to terra state

Add missing connection types to validation

ee990ee

Fix typo in list creation

e0fe15d

Fix failing empty plan on acceptance test

2750905

Add VD to Azure example and update interface id in docs for connection

edc81d6

Update schema descriptions, attr behaviors, and refactor common schem…

1ff8a47

…a package name

ctreatma force-pushed the CXF-83370-schema-updates-port-connection branch from 126c37a to 1ff8a47 Compare February 2, 2024 22:21

ctreatma temporarily deployed to internal February 2, 2024 22:21 — with GitHub Actions Inactive

ctreatma approved these changes Feb 2, 2024

View reviewed changes

srushti-patl approved these changes Feb 2, 2024

View reviewed changes

thogarty merged commit aff0633 into main Feb 2, 2024
6 of 9 checks passed

thogarty deleted the CXF-83370-schema-updates-port-connection branch February 2, 2024 22:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: fabric connection and fabric port schema updates #548

refactor: fabric connection and fabric port schema updates #548

thogarty commented Jan 31, 2024

codecov-commenter commented Jan 31, 2024 •

edited

Loading

srushti-patl left a comment

displague Feb 1, 2024 •

edited

Loading

displague Feb 1, 2024 •

edited

Loading

thogarty Feb 1, 2024

displague Feb 1, 2024 •

edited

Loading

displague Feb 1, 2024

thogarty Feb 1, 2024

displague Feb 1, 2024

displague Feb 1, 2024 •

edited

Loading

thogarty Feb 1, 2024

thogarty Feb 1, 2024 •

edited

Loading

displague Feb 1, 2024 •

edited

Loading

thogarty Feb 1, 2024

ctreatma Feb 1, 2024

ctreatma Feb 1, 2024

ctreatma Feb 1, 2024

thogarty Feb 1, 2024

thogarty commented Feb 1, 2024

ctreatma left a comment

srushti-patl left a comment

		@@ -25,15 +25,16 @@ data "equinix_fabric_cloud_router" "cloud_router_data_name" {

		### Optional

		- `project` (Block Set) Project information (see [below for nested schema](#nestedblock--project))

refactor: fabric connection and fabric port schema updates #548

refactor: fabric connection and fabric port schema updates #548

Conversation

thogarty commented Jan 31, 2024

codecov-commenter commented Jan 31, 2024 • edited Loading

Codecov Report

srushti-patl left a comment

Choose a reason for hiding this comment

displague Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

displague Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

displague Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

displague Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thogarty Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

displague Feb 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thogarty commented Feb 1, 2024

ctreatma left a comment

Choose a reason for hiding this comment

srushti-patl left a comment

Choose a reason for hiding this comment

codecov-commenter commented Jan 31, 2024 •

edited

Loading

displague Feb 1, 2024 •

edited

Loading

displague Feb 1, 2024 •

edited

Loading

displague Feb 1, 2024 •

edited

Loading

displague Feb 1, 2024 •

edited

Loading

thogarty Feb 1, 2024 •

edited

Loading

displague Feb 1, 2024 •

edited

Loading