feat: add equinix_fabric_precision_time resource and data source #745

thogarty · 2024-07-31T22:26:22Z

Add precision_time resource/data source to internal package
Use terraform-plugin-framework for development
Add doc templates for the new resource/data_source

internal/resources/fabric/precision_time/models.go

internal/resources/fabric/precision_time/resource_schema.go

ctreatma · 2024-08-01T20:22:25Z

examples/data-sources/fabric_precision_time/get_by_uuid.tf

@@ -0,0 +1,3 @@
+data "equinix_fabric_precision_time" "time_service" {


Note that you can avoid adding a custom doc template for this datasource by moving this file to examples/data-sources/equinix_fabric_precision_time/data-source.tf

ctreatma · 2024-08-01T20:24:00Z

examples/resources/fabric_precision_time/ntp_time_service.tf

@@ -0,0 +1,17 @@
+resource "equinix_fabric_precision_time" "ntp" {


There's more of a tradeoff for these examples than the data source example, but you could merge these resource examples into a single file, with # comments describing each block if necessary, put the merged file at examples/resources/equinix_fabric_precision_time/resource.tf and remove the custom doc template for the resource as well.

I tried out this documentation adjustment you suggested for Data Source and Resource and didn't get the results that were expected. I still feel the templates are necessary based on what Fabric team expects before the schema information, and the examples weren't included following that pattern.

ctreatma · 2024-08-02T15:57:45Z

It looks like this branch will need to be rebased on the latest from main to ensure that all necessary validation workflows run for this PR.

… Create method for Fabric EPT

…nceled

…plates for doc generation

…ed as optional or computed

ctreatma · 2024-08-07T20:04:03Z

internal/resources/fabric/precision_time/resource_test.go

@@ -0,0 +1,268 @@
+package precision_time


We encourage black box testing rather than tying tests to private implementation details. The _test package is a Go idiom that allows you to keep tests in the same directory as the code under test, but force those tests to use the code as a consumer would. (Ultimately your team are the owners of this code and the decision as to whether to use black box testing for each resource is entirely up to you.)

Suggested change

package precision_time

package precision_time_test

ctreatma · 2024-08-07T20:10:18Z

templates/data-sources/fabric_precision_time.md.tmpl

+Fabric V4 API compatible data resource that allow user to fetch Equinix Fabric Precision Time Service by Uuid
+
+Additional documentation:
+* Getting Started: https://docs.equinix.com/en-us/Content/Edge-Services/EPT/EPT.htm
+* API: https://developer.equinix.com/dev-docs/fabric/api-reference/fabric-v4-apis#precision-time


You could instead put this in the Description or MarkdownDescription attribute of the data source schema and reference {{ .Description | trimspace }} in the docs template. This would keep the description in the docs while also making available to other tools.

ctreatma · 2024-08-07T20:12:17Z

templates/data-sources/fabric_precision_time.md.tmpl

+{{tffile "examples/data-sources/fabric_precision_time/get_by_uuid.tf"}}
+
+
+{{ .SchemaMarkdown }}


I've usually seen that as {{ .SchemaMarkdown | trimspace }}, which would I think eliminate some of the empty lines at the end of the generated docs.

ctreatma · 2024-08-07T20:23:31Z

internal/resources/fabric/precision_time/resource.go

+	}
+
+	// Set state to fully populated data
+	resp.Diagnostics.Append(resp.State.Set(ctx, &plan)...)


Is this where you're running into issues due to fields that are only present in the request and not in the response?

Yea, this is the spot. It's related to advance_configuration only at this point. I'm considering separating ntp and ptp into their own attributes:

advance_configuration_ntp and advance_configuration_ptp.

NTP will be in request only and not in response. So it could be marked optional to allow for it being set early and not set later.

PTP will be in request and response so it could stay as optional/computed.

The nested object structure in terraform-plugin-framework along with it's strict checking of attribute behavior has proven difficult for me. Hoping this uncovers an appropriate path forward that doesn't actually involve modifying the schemas to be so different from the API schema similar to how project_id was handled. It can backfire if the project object gets extended or if advance_configuration gets extended.

This also happens during Read and Update. Read when importing a resource, and Update when the response doesn't contain the details it expects. Though I haven't been able to test those as much because the Create has gotten a plan error every time advance_configuration is used. When advance_configuration is not used then we're able to proceed without plan errors based on parsing method putting some zero values there.

It looks like the data source and resource have the same attributes. Is the data source able to load the advance_configuration details on read? Or does that attribute not work at all for data sources?

Is there some other data in the API response that corresponds to the advance_configuration settings? Or a different API endpoint that can be used fetch the advanced configuration details?

ctreatma · 2024-08-07T20:30:22Z

internal/config/config.go

 func (c *Config) NewFabricClientForSDK(d *schema.ResourceData) *fabricv4.APIClient {
-	client := c.newFabricClient()
+	client := c.newFabricClient(context.WithValue(context.Background(), ctxKey("fabricSDKv2"), "no_oauth_reload"))


I'd recommend waiting to make this change until you can update all of the functions calling NewFabricClientForSDK to pass in the correct context. The background context is probably not the right one to latch on to.

ctreatma · 2024-08-07T20:31:40Z

internal/config/config.go

@@ -134,18 +137,37 @@ func (c *Config) NewFabricClientForSDK(d *schema.ResourceData) *fabricv4.APIClie
 // Shim for Fabric tests.
 // Deprecated: when the acceptance package starts to contain API clients for testing/cleanup this will move with them
 func (c *Config) NewFabricClientForTesting() *fabricv4.APIClient {
-	client := c.newFabricClient()
+	client := c.newFabricClient(context.WithValue(context.Background(), ctxKey("fabricSDKv2"), "no_oauth_reload"))


This is probably fine since it's only used in tests, but this could be an opportunity to define a client in the acceptance package for testing/cleanup instead.

ctreatma · 2024-08-07T20:35:11Z

internal/config/config.go

 	//nolint:staticcheck // We should move to subsystem loggers, but that is a much bigger change
 	transport := logging.NewTransport("Equinix Fabric (fabricv4)", c.authClient.Transport)
+	if v := ctx.Value(ctxKey("fabricSDKv2")); v == nil {


IMO it'd be better to have two functions, one that takes a context and includes authClient creation, and the old one that didn't take a context.

I'm curious what is the benefit of choosing not to create a new authClient, though?

ctreatma · 2024-08-09T19:15:17Z

internal/resources/fabric/precision_time/models.go

+	eptAdvanceConfiguration := ept.GetAdvanceConfiguration()
+	parsedEptAdvanceConfiguration, diags := parseAdvanceConfiguration(ctx, &eptAdvanceConfiguration)
+	if diags.HasError() {
+		return diags
+	}
+	if !reflect.DeepEqual(parsedEptAdvanceConfiguration, fwtypes.NewListNestedObjectValueOfNull[AdvanceConfigurationModel](ctx)) {
+		*advanceConfiguration = parsedEptAdvanceConfiguration
+	}


To avoid setting the advance_configuration attribute when parsing an API response, you should only set that attribute on the model if it was non-null in the response (suggestion may not be entirely valid code):

Suggested change

eptAdvanceConfiguration := ept.GetAdvanceConfiguration()

parsedEptAdvanceConfiguration, diags := parseAdvanceConfiguration(ctx, &eptAdvanceConfiguration)

if diags.HasError() {

return diags

}

if !reflect.DeepEqual(parsedEptAdvanceConfiguration, fwtypes.NewListNestedObjectValueOfNull[AdvanceConfigurationModel](ctx)) {

*advanceConfiguration = parsedEptAdvanceConfiguration

}

eptAdvanceConfiguration := ept.AdvanceConfiguration

if eptAdvanceConfiguration != nil {

parsedEptAdvanceConfiguration, diags := parseAdvanceConfiguration(ctx, eptAdvanceConfiguration)

if diags.HasError() {

return diags

}

if !reflect.DeepEqual(parsedEptAdvanceConfiguration, fwtypes.NewListNestedObjectValueOfNull[AdvanceConfigurationModel](ctx)) {

*advanceConfiguration = parsedEptAdvanceConfiguration

}

}

If there's no way for this property to be included in an API response, you could also just remove this section of code from the parse code path for now.

thogarty requested review from a team as code owners July 31, 2024 22:26

thogarty temporarily deployed to internal July 31, 2024 22:26 — with GitHub Actions Inactive

ctreatma reviewed Aug 1, 2024

View reviewed changes

internal/resources/fabric/precision_time/models.go Outdated Show resolved Hide resolved

ctreatma reviewed Aug 1, 2024

View reviewed changes

internal/resources/fabric/precision_time/resource_schema.go Outdated Show resolved Hide resolved

ctreatma reviewed Aug 1, 2024

View reviewed changes

thogarty deployed to internal August 6, 2024 22:40 — with GitHub Actions Active

thogarty temporarily deployed to internal August 6, 2024 22:40 — with GitHub Actions Inactive

thogarty added 7 commits August 6, 2024 15:45

Add schema, data models, parse method, buildRequest, getResource, and…

dc717b7

… Create method for Fabric EPT

Add code for precision time data source

4c402a3

Working build and initial API request from EPT; oauth2 has context ca…

4dc5717

…nceled

Completed initial testing of Time Precision Service and added doc tem…

eb6268e

…plates for doc generation

Update set blocks to single nested blocks with no required attributes

04edb45

Update schema to handle computed values for non-set config items mark…

43f6d85

…ed as optional or computed

Resolve rebase modifications

0f97aa0

thogarty force-pushed the CXF-96037-ept-resource-addition branch from a3cf620 to 0f97aa0 Compare August 6, 2024 22:49

thogarty temporarily deployed to internal August 6, 2024 22:49 — with GitHub Actions Inactive

Resolve linting errors

6f0dd0f

thogarty temporarily deployed to internal August 6, 2024 23:50 — with GitHub Actions Inactive

Fix go imports for linting

e659ad9

thogarty temporarily deployed to internal August 7, 2024 00:05 — with GitHub Actions Inactive

Fix go imports for linting

d8caa60

thogarty temporarily deployed to internal August 7, 2024 00:07 — with GitHub Actions Inactive

Docs update after schema changes

5f3b400

thogarty temporarily deployed to internal August 7, 2024 00:23 — with GitHub Actions Inactive

thogarty deployed to internal August 7, 2024 00:23 — with GitHub Actions Active

Add acceptance test starter

3ccd4d2

thogarty temporarily deployed to internal August 7, 2024 01:37 — with GitHub Actions Inactive

ctreatma reviewed Aug 7, 2024

View reviewed changes

thogarty marked this pull request as draft August 7, 2024 21:51

ctreatma reviewed Aug 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add equinix_fabric_precision_time resource and data source #745

feat: add equinix_fabric_precision_time resource and data source #745

thogarty commented Jul 31, 2024

ctreatma Aug 1, 2024

ctreatma Aug 1, 2024

thogarty Aug 7, 2024

ctreatma commented Aug 2, 2024

ctreatma Aug 7, 2024

ctreatma Aug 7, 2024

ctreatma Aug 7, 2024

ctreatma Aug 7, 2024

thogarty Aug 7, 2024

thogarty Aug 8, 2024

ctreatma Aug 8, 2024

ctreatma Aug 7, 2024

ctreatma Aug 7, 2024

ctreatma Aug 7, 2024

ctreatma Aug 9, 2024

		@@ -0,0 +1,3 @@
		data "equinix_fabric_precision_time" "time_service" {

		@@ -0,0 +1,17 @@
		resource "equinix_fabric_precision_time" "ntp" {

		{{tffile "examples/data-sources/fabric_precision_time/get_by_uuid.tf"}}


		{{ .SchemaMarkdown }}

feat: add equinix_fabric_precision_time resource and data source #745

Are you sure you want to change the base?

feat: add equinix_fabric_precision_time resource and data source #745

Conversation

thogarty commented Jul 31, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctreatma commented Aug 2, 2024

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

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