-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: allow use of interceptable mknod
in ProviderService
#607
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -51,9 +51,14 @@ | |
class ProviderService(base.ProjectService): | ||
"""Manager for craft_providers in an application. | ||
|
||
:param app: Metadata about this application. | ||
:param app: Metadata about this application | ||
:param services: Factory class for lazy-loading service classes | ||
:param project: The project model | ||
:param work_dir: The working directory for parts processing | ||
:param build_plan: The filtered build plan of platforms that are valid for | ||
:param provider_name: The provider to use - "lxd" or "multipass" | ||
:param install_snap: Whether to install this app's snap from the host (default True) | ||
:param intercept_mknod: If the host can, tell LXD instance to intercept mknod | ||
""" | ||
|
||
managed_mode_env_var = platforms.ENVIRONMENT_CRAFT_MANAGED_MODE | ||
|
@@ -68,13 +73,16 @@ def __init__( | |
build_plan: list[models.BuildInfo], | ||
provider_name: str | None = None, | ||
install_snap: bool = True, | ||
intercept_mknod: bool = True, | ||
) -> None: | ||
super().__init__(app, services, project=project) | ||
self._provider: craft_providers.Provider | None = None | ||
self._work_dir = work_dir | ||
self._build_plan = build_plan | ||
self.snaps: list[Snap] = [] | ||
self._install_snap = install_snap | ||
self._intercept_mknod = intercept_mknod | ||
|
||
self.environment: dict[str, str | None] = {self.managed_mode_env_var: "1"} | ||
self.packages: list[str] = [] | ||
# this is a private attribute because it may not reflect the actual | ||
|
@@ -309,7 +317,11 @@ def _get_provider_by_name(self, name: str) -> craft_providers.Provider: | |
def _get_lxd_provider(self) -> LXDProvider: | ||
"""Get the LXD provider for this manager.""" | ||
lxd_remote = self._services.config.get("lxd_remote") | ||
return LXDProvider(lxd_project=self._app.name, lxd_remote=lxd_remote) | ||
return LXDProvider( | ||
lxd_project=self._app.name, | ||
lxd_remote=lxd_remote, | ||
intercept_mknod=self._intercept_mknod, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nitpick?: I feel like this is an implementation detail, or at least something living at a deeper abstraction level than the other arguments given to the LXDProvider class. I do not have a good solution and I do not really know if we will ever need to gather other configuration values of this kind at some point in the future but maybe this should be in a "lxd_tuning" object? |
||
) | ||
|
||
def _get_multipass_provider(self) -> MultipassProvider: | ||
"""Get the Multipass provider for this manager.""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,7 +9,8 @@ dependencies = [ | |
"craft-grammar>=2.0.0", | ||
"craft-parts>=2.1.1", | ||
"craft-platforms>=0.5.0", | ||
"craft-providers>=2.1.0", | ||
"craft-providers @ git+https://github.com/canonical/craft-providers.git@work/CRAFT-2568-settable-mknod-intercept", | ||
#"craft-providers>=2.1.0", | ||
Comment on lines
+12
to
+13
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reminder to future self: update this once I release a new |
||
"Jinja2~=3.1", | ||
"snap-helpers>=0.4.2", | ||
"platformdirs>=3.10", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -237,7 +237,7 @@ def test_get_lxd_provider(monkeypatch, provider_service, lxd_remote, check): | |
check.equal(actual, mock_provider.return_value) | ||
with check: | ||
mock_provider.assert_called_once_with( | ||
lxd_project="testcraft", lxd_remote=lxd_remote | ||
lxd_project="testcraft", lxd_remote=lxd_remote, intercept_mknod=True | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you exercise the default value and setting A quick way to do this would be to instantiate a ProviderService like some of the other tests and add something along the lines of |
||
) | ||
|
||
|
||
|
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.
Here and in craft-providers this is set to
True
by default. But according to the discussion in canonical/craft-providers#191 I understand the proposition was to set it toFalse
by default. Is that a mistake or did we change our mind?