Copilot commented on code in PR #2123:
URL: https://github.com/apache/libcloud/pull/2123#discussion_r2816170698
##########
libcloud/compute/drivers/ovh.py:
##########
@@ -29,6 +29,8 @@
from libcloud.compute.types import Provider, StorageVolumeState,
VolumeSnapshotState
from libcloud.compute.drivers.openstack import OpenStackKeyPair,
OpenStackNodeDriver
+from libcloud.libcloud.utils import logging
Review Comment:
The import path is incorrect and contains a duplicate 'libcloud' segment.
This should be `import logging` (standard library) or `from
libcloud.utils.logging import ...` if using a custom logging utility. The
current path `from libcloud.libcloud.utils import logging` will cause an
ImportError at runtime.
```suggestion
import logging
```
##########
libcloud/common/ovh.py:
##########
@@ -63,6 +63,24 @@
"SYD1": {"id": "SYD1", "name": "Sydney 1", "country": "AU"},
"VIN1": {"id": "VIN1", "name": "Vint Hill, Virginia 1", "country": "US"},
"WAW1": {"id": "WAW1", "name": "Warsaw 1", "country": "PL"},
+
Review Comment:
This line appears to have trailing whitespace. Remove the trailing spaces to
maintain code cleanliness and comply with standard linting rules.
```suggestion
```
##########
libcloud/compute/drivers/ovh.py:
##########
@@ -537,7 +539,10 @@ def _to_volumes(self, objs):
return [self._to_volume(obj) for obj in objs]
def _to_location(self, obj):
- location = self.connectionCls.LOCATIONS[obj]
+ location = self.connectionCls.LOCATIONS.get(obj)
+ if not location:
+ logging.warning(f"Unknown location {obj} for OVH")
Review Comment:
After fixing the import statement, you should create a module-level logger
instance (e.g., `logger = logging.getLogger(__name__)`) and use
`logger.warning()` instead of calling `logging.warning()` directly. This
follows the established pattern in the codebase (see
libcloud/compute/drivers/vsphere.py:53 and libcloud/common/google.py:103).
##########
libcloud/common/ovh.py:
##########
@@ -63,6 +63,24 @@
"SYD1": {"id": "SYD1", "name": "Sydney 1", "country": "AU"},
"VIN1": {"id": "VIN1", "name": "Vint Hill, Virginia 1", "country": "US"},
"WAW1": {"id": "WAW1", "name": "Warsaw 1", "country": "PL"},
+
+ "BHS": {"id": "BHS", "name": "Beauharnois, Quebec", "country": "CA"},
+ "CA-EAST-TOR": {"id": "CA-EAST-TOR", "name": "Toronto, Canada", "country":
"CA"},
+ "DE": {"id": "DE", "name": "Germany", "country": "DE"},
+ "DE1": {"id": "DE1", "name": "Germany 1", "country": "DE"},
+ "EU-SOUTH-MIL": {"id": "EU-SOUTH-MIL", "name": "Milan, Italy", "country":
"IT"},
+ "EU-WEST-PAR": {"id": "EU-WEST-PAR", "name": "Paris, France", "country":
"FR"},
+ "GRA": {"id": "GRA", "name": "Gravelines", "country": "FR"},
+ "GRA9": {"id": "GRA9", "name": "Gravelines 9", "country": "FR"},
+ "GRA11": {"id": "GRA11", "name": "Gravelines 11", "country": "FR"},
+ "RBX": {"id": "RBX", "name": "Roubaix", "country": "FR"},
+ "RBX-A": {"id": "RBX-A", "name": "Roubaix A", "country": "FR"},
+ "RBX-ARCHIVE": {"id": "RBX-ARCHIVE", "name": "Roubaix Archive", "country":
"FR"},
+ "SBG": {"id": "SBG", "name": "Strasbourg", "country": "FR"},
+ "SBG5": {"id": "SBG5", "name": "Strasbourg 5", "country": "FR"},
+ "UK": {"id": "UK", "name": "United Kingdom", "country": "UK"},
+ "UK1": {"id": "UK1", "name": "United Kingdom 1", "country": "UK"},
Review Comment:
Using 'UK' as a country code is inconsistent with ISO 3166-1 alpha-2
standards, which uses 'GB' for the United Kingdom. However, this appears to be
an existing pattern in the codebase (see line 50 with LON1). For consistency
with existing entries, this may be acceptable, but consider using 'GB' for
proper ISO compliance in future additions.
```suggestion
"UK": {"id": "UK", "name": "United Kingdom", "country": "GB"},
"UK1": {"id": "UK1", "name": "United Kingdom 1", "country": "GB"},
```
##########
libcloud/compute/drivers/ovh.py:
##########
@@ -537,7 +539,10 @@ def _to_volumes(self, objs):
return [self._to_volume(obj) for obj in objs]
def _to_location(self, obj):
- location = self.connectionCls.LOCATIONS[obj]
+ location = self.connectionCls.LOCATIONS.get(obj)
+ if not location:
+ logging.warning(f"Unknown location {obj} for OVH")
+ location = {"id": obj, "name": obj, "country": ""}
Review Comment:
The new fallback logic for unknown locations lacks test coverage. Consider
adding a test case that verifies the behavior when an unknown location is
encountered, ensuring that it creates a NodeLocation with the location ID as
both id and name, an empty country field, and logs an appropriate warning
message.
--
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]
