-
Notifications
You must be signed in to change notification settings - Fork 927
Comments
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:trunk from
Conversation
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.
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
Copilot
AI
Feb 17, 2026
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 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.
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.
Remove logging import and calls, drivers do not call logging.
Copilot
AI
Feb 17, 2026
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.
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.
Copilot
AI
Feb 17, 2026
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the logic behind the location being empty?
May be raising an error will be better...
libcloud/compute/drivers/ovh.py
Outdated
Copilot
AI
Feb 17, 2026
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.
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
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.
Remove logging import and calls, drivers do not call logging.
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.
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.
codecov-commenter
commented
Feb 18, 2026
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@ ## trunk #2123 +/- ## ========================================== - Coverage 83.36% 83.36% -0.00% ========================================== Files 353 353 Lines 81876 81878 +2 Branches 9003 9004 +1 ========================================== Hits 68250 68250 - Misses 10798 10799 +1 - Partials 2828 2829 +1
🚀 New features to boost your workflow:
|
Uh oh!
There was an error while loading. Please reload this page.
Description
add more location to OVH and avoid exception when location is not in the list
Status
Checklist (tick everything that applies)