Skip to content
This repository has been archived by the owner on Jun 28, 2024. It is now read-only.

fix: Added IPAssignments type #153

Merged
merged 1 commit into from
Sep 28, 2023
Merged

Conversation

codinja1188
Copy link
Contributor

No description provided.

@displague
Copy link
Member

displague commented Sep 21, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

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]

Copy link
Contributor

@ctreatma ctreatma left a comment

Choose a reason for hiding this comment

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

@Vasubabu I was going to ask if you had a chance to test with @displague's suggestion to mark assigned_to as required, but then I remembered that required field validation doesn't work in this SDK.

I created #159 to better track the required field issue (there was already a PR to experiment with implementing it, but having an issue for it will make it more visible). Given that issue, I'm OK with keeping the type workaround you've implemented, but I'd like @displague to give a 👍 or 👎 before this gets merged.

@ctreatma
Copy link
Contributor

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.

@ctreatma ctreatma merged commit 0145602 into equinix-labs:main Sep 28, 2023
2 checks passed
@github-actions
Copy link
Contributor

This PR is included in version 0.23.1 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants