Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for filtering OS images by instance type via ImagesService.get(instance_type=...) and refactors the Image model to a dataclass using dataclasses_json for (de)serialization consistency.
Changes:
- Refactor
verda.images.Imageto@dataclass+@dataclass_jsonand switch parsing toImage.from_dict(...). - Add optional
instance_typequery parameter toImagesService.get()and corresponding unit test. - Document the feature and refactor in
CHANGELOG.md.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
verda/images/_images.py |
Converts Image to a dataclass/json model and adds instance_type filtering to the images list call. |
tests/unit_tests/images/test_images.py |
Refactors existing test payload reuse and adds a test asserting the instance_type query param is sent. |
CHANGELOG.md |
Notes the new query capability and the Image dataclass refactor under Unreleased. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @dataclass_json(undefined=Undefined.EXCLUDE) | ||
| @dataclass | ||
| class Image: | ||
| """An image model class.""" | ||
|
|
||
| def __init__(self, id: str, name: str, image_type: str, details: list[str]) -> None: | ||
| """Initialize an image object. | ||
|
|
||
| :param id: image id | ||
| :type id: str | ||
| :param name: image name | ||
| :type name: str | ||
| :param image_type: image type, e.g. 'ubuntu-20.04-cuda-11.0' | ||
| :type image_type: str | ||
| :param details: image details | ||
| :type details: list[str] | ||
| """ | ||
| self._id = id | ||
| self._name = name | ||
| self._image_type = image_type | ||
| self._details = details | ||
|
|
||
| @property | ||
| def id(self) -> str: | ||
| """Get the image id. | ||
|
|
||
| :return: image id | ||
| :rtype: str | ||
| """ | ||
| return self._id | ||
|
|
||
| @property | ||
| def name(self) -> str: | ||
| """Get the image name. | ||
|
|
||
| :return: image name | ||
| :rtype: str | ||
| """ | ||
| return self._name | ||
|
|
||
| @property | ||
| def image_type(self) -> str: | ||
| """Get the image type. | ||
| """Represents an OS image available for instances. | ||
|
|
There was a problem hiding this comment.
The refactor removes Image.str() (previously returned a JSON-formatted string). If any SDK consumers relied on str(image) producing JSON (e.g., for logging/serialization), this is a backward-incompatible behavior change. Consider restoring a JSON-oriented str (e.g., delegating to dataclasses_json's serialization) or explicitly calling out this behavior change in the changelog/migration notes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for image in images | ||
| ] | ||
| return image_objects | ||
| params = {'instance_type': instance_type} if instance_type else None |
There was a problem hiding this comment.
params = {'instance_type': instance_type} if instance_type else None treats empty strings as “no filter”. If a caller passes instance_type='' (or any other falsy-but-valid string), the SDK will silently omit the query param. Prefer checking instance_type is not None (or always passing {'instance_type': instance_type} and letting requests drop None values) so only None means “no filter.”
| params = {'instance_type': instance_type} if instance_type else None | |
| params = {'instance_type': instance_type} if instance_type is not None else None |
Added
verda.images.get(instance_type=...)Changed
Imagemodel to use@dataclassand@dataclass_jsonfor consistency withInstanceandVolume