refactor: Moving utility code to `internal/` #420

t0mk · 2023-10-18T14:26:28Z

Fixes #106

displague · 2023-10-20T13:31:27Z

equinix/internal/fabric_cloud_router_read_schema.go

@@ -1,4 +1,4 @@
-package equinix
+package internal


I wouldn't categorize resource specific schema mapping helpers as utility code. I think we should attempt to migrate these types of functions with the schemas, resources, and datasources.

internal/fabric/{resourcename}/{resource,datasource,schema}.go

Utility code prime for this refactor would be generic and suite many resources. These should be split into packages specific to the type of data that it manipulates, algorithms it provides, or protocols it communicates in.

https://go.dev/blog/package-names

I don't believe we'll benefit from a singular internal package anymore than a singular equinix package (beyond making not sharing the exports globally: https://go.dev/doc/go1.4#internalpackages)

Looking at the AWS structure (which is not adapted to Framework), we see they chose the paths: internal/service/foo/: https://github.com/hashicorp/terraform-provider-aws/tree/main/internal/service/ec2/ec2_instance.go

Linode took another approach. they didn't use internal/ and they didn't create namespaces for the resources: https://github.com/linode/terraform-provider-linode/tree/dev/linode/instance

In my old PR, I went weith internal/resources/{service:metal}/{resource:device}/resource.go

Namespaces (metal, fabric, ne) will be important for how we collaborate with peer teams on these resources.

We'll also want per resource directories/packages -- https://developer.hashicorp.com/terraform/plugin/framework-benefits#separate-packages-for-each-type

We'll also want to use internal/ to keep the resources from becoming the dependency of other projects.

Notably, the equinix/Provider can be exported since its Go signature remains consistent and doesn't break compatibility. This package has been a dependency for Crossplane and was previously relied upon by Pulumi before Pulumi transitioned to a git submodule pattern.

t0mk · 2023-11-02T17:05:35Z

To move code for resource metal_ssh_key to a subdirectory, I had to

create subdirs metal_ssh_key/ and metal_project_ssh_key/ and moved code of respective resources and datasources to the subdirectories. provider.go then imports the metal_ssh_key resource code from the sub-package. Since the module code uses the Config struct, I had to move the Config struct out of the equinix/ namespace to avoid import cycle.
I moved the Config struct to internal/. I had to move more code along to internal/.
All around the provider source files, I had to fix a lot of references to the Config struct and also to other utilities which now live in internal/.
At this point the code in metal_ssh_key subdir compiled OK.

Then I wanted to run tests in the subdir metal_ssh_key. I realized that if the tests live in the same package as the module code, it's not straightforward because the tests need to instantiate the TF Provider object defined in equinix, and thus close the import cycle that I worked so hard to break..

I found out yesterday (after 6 years of Golang programming) that it's possible to have packages foo and foo_test live in the same directory foo/. Then, import foo will not import foo_test. So I just put package metal_ssh_key_test to the top of source files which contain the tests. Then, the main package didn't import them, and I could instantiate the provider object for acceptance tests. It's not necessary to import the metal_ssh_key package from the tests because the tests only use TF templates and don't call resource code directly.

There is a lot of shared functionality for acceptance tests, and that utility code can't live in internal/, because the main package imports internal/. I created acceptance/ subdir where I placed the test helpers.

I had a feedback from @ctreatma not to touch the _fabric_ code and resources, but it was not possible to make all this without changing the references to Config and some helper functions in most of the source files, including fabric stuff.

We do all this in order to migrate to framework plugin eventually [0].

Let's discuss the format! I will keep this up to date (rebasing) with main.

[0] #406 (comment)

ctreatma

We have a big ball of mud right now, so it probably won't be possible to avoid touching NE / Fabric stuff at all during the refactor. So far, this looks targeted enough in terms of those changes; I did comment on a couple places where the changes aren't strictly needed for refactoring.

We should add more package isolation as part of this refactor so that we don't end up with a new ball of mud in a different directory: config in a config package, errors in an error package, etc..

equinix/fabric_service_profile_schema.go

equinix/fabric_mapping_helper.go

equinix/fabric_connection_schema.go

t0mk · 2023-11-06T16:29:24Z

@ctreatma thanks for your review.

I went through internal/ in order to categorize code and suggest other subdirs.

There's

errors - could go to error/
config - could go to config/
utils like hashcode, mutex struct, could go to utils/
helpers for device and port resources. They could go to utils/

Not sure if we have any need or internal/ anymore.

displague · 2023-11-08T15:41:31Z

equinix/metal_project_ssh_key/data_source_test.go

@@ -1,4 +1,4 @@
-package equinix
+package metal_project_ssh_key_test


👁️ https://jdkaplan.dev/thinkin-logs/2021-10-07/

displague · 2023-11-08T16:11:57Z

Let's isolate some of these changes and introduce them one at a time in separate PRs:

internal/config, internal/errors etc (in #142 there was some suggestion in the packages proposed on how to reorganize these helpers)

Then we can move the resources around - again separate PRs, first with Metal, then NE, then Fabric.

From there it may be easy to convert a single resource to Framework and continue from there.

ctreatma · 2023-12-05T19:43:18Z

@t0mk when you have a moment, could you rebase this PR on the latest from main so we can get a better idea of what remains for unblocking framework adoption?

displague · 2023-12-06T16:13:12Z

Closing this since we are breaking the PR up into smaller pieces

ctreatma · 2023-12-07T15:52:48Z

internal/utils.go

@@ -0,0 +1,28 @@
+package internal


It looks like the functions in this file aren't used by anything related to the Metal SSH key refactor; we should still break this change out into a separate refactor, but if it's not necessary for the Metal SSH key refactor, we can remove these changes from the PR now with no impact.

ctreatma · 2023-12-07T15:55:28Z

internal/mutexkv.go

@@ -1,4 +1,4 @@
-package equinix
+package internal


This move also seems to not impact the SSH key refactor and could be removed from this PR without waiting for a separate mutexkv refactor PR to be merged.

ctreatma · 2023-12-07T15:59:36Z

internal/port_helpers.go

@@ -1,4 +1,4 @@
-package equinix
+package internal


This one also seems to not be needed to support the SSH key changes

ctreatma · 2023-12-07T16:02:06Z

internal/config/config.go

@@ -28,11 +28,6 @@ import (
 	xoauth2 "golang.org/x/oauth2"
 )

-var (
-	UuidRE         = regexp.MustCompile("^[a-fA-F0-9]{8}-[a-fA-F0-9]{4}-4[a-fA-F0-9]{3}-[8|9|aA|bB][a-fA-F0-9]{3}-[a-fA-F0-9]{12}$")


This, too, looks like it's not technically required in order to move the Metal SSH key code.

ctreatma · 2023-12-18T20:33:29Z

Closing again since the resources affected here were covered in #479 and the unrelated refactorings will be covered in separate PRs.

t0mk temporarily deployed to internal October 18, 2023 14:26 — with GitHub Actions Inactive

displague reviewed Oct 20, 2023

View reviewed changes

t0mk changed the title ~~Moving uility code to subdirs~~ Moving utility code to internal/ Nov 2, 2023

t0mk force-pushed the utilities_to_subdirs branch 2 times, most recently from 8bc528f to eeed008 Compare November 2, 2023 16:37

t0mk changed the title ~~Moving utility code to internal/~~ refactor: Moving utility code to internal/ Nov 2, 2023

ctreatma reviewed Nov 3, 2023

View reviewed changes

equinix/fabric_service_profile_schema.go Outdated Show resolved Hide resolved

equinix/fabric_mapping_helper.go Outdated Show resolved Hide resolved

equinix/fabric_connection_schema.go Outdated Show resolved Hide resolved

t0mk force-pushed the utilities_to_subdirs branch 4 times, most recently from fc0074d to 065f50e Compare November 6, 2023 16:09

t0mk force-pushed the utilities_to_subdirs branch from a1dd3f7 to ac90fb0 Compare November 8, 2023 14:15

displague reviewed Nov 8, 2023

View reviewed changes

t0mk force-pushed the utilities_to_subdirs branch from ac90fb0 to 5054745 Compare November 14, 2023 12:24

displague closed this Dec 6, 2023

ctreatma reopened this Dec 6, 2023

ctreatma force-pushed the utilities_to_subdirs branch 5 times, most recently from 7300b22 to d8b8506 Compare December 7, 2023 15:51

ctreatma reviewed Dec 7, 2023

View reviewed changes

Refactor provider code so that we can have a resources in subdirs

831b48a

ctreatma force-pushed the utilities_to_subdirs branch from d8b8506 to 831b48a Compare December 7, 2023 15:56

ctreatma reviewed Dec 7, 2023

View reviewed changes

ctreatma closed this Dec 18, 2023

ctreatma deleted the utilities_to_subdirs branch February 15, 2024 17:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Moving utility code to `internal/` #420

refactor: Moving utility code to `internal/` #420

t0mk commented Oct 18, 2023 •

edited by displague

Loading

displague Oct 20, 2023

displague Nov 8, 2023 •

edited

Loading

t0mk commented Nov 2, 2023

ctreatma left a comment

t0mk commented Nov 6, 2023

displague Nov 8, 2023

displague commented Nov 8, 2023

ctreatma commented Dec 5, 2023

displague commented Dec 6, 2023

ctreatma Dec 7, 2023

ctreatma Dec 7, 2023

ctreatma Dec 7, 2023

ctreatma Dec 7, 2023

ctreatma commented Dec 18, 2023

		@@ -1,4 +1,4 @@
		package equinix
		package metal_project_ssh_key_test

refactor: Moving utility code to internal/ #420

refactor: Moving utility code to internal/ #420

Conversation

t0mk commented Oct 18, 2023 • edited by displague Loading

displague Oct 20, 2023

Choose a reason for hiding this comment

displague Nov 8, 2023 • edited Loading

Choose a reason for hiding this comment

t0mk commented Nov 2, 2023

ctreatma left a comment

Choose a reason for hiding this comment

t0mk commented Nov 6, 2023

displague Nov 8, 2023

Choose a reason for hiding this comment

displague commented Nov 8, 2023

ctreatma commented Dec 5, 2023

displague commented Dec 6, 2023

ctreatma Dec 7, 2023

Choose a reason for hiding this comment

ctreatma Dec 7, 2023

Choose a reason for hiding this comment

ctreatma Dec 7, 2023

Choose a reason for hiding this comment

ctreatma Dec 7, 2023

Choose a reason for hiding this comment

ctreatma commented Dec 18, 2023

refactor: Moving utility code to `internal/` #420

refactor: Moving utility code to `internal/` #420

t0mk commented Oct 18, 2023 •

edited by displague

Loading

displague Nov 8, 2023 •

edited

Loading