refactor: First draft of changes to separate 2 basic resources away from equinix/ #479

t0mk · 2023-12-07T15:30:42Z

This PR is an attempt to move two basic resources away from the equinix namespace. Once this is in, it should be easier to start working on the framework plugin migration.

Part of #106 towards #365

t0mk · 2023-12-07T15:53:08Z

I had to move both metal_ssh_key and metal_project_ssh_key because they share some code. They are both simple resources anyway.

The internal/acceptance package contains helpers for acceptance tests. Right now it duplicates code from equinix/. we will need to be aware of it when moving resources away from equinix/ and use test helpers from internal/equinix in moved resources. I can't import internal/acceptance in equinix/, Go complains about import cycle. I'm not sure how it formed.

I also added a Makefile target to run tests from internal/resources/*/*.

t0mk · 2023-12-07T15:57:19Z

@ctreatma @displague What do you think?

ctreatma · 2023-12-07T20:06:11Z

internal/config/envvars.go

+package config
+
+
+const (


This (and the related changes) could be broken out to a separate PR, and IMO the constants could be added to config.go rather than going in their own file.

Why not have the envvars defined in a separate file? They are used by provider and sweeper code; and are obviously unreleated to code in config.go. I would consider a separate package (internal/envvars) but since it's configuration related, I dropped it here.

The envvars were moved to internal because sweeping in the acceptance tests for the ssh_key uses them, and can't use them from equinix/ because of import cycyle. If I wouldn't actually try to make acceptance tests of moved metal_ssh_key work, I wouldn't know that they need to be moved. I just spend 15 minutes researching on how to justify this :(.

If they're going to the config package (which is IMO reasonable since they are used as configuration), then there's not really a compelling reason to add them to a separate file that only defines the constants: it's still in the config package and it still gets imported along with all of the code in config.go. Moving the constants to a different file in the same package shouldn't lead to an import cycle.

OK, I moved the const block to config.go and removed envvars.go.

displague · 2023-12-08T03:25:58Z

internal/resources/metal/metal_ssh_key/resource.go

 	}
 }

-func resourceMetalSSHKeyCreate(d *schema.ResourceData, meta interface{}) error {
+func ResourceMetalSSHKeyCreate(d *schema.ResourceData, meta interface{}) error {


nit: We shouldn't need to export the CRUD functions, just the Resource or DataSource itself.

And since we are sharing CRUD and Schema between the project ssh key and user ssh keys, we could still use the exported Resource as the base and then give it a Project variation of Schema. The shared schema (MetalSSHKeyCommonFields) could be exported.

displague · 2023-12-08T03:31:33Z

internal/resources/metal/metal_ssh_key/resource.go

@@ -13,7 +13,7 @@ import (
 	"github.com/packethost/packngo"
 )

-func metalSSHKeyCommonFields() map[string]*schema.Schema {
+func MetalSSHKeyCommonFields() map[string]*schema.Schema {


We should avoid repeating the package name in the function names. For example, this would be metal_ssh_key.CommonFields().

Same for the CRUD functions. (ResourceMetalSSHKeyCreate -> Create, and perhaps create or resourceCreate since we don't need to export these functions.

I changed the CRUD callbacks to private and renamed them. I also made the schema easier to share between metal_ssh_key and metal_project_ssh_key.

displague · 2023-12-08T03:48:28Z

internal/acceptance/ssh_key_helpers.go

+	"github.com/packethost/packngo"
+)
+
+func TestAccCheckMetalSSHKeyExists(n string, key *packngo.SSHKey) resource.TestCheckFunc {


I suspect this file should be named ..._test.go

This is not a test, just a helper function, and we've got one like this for most of the resource types. I'm not sure why.

I would name the file resource_checkers.go and put there all the other test helpers which check if resource exists remotely.

@displague What about this?

displague · 2023-12-08T04:18:40Z

GNUmakefile

-testacc:
+testaccmoved:
+	# go through all the directories in internal/resources and run tests
+	for d in $$(find ./internal/resources -maxdepth 2 -mindepth 2 -type d); do \


These packages should already be covered by $(TEST). The for loop over directories in internal/ doesn't seem necessary and I suspect we are retesting packages.

You are right. I thought make testacc was only going through tests in equnix/. I will remove changes in the Makefile.

displague · 2023-12-12T13:53:46Z

internal/acceptance/device_helpers.go

@@ -0,0 +1,119 @@
+package acceptance


Not requesting any changes, just commenting:

These helpers may turn out to all be "metal" related. In the future we may want to consider moving such service-shared testing / utility code to a service specific home.

internal/services/metal/{resourcename}
internal/services/metal/testing

More likely, and for more gain, I think we would want to move towards a testing pattern like the one in the Linode provider:
https://github.com/linode/terraform-provider-linode/tree/dev/linode/instance/tmpl

We have a few string building helpers and the go template approach is better suited for that.

codecov-commenter · 2023-12-12T16:43:47Z

Codecov Report

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

Comparison is base (26b2f3d) 60.11% compared to head (2dfde31) 59.43%.
Report is 99 commits behind head on main.

Files	Patch %	Lines
equinix/fabric_mapping_helper.go	0.00%	68 Missing ⚠️
equinix/resource_fabric_connection.go	0.00%	52 Missing ⚠️
equinix/resource_fabric_cloud_router.go	0.00%	42 Missing ⚠️
equinix/resource_fabric_service_profile.go	0.00%	24 Missing ⚠️
internal/resources/metal/metal_ssh_key/resource.go	73.62%	19 Missing and 5 partials ⚠️
equinix/resource_fabric_routing_protocol.go	0.00%	22 Missing ⚠️
equinix/resource_metal_device.go	78.46%	10 Missing and 4 partials ⚠️
equinix/resource_metal_organization_member.go	40.90%	13 Missing ⚠️
equinix/resource_network_device_link.go	0.00%	10 Missing ⚠️
equinix/resource_metal_connection.go	60.86%	8 Missing and 1 partial ⚠️
... and 40 more

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #479      +/-   ##
==========================================
- Coverage   60.11%   59.43%   -0.69%     
==========================================
  Files          99       96       -3     
  Lines       20045    19745     -300     
==========================================
- Hits        12051    11736     -315     
- Misses       7691     7714      +23     
+ Partials      303      295       -8

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

ctreatma

IMO this is good as-is; I see the relevant tests passing in CI, and the failed tests are all platform issues. If there are things we want to adjust we can do that in follow-up PRs.

t0mk temporarily deployed to internal December 7, 2023 15:30 — with GitHub Actions Inactive

t0mk changed the title ~~First draft of changes to separate a 2 basic resource away from equinix/~~ refactor: First draft of changes to separate a 2 basic resource away from equinix/ Dec 7, 2023

t0mk changed the title ~~refactor: First draft of changes to separate a 2 basic resource away from equinix/~~ refactor: First draft of changes to separate 2 basic resources away from equinix/ Dec 7, 2023

ctreatma reviewed Dec 7, 2023

View reviewed changes

displague reviewed Dec 8, 2023

View reviewed changes

t0mk force-pushed the seprate_metal_ssh_key branch from a30aee9 to f9f13e0 Compare December 11, 2023 14:17

t0mk temporarily deployed to internal December 11, 2023 14:17 — with GitHub Actions Inactive

t0mk force-pushed the seprate_metal_ssh_key branch from f9f13e0 to 9293144 Compare December 12, 2023 12:20

t0mk temporarily deployed to internal December 12, 2023 12:20 — with GitHub Actions Inactive

displague reviewed Dec 12, 2023

View reviewed changes

t0mk force-pushed the seprate_metal_ssh_key branch from 9293144 to e9d9e2e Compare December 12, 2023 15:33

t0mk temporarily deployed to internal December 12, 2023 15:33 — with GitHub Actions Inactive

t0mk force-pushed the seprate_metal_ssh_key branch from e9d9e2e to a706f2b Compare December 12, 2023 15:39

t0mk temporarily deployed to internal December 12, 2023 15:39 — with GitHub Actions Inactive

First draft of changes to separate a resource away from equinix pkg

2dfde31

t0mk force-pushed the seprate_metal_ssh_key branch from a706f2b to 2dfde31 Compare December 12, 2023 15:41

t0mk temporarily deployed to internal December 12, 2023 15:41 — with GitHub Actions Inactive

ctreatma approved these changes Dec 18, 2023

View reviewed changes

displague merged commit 0e3709c into main Dec 18, 2023
4 of 5 checks passed

displague deleted the seprate_metal_ssh_key branch December 18, 2023 15:58

ctreatma mentioned this pull request Dec 18, 2023

refactor: Moving utility code to internal/ #420

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: First draft of changes to separate 2 basic resources away from equinix/ #479

refactor: First draft of changes to separate 2 basic resources away from equinix/ #479

t0mk commented Dec 7, 2023 •

edited by displague

Loading

t0mk commented Dec 7, 2023

t0mk commented Dec 7, 2023

ctreatma Dec 7, 2023

t0mk Dec 11, 2023

ctreatma Dec 11, 2023

t0mk Dec 12, 2023

displague Dec 8, 2023

displague Dec 8, 2023

displague Dec 8, 2023

displague Dec 8, 2023 •

edited

Loading

t0mk Dec 12, 2023

displague Dec 8, 2023

t0mk Dec 11, 2023

t0mk Dec 12, 2023

displague Dec 8, 2023

t0mk Dec 11, 2023

displague Dec 12, 2023 •

edited

Loading

codecov-commenter commented Dec 12, 2023

ctreatma left a comment

refactor: First draft of changes to separate 2 basic resources away from equinix/ #479

refactor: First draft of changes to separate 2 basic resources away from equinix/ #479

Conversation

t0mk commented Dec 7, 2023 • edited by displague Loading

t0mk commented Dec 7, 2023

t0mk commented Dec 7, 2023

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

displague Dec 8, 2023 • 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

Choose a reason for hiding this comment

displague Dec 12, 2023 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Dec 12, 2023

Codecov Report

ctreatma left a comment

Choose a reason for hiding this comment

t0mk commented Dec 7, 2023 •

edited by displague

Loading

displague Dec 8, 2023 •

edited

Loading

displague Dec 12, 2023 •

edited

Loading