refactor: move device resource and datasource to separate package

[ctreatma]

Part of #106

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 62.50000% with 6 lines in your changes are missing coverage. Please review.

Project coverage is 41.67%. Comparing base (5173ee0) to head (24d9d96).

Files	Patch %	Lines
internal/resources/metal/device/resource.go	33.33%	6 Missing ⚠️

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #574      +/-   ##
==========================================
- Coverage   41.80%   41.67%   -0.14%     
==========================================
  Files         102      103       +1     
  Lines       18533    18591      +58     
==========================================
  Hits         7747     7747              
- Misses      10580    10638      +58     
  Partials      206      206              

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

[ctreatma]

I opted to move the metal_devices data source into the same directory as the metal_device resource and datasource, because I think that's better than putting it in a separate internal/resources/metal/devices directory.

If that is agreed to be a good approach, then that raises the question: should metal_device_bgp_neighbors also go in this directory, or does it still go in a separate internal/resources/metal/device_bgp_neighbors directory?

[displague]

The pluralization of the resource names, as in multi-response datasources, was a subtle point that I missed on first glance of this comment.

The AWS provider followed this pattern. Seems fine to me.
https://github.com/hashicorp/terraform-provider-aws/blob/main/internal/service/ec2/ec2_instances_data_source.go

[ctreatma]

Chose to duplicate this matcher instead of finding a place to put it. IMO we can clean that up after framework and equinix-sdk-go migrations are done.

[displague]

I don't think the validation this enforces is necessary in the Terraform provider.

terraform-provider-equinix/equinix/resource_metal_spot_market_request.go

Lines 274 to 277 in 21c9dcc

	if matchIPXEScript.MatchString(params.UserData) {
	return diag.Errorf("\"user_data\" should not be an iPXE " +
	"script when \"ipxe_script_url\" is also provided.")
	}

Perhaps this block would be better as a warning or left to the API to enforce. It's well enough to leave this validation alone since there are no known edge cases where the combination of an IPXE URL and a Custom IPXE Userdata are necessary.

[displague]

Perhaps the regexp could be moved closer to where it is used since it is not used elsewhere.

[displague]

Why not ReadContext? (especially after #581)

[ctreatma]

ReadWithoutTimeout and ReadContext are both context-aware lifecycle methods: the first argument to the function specified for either of those hooks must be a context.Context. The difference between the two hooks is that ReadContext enforces a 20-minute default timeout that can be overridden in a timeout block and ReadWithoutTimeout doesn't enforce any timeout. When this is migrated to framework (probably in this PR at some point), we might be forced to revisit timeouts anyway since SDKv2 and framework model/implement them differently.

[displague]

Should be rebased after #586

[displague]

I'm curious what this looks like after a rebase (and again after #593 and perhaps other open general PRs); whether we've reached a point in refactoring where we can move all remaining metal resources to internal/resources/metal/* with little more than git mv, import list changes, and function exporting?

[ctreatma]

Closing this for now; it can be reopened or reimplemented later if needed.