-
Notifications
You must be signed in to change notification settings - Fork 927
Comments
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 pull request uniformizes the node creation API in the OVH driver by adding support for the auth parameter, aligning it with the pattern used in the EC2 driver. The change allows users to pass authentication credentials directly through the auth parameter instead of requiring the ex_keyname parameter, providing a more consistent API across different cloud providers.
Changes:
- Added
authparameter support to thecreate_nodemethod with mutual exclusivity validation againstex_keyname - Implemented
ex_find_or_import_keypair_by_key_materialhelper method to automatically find or create SSH keypairs based on public key material - Updated imports to include public key utility functions for fingerprint calculation and comment extraction
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Missing documentation for the new public method ex_find_or_import_keypair_by_key_material. Public methods should have docstrings that describe their purpose, parameters, return values, and any exceptions they may raise. Consider following the pattern from the EC2 driver's implementation for consistency.
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.
Missing test coverage for the new auth parameter functionality. The existing test_create_node test does not cover the new authentication parameter flow. Consider adding tests for: 1) creating a node with the auth parameter and a new keypair, 2) creating a node with the auth parameter when the keypair already exists, and 3) the mutual exclusivity validation that raises AttributeError when both auth and ex_keyname are provided.
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.
Trailing whitespace detected. Remove the trailing whitespace at the end of this line to maintain consistency with the rest of the codebase and adhere to Python style guidelines (PEP 8).
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.
Trailing whitespace detected. Remove the trailing whitespace at the end of this line to maintain consistency with the rest of the codebase and adhere to Python style guidelines (PEP 8).
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.
Potential issue with key name collision. If the same public key material already exists in OVH but with a different name than the generated one (computed from fingerprint and comment), attempting to import it on line 166 may fail if OVH enforces uniqueness on public key material. Consider handling potential exceptions from import_key_pair_from_string and checking if the failure is due to a duplicate key, in which case you should search for the existing key by its fingerprint or public key material and return that instead.
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.
Missing space after comma. Should be self.ex_find_or_import_keypair_by_key_material(location, auth.pubkey) for consistency with Python style guidelines (PEP 8).
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.
Missing space around assignment operator. Should be key_pair = self.import_key_pair_from_string(...) for consistency with Python style guidelines (PEP 8).
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.
Inconsistent return value structure and potential issue with None value. Line 162 returns keyId (from key_pair.extra["id"]) but line 167 returns keyFingerprint (from key_pair.fingerprint). However, the _to_key_pair method (line 612) sets fingerprint=None for OVH keypairs, so this will return None instead of a valid fingerprint value. Both code paths should return the same dictionary structure with the same keys. Since the calling code on line 149 only uses key["keyName"], you should either: 1) return keyId in both cases (preferred, for consistency), or 2) ensure that only keyName is used and remove the extra keys from the return values.
OVH does not support : in keyname
Description
the old format of create nodes in OVH need ex_keyname which is changed to auth so based on the implementation on EC2 i have implement this for ovh
Status
Checklist (tick everything that applies)