feat: Fabric stream resource and data sources by thogarty · Pull Request #844 · equinix/terraform-provider-equinix

thogarty · 2025-01-24T06:01:04Z

Using terraform-plugin-framework; added the following:

equinix_fabric_stream resource
equinix_fabric_stream data source
equinix_fabric_streams data source

Included automated testing and docs creation leveraging description on schemas

NOTE: Fixed usage of context within Fabric client creations and updated all usages in SDKv2 as well

ctreatma · 2025-01-27T16:54:20Z

+		ClientSecret: c.ClientSecret,
+		BaseURL:      c.BaseURL,
+	}
+	authClient := authConfig.New(ctx)


Can you remind me why this change is needed? Continuing to use a shared auth client across all API clients would be ideal, but IIRC there's an issue with the shared client using a canceled context, maybe? We should make sure those issue details are captured somewhere so we can work on replacing or at least refactoring the Equinix oauth2-go library in a way that allows sharing auth across clients.

So before we had a different change outlined in the EPT draft: https://github.com/equinix/terraform-provider-equinix/pull/745/files#diff-54c7c1af5fa8d5db4dc49f0e8e80e93ba2b1183ba4d5c9e2e5729e6deae6a3cd

We discussed a few things. One of them was if we were going to use the context in both that we should update all callers (like we are doing here) and the other was to keep the old method along with the new one.

The issue with not passing context for Framework is that there's a canceled context in the config when using the Interface methods that requires passing context to complete those flows.

My theory is that for SDKv2 they shared the same context as the config long enough that it wasn't an issue; but the change resolves that even if it was separate because each lifecycle method passes a usable context into it.

TL;DR, Framework requires context. SDKv2 didn't but changing it so that it uses it doesn't create a problem.

Ah, the framework issue is the one I was thinking of. While this change replicates what is happening for framework, which means it's not necessarily introducing new behavior, my concern is that we're creating a bunch of separate auth clients that are going to be using the same client ID and secret but not using the same token. The SDKv2 clients previously shared the same auth client, which reduces the number of token requests across resources, but because of how equinix/oauth2-go is written, the auth client needs a context. The framework doesn't have a suitable context for that, so we got those canceled context errors. It's not necessarily a problem to replicate the framework behavior in SDKv2, but going all-in on duplicate auth clients means we'll be making even more unnecessary token requests (two resources using the same client ID & secret each have to make their own token request even though their tokens will have the same permissions). Duplicate requests could become a problem (or at least a complication) if, for example, rate limits are introduced on the token endpoint. Long-term it would be preferable to change our approach to auth clients in the terraform provider so that tokens can be reused across API clients.

To be clear I'm on board with this change as-is; we might have to standardize on accepting a context anyway in order to make the optimization I described, whenever that time comes.

Ok, I was able to follow this explanation through to the end on this one. Initially my confusion on context was distracting me from the token issue; I only partially understood what might have been happening. Now that I've got a better handle on the context cancellation issue the auth client separation per resource is clear.

We're on the same page now. Thank you for a very thorough explanation, @ctreatma!

d-bhola · 2025-01-29T17:38:06Z

+  }
+}
+
+output "stream_state" {


Do we need more outputs in this data source ?

It's a resource, and no. We can limit the amount that we provide outputs in examples for maintenance reasons. We just need some display of attribute selection.

d-bhola · 2025-01-29T17:38:34Z

+  }
+}
+
+output "number_of_returned_streams" {


Same here. Do we need to add any other outputs here?

Same explanation for output.

d-bhola · 2025-01-29T17:49:21Z

+					resource.TestCheckResourceAttr("data.equinix_fabric_streams.data_streams", "pagination.limit", "2"),
+					resource.TestCheckResourceAttr("data.equinix_fabric_streams.data_streams", "pagination.offset", "1"),
+				),
+				ExpectNonEmptyPlan: true,


If the plan is empty, this test will fail. ExpectNonEmptyPlan: false would keep it consistent with other tests (for other resources)

This is set specifically to allow the test to pass for my use case. I do expect a non-empty plan after my tests run. You can actually omit ExpectNonEmptyPlan: false as well because that's the default value for that config item.

Okay. Sounds good.

d-bhola · 2025-01-29T17:59:46Z

+	}
+	*project = fwtypes.NewObjectValueOf[ProjectModel](ctx, &projectModel)
+
+	const TIMEFORMAT = "2006-01-02T15:04:05.000Z"


The naming convention in Go for unexported constants is camelCase. const timeFormat

Learned something new! There are other places where we use the non-idiomatic Go styling for constants so we can update them with the same reference as well.

Good to cite it also: https://google.github.io/styleguide/go/guide.html#mixedcaps

If we're adopting or changing coding standards, that should be reflected in the linter settings to enforce convergence on the chosen style.

Ok, so I went through this thoroughly. I was mistaken, we don't really have any other places that all caps were used for variable names. We have camelCase throughout.

Additionally, even when adding stylecheck to the linter it doesn't care about all caps in a variable name. There isn't a Go style descriptor against all caps.

I'm going to leave it out of the linter for now, and I'll move this into the internal/fabric package so that it can be reused, because we'll have it in several places for the Framework implementations of fabric's new resources.

Update made 👍

Ah, it's running through all files because of the config change...

We're bottle necking a bit here because of this. All files introduced by this change pass the linters. The files changed for config are not compliant.

I'm going to remove the linter update for this one, then add in separate PR. Then when we are poking around for one-off fixes or updates from SDKv2 to Framework we can make necessary updates. It's too much time for the current scope we're on.

Opened #849

@d-bhola + @ctreatma ready for final review 👍

Ah, it's running through all files because of the config change...

This is true of the make lint task for running linters locally, but not the case for PR validation in GitHub (if the config change were made in this PR, the GitHub Actions workflow would only lint the changes in this PR, not the full codebase...but personally I still think it's better to update config separately). Last year, the plan was to eventually make PR validation lint the full codebase, but IMO linter config changes are a good reason to keep the limited PR linting indefinitely, in which case we may want to figure out how to update the local make lint task to more closely match the PR validation behavior.

Sorry, I should have been more clear. It's not linting the entire project, but because we updated all of the SDKv2 Fabric resources/datasources files to use the config.go update change to pass their context in; all of those files are being linted also even though they're not the target of this PR directly.

So to resolve all Fabric linter errors in this project is overkill for this PR. Which is why I've separated out the linter to a different PR while also ensuring the new Streams additions are adherent to it by running make lint locally.

d-bhola · 2025-01-29T18:06:00Z

+
+func testSweepStreams(region string) error {
+	var errs []error
+	log.Printf("[DEBUG] Sweeping Fabric Streams")


Do we need this logging statement here ? If it's for debugging, should we remove it ?

In test output it will show up and provide useful data for the state of sweepers when run.

d-bhola · 2025-01-29T18:08:11Z

+
+	for _, stream := range streams.GetData() {
+		if sweep.IsSweepableFabricTestResource(stream.GetName()) {
+			log.Printf("[DEBUG] Deleting stream: %s", stream.GetName())


Same here. Do we need this logging statement here ?

Same explanation; In test output it will show up and provide useful data for the state of sweepers when run.

d-bhola · 2025-01-29T18:09:28Z

Needs rebase

…fications

d-bhola

LGTM

codecov-commenter · 2025-01-31T01:39:25Z

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

Codecov Report

Attention: Patch coverage is 2.18712% with 805 lines in your changes missing coverage. Please review.

Project coverage is 33.96%. Comparing base (c71308e) to head (a8f8962).
Report is 24 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/resources/fabric/stream/resource.go	0.00%	204 Missing ⚠️
...rnal/resources/fabric/stream/datasources_schema.go	0.00%	164 Missing ⚠️
...nternal/resources/fabric/stream/resource_schema.go	0.00%	125 Missing ⚠️
internal/resources/fabric/stream/models.go	0.00%	121 Missing ⚠️
.../resources/fabric/stream/datasource_all_streams.go	0.00%	41 Missing ⚠️
.../resources/fabric/stream/datasource_by_streamid.go	0.00%	34 Missing ⚠️
internal/resources/fabric/stream/sweeper.go	0.00%	33 Missing ⚠️
internal/config/config.go	52.63%	9 Missing ⚠️
equinix/resource_fabric_service_profile.go	0.00%	8 Missing ⚠️
equinix/resource_fabric_cloud_router.go	0.00%	7 Missing ⚠️
... and 19 more

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #844      +/-   ##
==========================================
- Coverage   36.13%   33.96%   -2.18%     
==========================================
  Files         184      187       +3     
  Lines       24491    24846     +355     
==========================================
- Hits         8850     8438     -412     
- Misses      15480    16283     +803     
+ Partials      161      125      -36

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

…ations in as many files as this change is touching

ctreatma · 2025-02-03T16:33:55Z

 issues:
  max-issues-per-linter: 0
-  max-same-issues: 0
+  max-same-issues: 0


This whitespace change in the linter config is tagging DRE for review; if you revert this change then you'll only need review/approval from @d-bhola.

Oh wait, I'm wrong; the changes to config.go are technically shared, so that requires separate sign-off either way; taking care of that now.

ctreatma · 2025-02-06T15:47:46Z

+func parseStream(ctx context.Context, stream *fabricv4.Stream,
+	streamType, name, description, href, uuid, state *basetypes.StringValue,
+	assetsCount, streamSubscriptionCount *basetypes.Int32Value,
+	project *fwtypes.ObjectValueOf[ProjectModel],
+	changeLog *fwtypes.ObjectValueOf[ChangeLogModel]) diag.Diagnostics {


I missed this at review time, but this pattern of defining a shared parseThing(...) function for separate models should not be followed any more. It was a workaround for the fact that terraform-plugin-framework did not support embedded structs when we started adopting it. The framework supports embedded structs now, which means we can reuse the thing.parse(data) function across different model types for more idiomatic Go code. #853 is an example of that change.

Copy that! Will follow.

github-actions · 2025-02-21T00:16:39Z

This PR is included in version 3.2.0 🎉

Using terraform-plugin-framework; added the following: * equinix_fabric_stream resource * equinix_fabric_stream data source * equinix_fabric_streams data source Included automated testing and docs creation leveraging description on schemas NOTE: Fixed usage of context within Fabric client creations and updated all usages in SDKv2 as well

#844 updated the `Config` object to create a new auth client every time a new Fabric API client was created for SDK resources. If we stuck with this pattern, we would have to duplicate that behavior for every Equinix API client except for Metal. In addition, this pattern means that the provider will make a separate auth token request per resource rather than reusing valid tokens across all resources that use the same provider (and therefore auth) config. This reverts the auth client change by adopting a custom HTTP transport and OAuth token source that enables passing the original request context for an API request to the auth token client. As a result, we no longer need to use `equinix/oauth2-go`.

thogarty requested review from a team as code owners January 24, 2025 06:01

thogarty temporarily deployed to internal January 24, 2025 06:01 — with GitHub Actions Inactive

thogarty temporarily deployed to internal January 24, 2025 23:29 — with GitHub Actions Inactive

thogarty force-pushed the fabric_stream_resource_and_data_sources branch from c4dfeea to a603dc5 Compare January 24, 2025 23:30

thogarty temporarily deployed to internal January 24, 2025 23:30 — with GitHub Actions Inactive

thogarty temporarily deployed to internal January 24, 2025 23:35 — with GitHub Actions Inactive

thogarty temporarily deployed to internal January 24, 2025 23:38 — with GitHub Actions Inactive

thogarty temporarily deployed to internal January 25, 2025 01:10 — with GitHub Actions Inactive

ctreatma reviewed Jan 27, 2025

View reviewed changes

d-bhola reviewed Jan 29, 2025

View reviewed changes

thogarty added 7 commits January 29, 2025 14:24

Update Fabric Client config and update sdkv2 calls to it

087abf5

Add equinix_fabric_stream resource and data sources

eaf9224

Add Equinix Fabric Stream Resource and Data Sources

ccd6c8d

Add acceptance tests to streams resource and data source

f1cca20

Add docs for fabric stream resource and data sources

2f8b5ae

Finish stream resource and data source dev

34215ae

Fix linter error; remove fmt.Sprintf

d1646bd

thogarty added 3 commits January 29, 2025 14:24

Fix variable declaration and assignment linting errors

28e84c8

Add test sweeper for stream resource

955f9c0

Update const to match Go styling guide and update tests for plan modi…

f485a2b

…fications

thogarty force-pushed the fabric_stream_resource_and_data_sources branch from b8a6440 to f485a2b Compare January 29, 2025 22:49

thogarty temporarily deployed to internal January 29, 2025 22:49 — with GitHub Actions Inactive

d-bhola previously approved these changes Jan 29, 2025

View reviewed changes

Update reference to const timeout format

2f03c65

thogarty dismissed d-bhola’s stale review via 2f03c65 January 31, 2025 01:34

thogarty temporarily deployed to internal January 31, 2025 01:34 — with GitHub Actions Inactive

ctreatma mentioned this pull request Jan 31, 2025

chore(ci): perform full linting on all PRs #840

Closed

Add revive and stylecheck linters + update linting errors

a8f8962

thogarty temporarily deployed to internal January 31, 2025 22:38 — with GitHub Actions Inactive

Remove styling additions to move it to a separate PR to avoid modific…

36a88c8

…ations in as many files as this change is touching

thogarty temporarily deployed to internal January 31, 2025 22:53 — with GitHub Actions Inactive

ctreatma reviewed Feb 3, 2025

View reviewed changes

d-bhola approved these changes Feb 3, 2025

View reviewed changes

ctreatma approved these changes Feb 3, 2025

View reviewed changes

thogarty merged commit fc4011e into main Feb 3, 2025

thogarty deleted the fabric_stream_resource_and_data_sources branch February 3, 2025 22:13

ctreatma mentioned this pull request Feb 4, 2025

chore: use original API request context for auth token requests #847

Merged

ctreatma reviewed Feb 6, 2025

View reviewed changes

thogarty mentioned this pull request Feb 21, 2025

feat: Adding Route Aggregation Policy Resource and Data Source #850

Merged

Conversation

thogarty commented Jan 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ctreatma Jan 27, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thogarty Feb 3, 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!

d-bhola commented Jan 29, 2025

Uh oh!

d-bhola left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Jan 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

github-actions Bot commented Feb 21, 2025

Uh oh!

Reviewers

Assignees

Labels

thogarty commented Jan 24, 2025 •

edited

Loading

ctreatma Jan 27, 2025 •

edited

Loading

thogarty Feb 3, 2025 •

edited

Loading

codecov-commenter commented Jan 31, 2025 •

edited

Loading