Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: convert virtual circuit resource & datasource to equinix-sdk-go #700

Merged
merged 7 commits into from
Jun 20, 2024

Conversation

ctreatma
Copy link
Contributor

@ctreatma ctreatma commented Jun 18, 2024

This updates the metal_virtual_circuit resource and data source to use equinix-sdk-go instead of packngo. As mentioned in #402, the packngo SDK has been deprecated, and moving to equinix-sdk-go makes it possible for these resources to gain new features in the future.

As an added bonus, the affected code has also been moved to internal/resources/metal to align with #106.

@ctreatma ctreatma requested a review from a team as a code owner June 18, 2024 15:51
@ctreatma ctreatma force-pushed the virtual-circuit-sdk branch from db7cc99 to 366891b Compare June 18, 2024 20:46
@ctreatma ctreatma force-pushed the virtual-circuit-sdk branch from 366891b to 0447951 Compare June 18, 2024 20:58
@ctreatma ctreatma requested a review from a team as a code owner June 18, 2024 21:47
@ctreatma ctreatma changed the title feat: convert virtual network resource & datasource to equinix-sdk-go feat: convert virtual circuit resource & datasource to equinix-sdk-go Jun 18, 2024
@displague
Copy link
Member

Noting some failed sweeps:

2024/06/18 22:09:56 Sweeper Tests for region (all) ran unsuccessfully:
2024/06/18 22:09:56 	- equinix_metal_vrf: Error deleting VRFs DELETE https://api.equinix.com/metal/v1/vrfs/97b74582-92ab-4df3-af96-a2efe41e3e97: 422 Cannot delete VRF with associated Virtual Circuits, please delete any associated virtual circuits first 
2024/06/18 22:09:56 	- equinix_metal_project: 2 errors occurred:
	* Error deleting project DELETE https://api.equinix.com/metal/v1/projects/47b9d47f-57fd-4e3d-aa0c-d0b812bd18ce: 422 Cannot delete a project with active virtual circuits. 
	* Error deleting project DELETE https://api.equinix.com/metal/v1/projects/d3[135](https://github.com/equinix/terraform-provider-equinix/actions/runs/9572657239/job/26392474481?pr=700#step:7:136)1a8-fb6f-4b76-b835-4453971adec7: 422 Cannot delete a project with active virtual circuits. 


2024/06/18 22:09:56 	- equinix_metal_virtual_circuit: error getting organization list for sweeping VirtualCircuits: no value given for required property address

@displague
Copy link
Member

Some sweeper errors persist:

2024/06/19 16:46:55 Sweeper Tests for region (all) ran unsuccessfully:
2024/06/19 16:46:55 	- equinix_metal_project: 1 error occurred:
	* Error deleting project DELETE https://api.equinix.com/metal/v1/projects/f096688e-166d-42f1-9959-9acf97ebc8a1: 422 cannot delete when associated to a virtual circuit 


2024/06/19 16:46:55 	- equinix_metal_vlan: Error deleting vlan DELETE https://api.equinix.com/metal/v1/virtual-networks/851f2fc7-bfc1-4a9b-b8a7-4a8ec4e125f1: 422 Cannot delete Virtual Network when associated to a virtual circuit 
FAIL	github.com/equinix/terraform-provider-equinix/internal/sweep	7.564s
FAIL

client := meta.(*config.Config).NewMetalClientForSDK(d)
needsUpdate := false

ur := metalv1.VirtualCircuitUpdateInput{}
Copy link
Member

@displague displague Jun 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to think there is some pattern to be used across this resource that would have collapsed the if vrf {} else vlan {} blocks in this PR.

That could look like this:

type VCCommon interface {
  GetHref() string
  GetID() string
  Set...
}

Where that quickly falls apart is in the enum inputs/outputs and other one-off differences between VirtualCircuit types, such as int reference vs *int for NNID fields.

Within resourceMetalVirtualCircuitUpdate, it is more obvious to see how we could apply that pattern. We did so in metal-cli.

equinix/metal-cli#369 (comment)

I only mention this for posterity. Not looking for this PR to adopt this pattern.

@ctreatma ctreatma force-pushed the virtual-circuit-sdk branch from b50e3e9 to 9b74aff Compare June 19, 2024 18:51
@ctreatma ctreatma force-pushed the virtual-circuit-sdk branch from 9b74aff to 32e3025 Compare June 19, 2024 19:09
@ctreatma ctreatma requested a deployment to internal June 19, 2024 20:55 — with GitHub Actions Abandoned
@ctreatma
Copy link
Contributor Author

The remaining sweeper errors seem to pre-date this PR: https://github.com/equinix/terraform-provider-equinix/actions/runs/9503755717/job/26194913301#step:7:146

The connection in question does not have any virtual circuits associated, so it appears they were deleted, just not in time for the subsequent sweeper. That's a problem that impacts our sweepers in general: at a glance, approximately none of them wait for swept resources to disappear; they just assume that if the delete request passed, all is well.

@ctreatma ctreatma force-pushed the virtual-circuit-sdk branch from 29e0864 to 50a06ce Compare June 20, 2024 20:12
@ctreatma ctreatma requested a deployment to internal June 20, 2024 21:28 — with GitHub Actions Abandoned
@displague displague merged commit 379e4d6 into main Jun 20, 2024
4 of 5 checks passed
@displague displague deleted the virtual-circuit-sdk branch June 20, 2024 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants