-
Notifications
You must be signed in to change notification settings - Fork 50
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
Add support for Metros #110
Conversation
Signed-off-by: Marques Johansson <mjohansson@equinix.com>
Signed-off-by: Marques Johansson <mjohansson@equinix.com>
Signed-off-by: Marques Johansson <mjohansson@equinix.com>
if metro != "": | ||
params["metro"] = metro | ||
if facility != "": | ||
params["facility"] = facility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be ok to have both metro and facility? What if they are incompatible? IIRC for these types of things we let the API handle the error checking/response right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there's no check to ensure at least one of facility or metro is specified. This can be seen as a bit of change in behavior where previously facility was required and check by Python.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to send {"metro": "", "facility": "ewr1"}
, for example. It results in " is not a valid metro"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we avoid sending an empty string for metro or facility, by not including it in the JSON payload.
We're deliberately working around the previously required facility
field.
Perhaps facility = None
and metro = None
would be better defaults? We don't get the same API error with {"metro": null}
as payload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sending null
for this field should probably mean something else, and we would be getting lucky that the API treats this as a field omission (in this case). It's not documented.
I think we should try to avoid sending any value if we don't want a value (not include metro or facility in the payload in the presence of the other).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to send
{"metro": "", "facility": "ewr1"}
, for example. It results in" is not a valid metro"
What about:
if not metro and not facility:
# raise an error, one of metro or facility must be specified.
We're deliberately working around the previously required
facility
field.
Why? It seems to me that one of metro or facility must be required, otherwise the request is garbage right?
I guess similarly, we can choose to require one-and-only-one of metro or facility to be provided (though I'm on the fence on this tbh, the user could call metro = NY
, facility = NY5
and thats fine (if those are actually valid values), so I'm ok with letting the API do the validity check here))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The API does not allow for metro + facility in the request, even if the facility is currently included in that metro.
It's not enough to send empty strings for the opposite value. And sending null
for the opposite value relies on undefined behavior.
The request that we send must either contain a non-empty facility or a non-empty metro or else the request is not valid according to the API.
One of those fields (facility
) was previously required, so now it has been made optional with a default value.
This client needs to accept both fields in the parameter list and omit one of those fields from the request. The user can't toggle that behavior with the param list alone, so the function needs logic to do it.
In Packngo we got this for free at serialization time via:
type CreateDeviceInput struct {
Facilities []string `json:"facilities,omitempty"`
Metro string `json:"metro,omitempty"`
}
The conditional params
setting in create_device
is mimicking that behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm ok with letting the API do the validity check here
It results in " is not a valid metro"
-- this is an API error (I didn't make that clear before).
The API is handling the validation in all cases, python is not going to throw an early error.
- If you try to send both
facility
andmetro
tocreate_device
, we'll pass it along to the API. And you'll get an error. - If you send neither to
create_device
, the API will give you the error. - If you send
""
to either, the API will give you the error.
if facility != "": | ||
params["facility"] = facility | ||
if metro != "": | ||
params["metro"] = metro |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
if facility != "": | ||
request["facility"] = facility | ||
if metro != "": | ||
request["metro"] = metro |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
facility = data.get("facility", None) | ||
self.facility = Facility(facility) if facility else None | ||
metro = data.get("metro", None) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
facility = data.get("facility", None) | |
self.facility = Facility(facility) if facility else None | |
metro = data.get("metro", None) | |
facility = data.get("facility") | |
self.facility = Facility(facility) if facility else None | |
metro = data.get("metro") |
Co-authored-by: Manuel Mendez <708570+mmlb@users.noreply.github.com> Signed-off-by: Marques Johansson <mjohansson@equinix.com>
In case fetch ips for a project with `state=all` a server returns objects with null facility. It happens when IP in Pending status. Signed-off-by: Dmitry Tyzhnenko <t.dmitry@gmail.com>
Adds support for Metros: https://feedback.equinixmetal.com/changelog/new-metros-feature-live
Fixes #108
Closes #116
Closes #117