refactor: move Metal VLAN resource and data source to equinix-sdk-go #541
Conversation
| "vlan_id": vlan.GetId(), | ||
| "project_id": vlan.AssignedTo.GetId(), // vlan assigned_to is an href; should be project? | ||
| "vxlan": vlan.GetVxlan(), | ||
| "facility": vlan.FacilityCode, // facility is deprecated, vlan is metro-scoped; remove this attr? |
There was a problem hiding this comment.
In metal-cli we opted to remove facility from VLAN subcommands because facility is deprecated and VLANs are at the metro level anyway; however I think removing it entirely could be a bigger impact in terraform so we might want to maintain support for it for now?
There was a problem hiding this comment.
Based on API responses I saw in Chrome developer tools, both facility and facility_code are null anyway, so maybe this is a non-issue? Perhaps for now we just explicitly set it to null everywhere in code and deprecate the attribute in the resource & data source schemas so we can remove it later?
| } else if err != nil { | ||
| // missing vlans are deleted | ||
| return nil | ||
| } | ||
|
|
||
| // all device ports must be unassigned before delete | ||
| for _, i := range vlan.Instances { | ||
| for _, p := range i.NetworkPorts { | ||
| for _, p := range i.NetworkPorts { // instances is specced as a list of href; should be devices? | ||
| for _, a := range p.AttachedVirtualNetworks { | ||
| // a.ID is not set despite including instaces.network_ports.virtual_networks | ||
| // TODO(displague) packngo should offer GetID() that uses ID or Href |
There was a problem hiding this comment.
Hey look @displague, this TODO sounds familiar.
| vlan, resp, err := client.ProjectVirtualNetworks.Get(id, &packngo.GetOptions{Includes: []string{"instances", "instances.network_ports.virtual_networks", "internet_gateway"}}) | ||
| if equinix_errors.IgnoreResponseErrors(equinix_errors.HttpForbidden, equinix_errors.HttpNotFound)(resp, err) != nil { | ||
| return equinix_errors.FriendlyError(err) | ||
| vlan, resp, err := client.VLANsApi.GetVirtualNetwork(ctx, id).Include([]string{"instances", "instances.network_ports.virtual_networks", "internet_gateway"}).Execute() |
There was a problem hiding this comment.
It's possible a different include would fix the issue here (at least partially; may need spec changes so the SDK can correctly identify types)
| vlan, resp, err := client.VLANsApi.GetVirtualNetwork(ctx, id).Include([]string{"instances", "instances.network_ports.virtual_networks", "internet_gateway"}).Execute() | |
| vlan, resp, err := client.VLANsApi.GetVirtualNetwork(ctx, id).Include([]string{"instances", "virtual_networks", "internet_gateway"}).Execute() |
| meta.(*config.Config).AddModuleToMetalGoUserAgent(d) | ||
| client := meta.(*config.Config).Metalgo |
There was a problem hiding this comment.
I always trip myself up on updating these two lines. If config.Config had GetMetalClient(d *schema.ResourceData), GetFabricClient(d *schema.ResourceData), etc. functions we could handle the User-Agent modification there so it doesn't have to be replicated in every CRUD function in every resource & data source.
There was a problem hiding this comment.
every resource & data source
Turns out we're only modifying the User-Agent in data sources. Why is that?
internal/acceptance/acceptance.go
Outdated
| @@ -62,6 +62,7 @@ func getFromEnvDefault(varName string, defaultValue string) string { | |||
| return defaultValue | |||
| } | |||
|
|
|||
| // TODO: seems like this is sweeper config and broadly used; why is it non-standard? | |||
| func GetConfigForNonStandardMetalTest() (*config.Config, error) { | |||
There was a problem hiding this comment.
Doesn't necessarily need to happen in this PR, but IMO this should be GetConfigForSweeper and should fully replicate the behavior of the analogous sharedConfigForRegion in the equinix package
ctreatma
force-pushed
the
refactor-vlans
branch
from
e636de7 to
ffacb40
Compare
ctreatma
force-pushed
the
refactor-vlans
branch
2 times, most recently
from
6789e1f to
bdf1566
Compare
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #541 +/- ##
===========================================
+ Coverage 44.71% 58.95% +14.24%
===========================================
Files 99 100 +1
Lines 19890 19890
===========================================
+ Hits 8893 11727 +2834
+ Misses 10963 7867 -3096
- Partials 34 296 +262 ☔ View full report in Codecov by Sentry. |
ctreatma
force-pushed
the
refactor-vlans
branch
from
bdf1566 to
85973ed
Compare
|
Will need rebasing after #578 |
|
Closing this for now; it can be reopened or reimplemented later if needed. |
This refactors the VLAN resource and data source to use equinix-sdk-go instead of packngo. As part of this refactoring I've moved the VLAN resource and data source to an isolated package.
PR is in draft for now because there are a number of spec issues that need to be addressed and I want to get a feel for how manageable it is to move resources & change the SDK in one PR.
Part of #402
Part of #106