feat: Adding Marketplace Subscription capability to Fabric Cloud Router Resource #762

srushti-patl · 2024-08-27T17:52:42Z

Added marketplace_subscription schema to fabric_cloud_router resource
Marked 'account' schema as optional and computed because this parameter is optional while creating Fabric Cloud Router using Marketplace Subscription id.

…er Resource

codecov-commenter · 2024-08-27T21:26:24Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 46.55172% with 31 lines in your changes missing coverage. Please review.

Project coverage is 34.43%. Comparing base (4b6af14) to head (fcaef23).
Report is 3 commits behind head on main.

Files	Patch %	Lines
equinix/resource_fabric_cloud_router.go	46.55%	31 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #762      +/-   ##
==========================================
+ Coverage   34.38%   34.43%   +0.04%     
==========================================
  Files         154      154              
  Lines       21080    21116      +36     
==========================================
+ Hits         7249     7271      +22     
- Misses      13670    13686      +16     
+ Partials      161      159       -2

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

thogarty

This PR also needs the document changes to the schema for the cloud router resource. We should also update the Cloud Router data source schemas for the same changes to allow users to grab that data as well.

We should merge Charles' docs PR first

thogarty · 2024-08-30T06:08:36Z

equinix/resource_fabric_cloud_router.go

@@ -273,6 +313,10 @@ func resourceFabricCloudRouterCreate(ctx context.Context, d *schema.ResourceData
 	package_ := packageCloudRouterTerraformToGo(schemaPackage)
 	createCloudRouterRequest.SetPackage(package_)

+	schemaMarketplaceSubscription := d.Get("marketplace_subscription").(*schema.Set).List()
+	marketplaceSubscription := marketplaceSubscriptionCloudRouterTerraformToGo(schemaMarketplaceSubscription)
+	createCloudRouterRequest.SetMarketplaceSubscription(marketplaceSubscription)


Should only set the subscription if the value is provided. Do a reflect deep equal check on empty return struct to avoid empty value in JSON request body.

thogarty · 2024-08-30T06:09:45Z

equinix/resource_fabric_cloud_router.go

+	}
+	marketplaceSubscription := fabricv4.MarketplaceSubscription{}
+	marketplaceSubscriptionMap := marketplaceSubscriptionTerraform[0].(map[string]interface{})
+	subscriptionUUID := marketplaceSubscriptionMap["uuid"].(string)


You added type as Optional/Computed but you aren't accounting for type as an input here. Additionally, do empty string checks before setting, and use setters instead of direct assignment on attributes.

thogarty · 2024-08-30T06:11:40Z

equinix/resource_fabric_cloud_router.go

+	return map[string]*schema.Schema{
+		"type": {
+			Type:        schema.TypeString,
+			Computed:    true,


Just double checking, you can send type and uuid in the request AND they will be returned in the response if not provided, correct? We need to be spot on with attribute behaviors to prepare for terraform-plugin-framework migration when we move to tf-plugin-protocol v6

yes, we can send type and UUID in the request.
UUID is required parameter in marketplacesubscription filed. I have marked it as required in schema definition.
we will get the type in response if it's not provided.

thogarty · 2024-08-30T06:11:59Z

equinix/resource_fabric_cloud_router.go

 		"account": {
 			Type:        schema.TypeSet,
-			Required:    true,
+			Optional:    true,


Same question about account. I believe this is correct but double checking.

account number is required in the account filed and I have marked it as required in schema definition.

…rketplace subscription

thogarty

Last changes requested. Other than that it looks good.

thogarty · 2024-09-10T18:45:39Z

templates/resources/fabric_cloud_router.md.tmpl

@@ -0,0 +1,29 @@
+---
+# generated by https://github.com/hashicorp/terraform-plugin-docs


nit: Unnecessary change. You can remove lines 2, 3, 5, and 6. They aren't being added by the default implementation and aren't needed.

thogarty · 2024-09-10T18:47:03Z

templates/resources/fabric_cloud_router.md.tmpl

+## Example Usage
+
+Fabric Cloud Router
+{{tffile "examples/resources/equinix_fabric_cloud_router/example_1.tf"}}


So I don't know how the resources/fabric_cloud_router.md got updated correctly based on how you've oriented the examples in this template. The Code shows that example_1.tf is for the marketplace subscription and example_2.tf is for the standard cloud router. Could you run this again and see if it shows a diff? I think there's a disconnect here.

yes, I will rename example files.

thogarty

LGTM. Ship it

feat: Adding Marketplace Subscription capability to Fabric Cloud Rout…

8df9fe0

…er Resource

srushti-patl requested a review from a team as a code owner August 27, 2024 17:52

srushti-patl temporarily deployed to internal August 27, 2024 17:52 — with GitHub Actions Inactive

Merge branch 'main' into CXF-100674-Create-FCR-Subscription

642d8af

srushti-patl temporarily deployed to internal August 27, 2024 17:52 — with GitHub Actions Inactive

fix: Resolving lint errors

1647b2a

srushti-patl temporarily deployed to internal August 27, 2024 19:13 — with GitHub Actions Inactive

fix: Resolving fmt lint error

fcaef23

srushti-patl temporarily deployed to internal August 27, 2024 21:23 — with GitHub Actions Inactive

ctreatma mentioned this pull request Aug 28, 2024

chore(docs): use default template where possible for Fabric docs #764

Merged

thogarty requested changes Aug 30, 2024

View reviewed changes

Merge branch 'main' into CXF-100674-Create-FCR-Subscription

313a967

srushti-patl temporarily deployed to internal September 9, 2024 19:02 — with GitHub Actions Inactive

fix: Updating FCR resource as per comments

b21dd1f

srushti-patl temporarily deployed to internal September 9, 2024 19:14 — with GitHub Actions Inactive

Merge branch 'main' into CXF-100674-Create-FCR-Subscription

d1bcfbe

srushti-patl temporarily deployed to internal September 9, 2024 21:55 — with GitHub Actions Inactive

fix: Updating docs for cloud router resource and resource based on ma…

ec886cd

…rketplace subscription

srushti-patl temporarily deployed to internal September 10, 2024 06:21 — with GitHub Actions Inactive

srushti-patl requested a review from thogarty September 10, 2024 06:22

thogarty requested changes Sep 10, 2024

View reviewed changes

feat: Updating docs

9dc48cd

srushti-patl temporarily deployed to internal September 10, 2024 20:07 — with GitHub Actions Inactive

thogarty approved these changes Sep 10, 2024

View reviewed changes

srushti-patl merged commit 692594c into main Sep 10, 2024
8 of 11 checks passed

srushti-patl deleted the CXF-100674-Create-FCR-Subscription branch September 10, 2024 20:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Adding Marketplace Subscription capability to Fabric Cloud Router Resource #762

feat: Adding Marketplace Subscription capability to Fabric Cloud Router Resource #762

srushti-patl commented Aug 27, 2024

codecov-commenter commented Aug 27, 2024

thogarty left a comment

thogarty Aug 30, 2024

thogarty Aug 30, 2024

thogarty Aug 30, 2024

srushti-patl Sep 9, 2024

thogarty Aug 30, 2024

srushti-patl Sep 9, 2024

thogarty left a comment

thogarty Sep 10, 2024

thogarty Sep 10, 2024

srushti-patl Sep 10, 2024

thogarty left a comment

		@@ -0,0 +1,29 @@
		---
		# generated by https://github.com/hashicorp/terraform-plugin-docs

feat: Adding Marketplace Subscription capability to Fabric Cloud Router Resource #762

feat: Adding Marketplace Subscription capability to Fabric Cloud Router Resource #762

Conversation

srushti-patl commented Aug 27, 2024

codecov-commenter commented Aug 27, 2024

Codecov Report

thogarty left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thogarty left a 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 left a comment

Choose a reason for hiding this comment