Conversation
There was a problem hiding this comment.
Pull request overview
Adds a provider-level API for enumerating existing provider instances (to support cleanup workflows requested in #639), with initial implementations for LXD and Multipass and a new LXD unit test.
Changes:
- Introduce
Provider.list_instances()as a new abstract method. - Implement
list_instances()forLXDProviderandMultipassProvider. - Add
__repr__methods forLXDInstanceandMultipassInstance, plus a unit test for LXD listing.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/lxd/test_lxd_provider.py | Adds a unit test covering LXDProvider.list_instances(). |
| craft_providers/provider.py | Adds the new abstract list_instances() API to the provider base class. |
| craft_providers/multipass/multipass_provider.py | Implements MultipassProvider.list_instances(). |
| craft_providers/multipass/multipass_instance.py | Adds MultipassInstance.__repr__. |
| craft_providers/lxd/lxd_provider.py | Implements LXDProvider.list_instances(). |
| craft_providers/lxd/lxd_instance.py | Adds LXDInstance.__repr__. |
Comments suppressed due to low confidence (1)
craft_providers/provider.py:42
Providernow has duplicateTYPE_CHECKINGimports / blocks in this area, which makes the import section confusing and can lead to inconsistent type-only imports. Please consolidate into a singlefrom typing import TYPE_CHECKINGand a singleif TYPE_CHECKING:block with the needed annotation-only imports.
from typing import TYPE_CHECKING
from .base import Base
if TYPE_CHECKING:
from collections.abc import Callable, Collection
from craft_providers import Executor
from typing import TYPE_CHECKING
if TYPE_CHECKING:
import pathlib
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
craft_providers/provider.py
Outdated
| def list_instances(self) -> Collection[Executor]: | ||
| """Get a collection of existing instances for this provider.""" | ||
|
|
There was a problem hiding this comment.
The new list_instances() abstract method has no way to scope results to the current application or to control inclusion of LXD base instances (the issue calls out “base containers…behind a flag” and multipass filtering by relevant name). Consider extending the API to accept scoping inputs (e.g. project_name/instance_name_prefix) plus an include_base_instances: bool = False flag so providers can return only relevant instances by default.
| def list_instances(self) -> Collection[Executor]: | |
| """Get a collection of existing instances for this provider.""" | |
| def list_instances( | |
| self, | |
| *, | |
| project_name: str | None = None, | |
| instance_name_prefix: str | None = None, | |
| include_base_instances: bool = False, | |
| ) -> Collection[Executor]: | |
| """Get a collection of existing instances for this provider. | |
| :param project_name: Optional project name to scope instances to a specific | |
| application or project, if supported by the provider. | |
| :param instance_name_prefix: Optional prefix to filter instances by name. | |
| :param include_base_instances: If True, include any base instances created | |
| by the provider; if False (default), base instances should be excluded | |
| from the results when possible. | |
| """ |
craft_providers/lxd/lxd_provider.py
Outdated
| def list_instances(self) -> Collection[LXDInstance]: | ||
| """Get a collection of all existing instances for this LXD provider.""" | ||
| return [ | ||
| LXDInstance(name=name, project=self.lxd_project, remote=self.lxd_remote) | ||
| for name in self.lxc.list_names( | ||
| project=self.lxd_project, remote=self.lxd_remote | ||
| ) | ||
| ] |
There was a problem hiding this comment.
LXDProvider.list_instances() constructs LXDInstance objects without passing the provider’s existing self.lxc (and without a shared pylxd client), so each instance will create its own LXC() wrapper (and its own pylxd.Client). This adds unnecessary overhead when listing many instances; consider passing lxc=self.lxc and reusing a single client instance across the returned executors.
craft_providers/lxd/lxd_provider.py
Outdated
| def list_instances(self) -> Collection[LXDInstance]: | ||
| """Get a collection of all existing instances for this LXD provider.""" | ||
| return [ | ||
| LXDInstance(name=name, project=self.lxd_project, remote=self.lxd_remote) | ||
| for name in self.lxc.list_names( | ||
| project=self.lxd_project, remote=self.lxd_remote | ||
| ) | ||
| ] |
There was a problem hiding this comment.
Issue #639 specifies that LXD “base containers [should be] behind a flag”, but this implementation returns all instances in the project, including base instances created by craft-providers. Consider excluding base instances by default (e.g. filter names starting with base-instance-) and adding an explicit opt-in flag to include them.
| self._multipass = Multipass() | ||
|
|
||
| def __repr__(self) -> str: | ||
| return f"{self.__class__.__name__}(name={self.name!r}, instance_name={self.instance_name})" |
There was a problem hiding this comment.
MultipassInstance.__repr__ currently formats instance_name without !r, so the repr output is ambiguous for names containing special characters and is inconsistent with the name field. Use instance_name={self.instance_name!r} for a proper, unambiguous repr.
| return f"{self.__class__.__name__}(name={self.name!r}, instance_name={self.instance_name})" | |
| return f"{self.__class__.__name__}(name={self.name!r}, instance_name={self.instance_name!r})" |
craft_providers/lxd/lxd_instance.py
Outdated
| def __repr__(self) -> str: | ||
| return f"{self.__class__.__name__}(name={self.name!r}, instance_name={self.instance_name!r}, default_command_environment={self.default_command_environment!r}, project={self.project!r}, remote={self.remote!r})" |
There was a problem hiding this comment.
LXDInstance.__repr__ includes default_command_environment, which can be large and tends to make debug logs/noise harder to read when instances are logged. Consider limiting the repr to stable identifiers (e.g. instance_name, project, remote) and omitting large mutable fields.
| def list_instances(self) -> Collection[MultipassInstance]: | ||
| """Get a collection of all existing multipass VMs.""" | ||
| return [ | ||
| MultipassInstance(name=name, multipass=self.multipass) | ||
| for name in self.multipass.list() | ||
| ] |
There was a problem hiding this comment.
Issue #639 calls for listing only provider instances relevant to the current application (for multipass: VMs beginning with a relevant name), but MultipassProvider.list_instances() currently wraps and returns all VMs from multipass list. This can surface unrelated VMs and make cleanup dangerous; consider adding a prefix/project scoping mechanism and filtering to only those instances by default.
| def list_instances(self) -> Collection[MultipassInstance]: | ||
| """Get a collection of all existing multipass VMs.""" | ||
| return [ | ||
| MultipassInstance(name=name, multipass=self.multipass) | ||
| for name in self.multipass.list() | ||
| ] |
There was a problem hiding this comment.
MultipassProvider.list_instances() introduces new behavior but there’s no unit test coverage for it (and tests exist for other MultipassProvider methods). Please add a test in tests/unit/multipass/test_multipass_provider.py that stubs multipass.list() and asserts the returned instances (and any filtering semantics, once added).
| def test_lxd_list_instances(mock_lxc_container): | ||
| """Verify LXDProvider's instances""" | ||
| provider = LXDProvider( | ||
| lxc=mock_lxc_container, | ||
| lxd_project="default", | ||
| lxd_remote="local", | ||
| ) | ||
|
|
||
| instances = list(provider.list_instances()) | ||
|
|
There was a problem hiding this comment.
The new test_lxd_list_instances verifies returned instance names but doesn’t assert that lxc.list_names was called with the provider’s lxd_project/lxd_remote. Adding an assertion on the mock call would better lock in the intended behavior.
bepri
left a comment
There was a problem hiding this comment.
Good work so far :). Copilot's comments seem reasonable to me, could you incorporate those?
craft_providers/lxd/lxd_instance.py
Outdated
|
|
||
| self._client = client or pylxd.Client(project=self.project) | ||
|
|
||
| def __repr__(self) -> str: |
There was a problem hiding this comment.
I believe this was just left behind for debugging, can you remove it now?
Also, as a tip, Python has a nice shorthand syntax for the var={var!r} pattern:
print(f"{var=}")| else: | ||
| self._multipass = Multipass() | ||
|
|
||
| def __repr__(self) -> str: |
make lint && make test?Closes #639