add more location to OVH and avoid exception when location is not in the list#2123
add more location to OVH and avoid exception when location is not in the list#2123ali-corpo wants to merge 3 commits intoapache:trunkfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR enhances the OVH driver to support additional datacenter locations and implements graceful handling of unknown locations instead of raising exceptions.
Changes:
- Added 17 new location entries to the LOCATIONS dictionary, including parent regions (BHS, GRA, RBX, SBG, WAW) and specific locations (CA-EAST-TOR, EU-SOUTH-MIL, EU-WEST-PAR, etc.)
- Modified the
_to_locationmethod to use.get()for safe dictionary access and fall back to creating a basic location object when encountering unknown location codes - Added logging to warn when unknown locations are encountered
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| libcloud/common/ovh.py | Added 17 new OVH datacenter location mappings to expand geographic coverage |
| libcloud/compute/drivers/ovh.py | Implemented graceful fallback for unknown locations with logging support |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
libcloud/compute/drivers/ovh.py
Outdated
| from libcloud.compute.types import Provider, StorageVolumeState, VolumeSnapshotState | ||
| from libcloud.compute.drivers.openstack import OpenStackKeyPair, OpenStackNodeDriver | ||
|
|
||
| from libcloud.libcloud.utils import logging |
There was a problem hiding this 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.
| from libcloud.libcloud.utils import logging | |
| import logging |
There was a problem hiding this comment.
Remove logging import and calls, drivers do not call logging.
| "UK": {"id": "UK", "name": "United Kingdom", "country": "UK"}, | ||
| "UK1": {"id": "UK1", "name": "United Kingdom 1", "country": "UK"}, |
There was a problem hiding this 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.
| "UK": {"id": "UK", "name": "United Kingdom", "country": "UK"}, | |
| "UK1": {"id": "UK1", "name": "United Kingdom 1", "country": "UK"}, | |
| "UK": {"id": "UK", "name": "United Kingdom", "country": "GB"}, | |
| "UK1": {"id": "UK1", "name": "United Kingdom 1", "country": "GB"}, |
| location = self.connectionCls.LOCATIONS.get(obj) | ||
| if not location: | ||
| logging.warning(f"Unknown location {obj} for OVH") | ||
| location = {"id": obj, "name": obj, "country": ""} |
There was a problem hiding this 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.
There was a problem hiding this comment.
What's the logic behind the location being empty?
May be raising an error will be better...
libcloud/compute/drivers/ovh.py
Outdated
| location = self.connectionCls.LOCATIONS[obj] | ||
| location = self.connectionCls.LOCATIONS.get(obj) | ||
| if not location: | ||
| logging.warning(f"Unknown location {obj} for OVH") |
There was a problem hiding this 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/compute/drivers/ovh.py
Outdated
| from libcloud.compute.types import Provider, StorageVolumeState, VolumeSnapshotState | ||
| from libcloud.compute.drivers.openstack import OpenStackKeyPair, OpenStackNodeDriver | ||
|
|
||
| from libcloud.libcloud.utils import logging |
There was a problem hiding this comment.
Remove logging import and calls, drivers do not call logging.
| location = self.connectionCls.LOCATIONS.get(obj) | ||
| if not location: | ||
| logging.warning(f"Unknown location {obj} for OVH") | ||
| location = {"id": obj, "name": obj, "country": ""} |
There was a problem hiding this comment.
What's the logic behind the location being empty?
May be raising an error will be better...
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Removed logging warning for unknown OVH locations.
2 participants
Description
add more location to OVH and avoid exception when location is not in the list
Status
Checklist (tick everything that applies)