search

equinix-labs / metal-go

[Deprecated] Golang client for Equinix Metal
https://deploy.equinix.com/labs/equinix-sdk-go/
MIT License
3 stars 2 forks source link

fix: Added IPAssignments type #153

Closed codinja1188 closed 1 year ago

displague commented 1 year ago

Does it make sense to introduce an IPAddress.type enum with value IPAssignment if that field doesn't really exist in the API?

We've discussed alternative approaches elsewhere. Some of the fields that would be involved are not included in the spec today. We can also see these fields were included in packngo: https://github.com/packethost/packngo/blob/master/ip.go#L56-L130

We can see the complete result with PACKNGO_DEBUG=1 metal ip list --project-id $PROJECT_ID --include assignments -o yaml 2>&1 > /dev/null. What we see are a list of IPs (reservations and dynamically provisioned) with embedded "assignments". These assignments share some of the structure that IPs report, but have distinct fields and omissions when compared to IPs.

field evidences apparent purpose
addon IPReservation (not documented) appears to be true for all approved elastic IPs, false otherwise
bill IPReservation not obvious if only or all reservations would have this as true, but also appears true for approved elastic IPs, false otherwise
state reserved and non-reserved IPs will say "created" in their success state
requested_by IPReservation this field seems to have a User reference (href, id, short_id) with reservations, and is null (but present) otherwise
assignments no bearing on reserved or not, and appears to be a list of IPAssignments present in the root objects in the project IPs list
assigned_to IPAssignment a required field for an IPAssignment, not present in the the root objects in the project IPs list
type Will not be present in an IPAssignment, will be present in a reservation or non-reserved IP

Based on this, expecting addon to be present (required) may be our best way to distinguish IPReservations, and assigned_to (required) when checking for IPAssignments.

ctreatma commented 1 year ago

In terms of identifying that a particular JSON object is an IP reservation or a VRF IP reservation, that is already in place using the type enum as specified in the upstream spec, and I don't think we should change that; the case we are covering with this patch is how to identify that a particular JSON object that is an IP reservation (or a VRF IP reservation) is not also an IP assignment.

The invalid type workaround was initially chosen because it lines up with how IPReservation and VrfIPReservation are differentiated. By specifying an invalid value for type, we provide a way for the SDK to know that an IPReservation or VrfIPReservation is not an IPAssignment, without making assumptions about the other properties that are defined for IPAssignment. Since the type is optional, the JSON for an IP assignment will still match the patched spec; since the type value specified for IPAssignment is not a value the API will return, the JSON for an IP reservation or VRF IP reservation will not match the patched spec.

Requiring the assigned_to property in the IPAssignment schema instead would be more direct, so I'm on board with that.

codinja1188 commented 1 year ago

Here are the metal-cli test logs. check and approve the PR

Vasubabus-MacBook-Pro:metal-cli vasubabu$ make test
go test -v ./... -timeout 1000s
=== RUN   TestCli_RegisterCommands
=== RUN   TestCli_RegisterCommands/test
--- PASS: TestCli_RegisterCommands (0.00s)
    --- PASS: TestCli_RegisterCommands/test (0.00s)
PASS
ok      github.com/equinix/metal-cli/cmd    (cached)
?       github.com/equinix/metal-cli/cmd/metal  [no test files]
?       github.com/equinix/metal-cli/internal/capacity  [no test files]
?       github.com/equinix/metal-cli/internal/cli   [no test files]
?       github.com/equinix/metal-cli/internal/completion    [no test files]
?       github.com/equinix/metal-cli/internal/devices   [no test files]
?       github.com/equinix/metal-cli/internal/docs  [no test files]
?       github.com/equinix/metal-cli/internal/emdocs    [no test files]
?       github.com/equinix/metal-cli/internal/env   [no test files]
?       github.com/equinix/metal-cli/internal/events    [no test files]
?       github.com/equinix/metal-cli/internal/facilities    [no test files]
?       github.com/equinix/metal-cli/internal/gateway   [no test files]
?       github.com/equinix/metal-cli/internal/hardware  [no test files]
?       github.com/equinix/metal-cli/internal/init  [no test files]
?       github.com/equinix/metal-cli/internal/ips   [no test files]
?       github.com/equinix/metal-cli/internal/metros    [no test files]
?       github.com/equinix/metal-cli/internal/organizations [no test files]
?       github.com/equinix/metal-cli/internal/os    [no test files]
?       github.com/equinix/metal-cli/internal/outputs   [no test files]
?       github.com/equinix/metal-cli/internal/pagination    [no test files]
?       github.com/equinix/metal-cli/internal/plans [no test files]
?       github.com/equinix/metal-cli/internal/ports [no test files]
?       github.com/equinix/metal-cli/internal/projects  [no test files]
?       github.com/equinix/metal-cli/internal/ssh   [no test files]
?       github.com/equinix/metal-cli/internal/twofa [no test files]
?       github.com/equinix/metal-cli/internal/users [no test files]
?       github.com/equinix/metal-cli/internal/vlan  [no test files]
=== RUN   TestCli_Capacity
=== RUN   TestCli_Capacity/get
=== RUN   TestCli_Capacity/get_by_plan_1
=== RUN   TestCli_Capacity/get_by_plan_2
=== RUN   TestCli_Capacity/check_by_multi_metro
=== RUN   TestCli_Capacity/check_by_multi_plan
=== RUN   TestCli_Capacity/check_by_multi_metro_and_plan
--- PASS: TestCli_Capacity (8.92s)
    --- PASS: TestCli_Capacity/get (1.72s)
    --- PASS: TestCli_Capacity/get_by_plan_1 (2.64s)
    --- PASS: TestCli_Capacity/get_by_plan_2 (3.01s)
    --- PASS: TestCli_Capacity/check_by_multi_metro (0.47s)
    --- PASS: TestCli_Capacity/check_by_multi_plan (0.48s)
    --- PASS: TestCli_Capacity/check_by_multi_metro_and_plan (0.59s)
PASS
ok      github.com/equinix/metal-cli/test/e2e/capacitytest  (cached)
=== RUN   TestCli_Devices_Create_Flags
=== RUN   TestCli_Devices_Create_Flags/create_device
--- PASS: TestCli_Devices_Create_Flags (121.10s)
    --- PASS: TestCli_Devices_Create_Flags/create_device (121.10s)
PASS
ok      github.com/equinix/metal-cli/test/e2e/devices/devicecreateflagstest 122.105s
=== RUN   TestCli_Devices_Create
=== RUN   TestCli_Devices_Create/create_device
--- PASS: TestCli_Devices_Create (225.97s)
    --- PASS: TestCli_Devices_Create/create_device (225.97s)
PASS
ok      github.com/equinix/metal-cli/test/e2e/devices/devicecreatetest  226.697s
=== RUN   TestCli_Devices_Get
=== RUN   TestCli_Devices_Get/get_by_device_id
--- PASS: TestCli_Devices_Get (184.92s)
    --- PASS: TestCli_Devices_Get/get_by_device_id (184.92s)
PASS
ok      github.com/equinix/metal-cli/test/e2e/devices/devicegettest 186.207s
=== RUN   TestCli_Devices_Update
=== RUN   TestCli_Devices_Update/reinstall_device
--- PASS: TestCli_Devices_Update (775.33s)
    --- PASS: TestCli_Devices_Update/reinstall_device (775.33s)
PASS
ok      github.com/equinix/metal-cli/test/e2e/devices/devicereinstalltest   777.436s
=== RUN   TestCli_Devices_Update
=== RUN   TestCli_Devices_Update/start_device
--- PASS: TestCli_Devices_Update (192.14s)
    --- PASS: TestCli_Devices_Update/start_device (192.14s)
PASS
ok      github.com/equinix/metal-cli/test/e2e/devices/devicestarttest   193.709s
=== RUN   TestCli_Devices_Update
=== RUN   TestCli_Devices_Update/stop_device
--- PASS: TestCli_Devices_Update (299.97s)
    --- PASS: TestCli_Devices_Update/stop_device (299.97s)
PASS
ok      github.com/equinix/metal-cli/test/e2e/devices/devicestoptest    302.922s
=== RUN   TestCli_Devices_Update
=== RUN   TestCli_Devices_Update/update_device
--- PASS: TestCli_Devices_Update (209.89s)
    --- PASS: TestCli_Devices_Update/update_device (209.89s)
PASS
ok      github.com/equinix/metal-cli/test/e2e/devices/deviceupdatetest  212.551s
=== RUN   TestCli_Events_Get
=== RUN   TestCli_Events_Get/get_events_by_dev_id
--- PASS: TestCli_Events_Get (207.06s)
    --- PASS: TestCli_Events_Get/get_events_by_dev_id (207.06s)
PASS
ok      github.com/equinix/metal-cli/test/e2e/events/deviceeventstest   207.497s
=== RUN   TestCli_Events_Get
=== RUN   TestCli_Events_Get/get_events_by_proj_id
--- PASS: TestCli_Events_Get (2.36s)
    --- PASS: TestCli_Events_Get/get_events_by_proj_id (2.36s)
PASS
ok      github.com/equinix/metal-cli/test/e2e/events/projecteventstest  4.735s
=== RUN   TestCli_Ips_Get
=== RUN   TestCli_Ips_Get/get_ip_reservations
--- PASS: TestCli_Ips_Get (3.46s)
    --- PASS: TestCli_Ips_Get/get_ip_reservations (3.46s)
=== RUN   TestCli_Vlan_Create
=== RUN   TestCli_Vlan_Create/Request_NewIP
--- PASS: TestCli_Vlan_Create (12.19s)
    --- PASS: TestCli_Vlan_Create/Request_NewIP (12.19s)
PASS
ok      github.com/equinix/metal-cli/test/e2e/ipstest   17.471s
=== RUN   TestCli_Metros
=== RUN   TestCli_Metros/get
--- PASS: TestCli_Metros (0.53s)
    --- PASS: TestCli_Metros/get (0.53s)
PASS
ok      github.com/equinix/metal-cli/test/e2e/metrotest (cached)
=== RUN   TestCli_Organization
=== RUN   TestCli_Organization/get
+--------------------------------------+---------------------------------+-------------------------------+
|                  ID                  |              NAME               |            CREATED            |
+--------------------------------------+---------------------------------+-------------------------------+
| 4d12f460-8c5e-43ea-986d-529d328815ee | DevRel Engineering (Playground) | 2023-08-03 20:50:33 +0000 UTC |
+--------------------------------------+---------------------------------+-------------------------------+
--- PASS: TestCli_Organization (0.54s)
    --- PASS: TestCli_Organization/get (0.54s)
PASS
ok      github.com/equinix/metal-cli/test/e2e/organizationtest  (cached)
=== RUN   TestCli_OperatingSystem
=== RUN   TestCli_OperatingSystem/get
--- PASS: TestCli_OperatingSystem (0.83s)
    --- PASS: TestCli_OperatingSystem/get (0.83s)
PASS
ok      github.com/equinix/metal-cli/test/e2e/ostest    (cached)
=== RUN   TestCli_Plans
=== RUN   TestCli_Plans/get
=== RUN   TestCli_Plans/get_by_slug
=== RUN   TestCli_Plans/get_by_type
--- PASS: TestCli_Plans (3.25s)
    --- PASS: TestCli_Plans/get (1.90s)
    --- PASS: TestCli_Plans/get_by_slug (0.35s)
    --- PASS: TestCli_Plans/get_by_type (1.00s)
PASS
ok      github.com/equinix/metal-cli/test/e2e/plantest  (cached)
=== RUN   TestCli_SshKey
=== RUN   TestCli_SshKey/get
--- PASS: TestCli_SshKey (0.44s)
    --- PASS: TestCli_SshKey/get (0.44s)
PASS
ok      github.com/equinix/metal-cli/test/e2e/sshtest   (cached)
=== RUN   TestCli_Users
=== RUN   TestCli_Users/get
--- PASS: TestCli_Users (0.59s)
    --- PASS: TestCli_Users/get (0.59s)
PASS
ok      github.com/equinix/metal-cli/test/e2e/usertest  (cached)
?       github.com/equinix/metal-cli/test/helper    [no test files]
ctreatma commented 1 year ago

I'll go ahead and merge this; it's already been tested in metal-cli and we don't have a viable alternative. We can revisit after #159 is addressed.

github-actions[bot] commented 1 year ago

This PR is included in version 0.23.1 :tada: