From f3ff1bda04bffe3571f771d743e81e89a19cab3e Mon Sep 17 00:00:00 2001 From: Alex Lowe Date: Thu, 13 Feb 2025 23:54:11 -0500 Subject: [PATCH] feat(ProviderService): deprecate work_dir on instance This is not needed since the ProviderService has a work_dir instance attribute. Does not remove it though, as that will need to wait for a major release. --- craft_application/application.py | 1 - craft_application/services/provider.py | 14 ++++++++++++-- docs/reference/changelog.rst | 12 +++++++----- tests/unit/services/test_provider.py | 24 +++++++++++++++++++----- 4 files changed, 38 insertions(+), 13 deletions(-) diff --git a/craft_application/application.py b/craft_application/application.py index be8b3c7a1..f86ede68e 100644 --- a/craft_application/application.py +++ b/craft_application/application.py @@ -394,7 +394,6 @@ def run_managed(self, platform: str | None, build_for: str | None) -> None: with self.services.provider.instance( build_info, - work_dir=self._work_dir, clean_existing=self._enable_fetch_service, ) as instance: if self._enable_fetch_service: diff --git a/craft_application/services/provider.py b/craft_application/services/provider.py index 95dcbe280..bf06668c8 100644 --- a/craft_application/services/provider.py +++ b/craft_application/services/provider.py @@ -27,6 +27,8 @@ import sys import urllib.request from collections.abc import Generator, Iterable, Sequence +import warnings +from collections.abc import Generator, Iterable from pathlib import Path from typing import TYPE_CHECKING @@ -136,7 +138,7 @@ def instance( self, build_info: craft_platforms.BuildInfo, *, - work_dir: pathlib.Path, + work_dir: pathlib.Path | None = None, allow_unstable: bool = True, clean_existing: bool = False, project_name: str | None = None, @@ -145,12 +147,20 @@ def instance( """Context manager for getting a provider instance. :param build_info: Build information for the instance. - :param work_dir: Local path to mount inside the provider instance. + :param work_dir: (DEPRECATED) Local path to mount inside the provider instance. :param allow_unstable: Whether to allow the use of unstable images. :param clean_existing: Whether pre-existing instances should be wiped and re-created. :returns: a context manager of the provider instance. """ + if work_dir is not None: + warnings.warn( + "work_dir is deprecated. Use the service's work dir instead.", + DeprecationWarning, + stacklevel=3, + ) + else: + work_dir = self._work_dir if not project_name: project_name = self._services.get("project").get().name instance_name = self._get_instance_name(work_dir, build_info, project_name) diff --git a/docs/reference/changelog.rst b/docs/reference/changelog.rst index 81c21c8d7..45686e6b0 100644 --- a/docs/reference/changelog.rst +++ b/docs/reference/changelog.rst @@ -7,6 +7,13 @@ Changelog 5.4.0 (2025-MM-DD) ------------------ +Services +======== + +- Deprecate the ``work_dir`` parameter on :meth:`ProviderService.instance`. The + parameter is still used if provided, but is now optional. It will be removed in + a future release. + Models ====== @@ -63,15 +70,10 @@ Commands - The ``test`` command now accepts paths to specific tests as well as the ``--debug``, ``--shell`` and ``--shell-after`` parameters. -======= -4.x.x (YYYY-MMM-DD) -------------------- ->>>>>>> 7e225ac (feat(models): expose Part type) Models ====== -<<<<<<< HEAD - A new :doc:`how-to guide ` describes how to implement application-specific ``platforms`` keys. diff --git a/tests/unit/services/test_provider.py b/tests/unit/services/test_provider.py index 77ee43b9f..09c7d61c7 100644 --- a/tests/unit/services/test_provider.py +++ b/tests/unit/services/test_provider.py @@ -620,7 +620,7 @@ def test_instance( mock_capture_pack_state, ): with provider_service.instance( - fake_build_info, work_dir=tmp_path, allow_unstable=allow_unstable + fake_build_info, allow_unstable=allow_unstable ) as instance: pass @@ -645,6 +645,22 @@ def test_instance( emitter.assert_progress("Launching managed .+ instance...", regex=True) +def test_instance_work_dir_deprecated(provider_service, mock_provider): + arch = util.get_host_architecture() + build_info = models.BuildInfo("foo", arch, arch, bases.BaseName("ubuntu", "24.04")) + with pytest.warns(DeprecationWarning, match="work_dir is deprecated"): + with provider_service.instance(build_info, work_dir=pathlib.Path("/")): + pass + + mock_provider.launched_environment.assert_called_once_with( + project_name=mock.ANY, + project_path=pathlib.Path("/"), + instance_name=mock.ANY, + base_configuration=mock.ANY, + allow_unstable=True, + ) + + @pytest.mark.parametrize("clean_existing", [True, False]) def test_instance_clean_existing( tmp_path, @@ -658,7 +674,7 @@ def test_instance_clean_existing( build_info = craft_platforms.BuildInfo("foo", arch, arch, base_name) with provider_service.instance( - build_info, work_dir=tmp_path, clean_existing=clean_existing + build_info, clean_existing=clean_existing ) as _instance: pass @@ -705,7 +721,7 @@ def test_load_bashrc_missing( mocker.patch.object(pkgutil, "get_data", return_value=None) with provider_service.instance( - fake_build_info, work_dir=tmp_path, allow_unstable=allow_unstable + fake_build_info, allow_unstable=allow_unstable ) as instance: instance._setup_instance_bashrc(instance) emitter.assert_debug( @@ -771,7 +787,6 @@ def test_instance_fetch_logs( with ( provider_service.instance( build_info=fake_build_info, - work_dir=pathlib.Path(), ) as mock_instance, ): pass @@ -832,7 +847,6 @@ def test_instance_fetch_logs_missing_file( provider_service = setup_fetch_logs_provider(should_have_logfile=False) with provider_service.instance( build_info=fake_build_info, - work_dir=pathlib.Path(), ) as mock_instance: pass