Copilot commented on code in PR #2170:
URL: https://github.com/apache/libcloud/pull/2170#discussion_r3643472235
##########
libcloud/compute/drivers/maxihost.py:
##########
@@ -35,7 +35,7 @@ class MaxihostNodeDriver(NodeDriver):
name = "Maxihost"
website = "https://www.maxihost.com/";
- def create_node(self, name, size, image, location, ex_ssh_key_ids=None):
+ def create_node(self, name, size, image, location=None, auth=None,
ex_ssh_key_ids=None):
"""
Create a node.
Review Comment:
`create_node()` now defaults `location=None`, but the implementation
unconditionally accesses `location.id.lower()` when building the request
payload. Calling `create_node()` without a location will now raise an
`AttributeError` instead of a clear error / TypeError, which is a behavior
regression.
##########
libcloud/compute/drivers/maxihost.py:
##########
@@ -214,7 +220,11 @@ def list_key_pairs(self):
data = self.connection.request("/account/keys")
return list(map(self._to_key_pair, data.object["ssh_keys"]))
- def create_key_pair(self, name, public_key):
+ def create_key_pair(
+ self,
+ name,
+ public_key=None,
+ ):
Review Comment:
`create_key_pair()` now makes `public_key` optional (`None` by default) but
still always sends it to the API. This means `create_key_pair(name)` will now
make an invalid request instead of failing fast. If this driver requires a
public key to be provided, keep the parameter required (or validate and raise a
clear error when missing).
##########
libcloud/compute/drivers/equinixmetal.py:
##########
@@ -416,7 +424,11 @@ def list_key_pairs(self):
return list(map(self._to_key_pairs, data))
- def create_key_pair(self, name, public_key):
+ def create_key_pair(
+ self,
+ name,
+ public_key=None,
+ ):
Review Comment:
`create_key_pair()` now makes `public_key` optional (`None` by default) but
the request always includes it. This can result in confusing API errors when
callers use the base `NodeDriver.create_key_pair(name)` contract (which
supplies no public key). If a public key is required for this provider, the
method should fail fast with a clear error (or keep the parameter required).
##########
libcloud/compute/drivers/outscale.py:
##########
@@ -1675,11 +1680,13 @@ def ex_update_snapshot(
def create_volume(
self,
- ex_subregion_name: str,
+ size,
+ name,
+ location=None,
+ snapshot=None,
+ ex_subregion_name: str = None,
ex_dry_run: bool = False,
ex_iops: int = None,
Review Comment:
`create_volume()` still always sends `SubregionName` from
`ex_subregion_name`, but the signature now defaults it to `None`. This makes it
easy to call `create_volume(size, name, location=...)` and still send
`SubregionName=None`, which will likely fail at the API with a less actionable
error. Consider making `ex_subregion_name` explicitly required (e.g., as a
required keyword-only argument) or deriving it from `location`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
