Skip to content
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

charmcraft: perform LXD checks before use (CRAFT-420) #474

Merged
merged 4 commits into from
Aug 3, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 24 additions & 27 deletions charmcraft/providers.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,10 @@

from craft_providers import Executor, bases, lxd
from craft_providers.actions import snap_installer
from craft_providers.lxd import LXDInstallationError
from craft_providers.lxd import installer as lxd_installer
from craft_providers.lxd.remotes import configure_buildd_image_remote

from charmcraft.cmdbase import CommandError
from charmcraft.config import Base
from charmcraft.env import (
get_managed_environment_log_path,
get_managed_environment_project_path,
)
from charmcraft.env import get_managed_environment_log_path, get_managed_environment_project_path
from charmcraft.utils import confirm_with_user, get_host_architecture

logger = logging.getLogger(__name__)
Expand Down Expand Up @@ -112,26 +106,29 @@ def ensure_provider_is_available() -> None:

:raises CommandError: if provider is not available.
"""
if is_provider_available():
return

if confirm_with_user(
"LXD is required, but not installed. Do you wish to install LXD "
"and configure it with the defaults?",
default=False,
):
try:
lxd_installer.install()
except LXDInstallationError as error:
if not is_provider_available():
if confirm_with_user(
"LXD is required, but not installed. Do you wish to install LXD "
"and configure it with the defaults?",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not worth for this branch, but for the future: for readability in these cases please write the long message outside the if. Thanks!

default=False,
):
try:
lxd.install()
except lxd.LXDInstallationError as error:
raise CommandError(
"Failed to install LXD. Please visit https://snapcraft.io/lxd for "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove Please, start with Visit -> https://discourse.ubuntu.com/t/tone-of-voice/18883

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I questioned it when I wrote it way back when and never did reconsider. 🤔 I think I just feel compelled to be polite 🤣 will fix (but note that this was just an indent change, not new text).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

"instructions on how to install the LXD snap for your distribution"
) from error
else:
raise CommandError(
"Failed to install LXD. Please visit https://snapcraft.io/lxd for "
"LXD is required, but not installed. Please visit https://snapcraft.io/lxd for "
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

"instructions on how to install the LXD snap for your distribution"
) from error
else:
raise CommandError(
"LXD is required, but not installed. Please visit https://snapcraft.io/lxd for "
"instructions on how to install the LXD snap for your distribution"
)
)

try:
lxd.ensure_lxd_is_ready()
except lxd.LXDError as error:
raise CommandError(str(error))


def is_base_providable(base: Base) -> Tuple[bool, Union[str, None]]:
Expand Down Expand Up @@ -216,7 +213,7 @@ def is_provider_available() -> bool:

:returns: True if installed.
"""
return lxd_installer.is_installed()
return lxd.is_installed()


@contextlib.contextmanager
Expand Down Expand Up @@ -250,7 +247,7 @@ def launched_environment(
)

environment = get_command_environment()
image_remote = configure_buildd_image_remote()
image_remote = lxd.configure_buildd_image_remote()
base_configuration = CharmcraftBuilddBaseConfiguration(
alias=alias, environment=environment, hostname=instance_name
)
Expand Down
8 changes: 4 additions & 4 deletions requirements-dev.txt
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,13 @@ attrs==21.2.0
black==21.7b0
certifi==2021.5.30
cffi==1.14.6
charset-normalizer==2.0.3
charset-normalizer==2.0.4
click==8.0.1
coverage==5.5
craft-parts @ https://github.com/canonical/craft-parts/archive/refs/tags/v1.0-alpha.3.tar.gz
craft-providers==1.0.1
craft-providers==1.0.2
flake8==3.9.2
humanize==3.10.0
humanize==3.11.0
idna==3.2
iniconfig==1.1.1
Jinja2==3.0.1
Expand Down Expand Up @@ -51,6 +51,6 @@ six==1.16.0
snowballstemmer==2.1.0
tabulate==0.8.9
toml==0.10.2
tomli==1.1.0
tomli==1.2.0
typing-extensions==3.10.0.0
urllib3==1.26.6
6 changes: 3 additions & 3 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@ appdirs==1.4.4
attrs==21.2.0
certifi==2021.5.30
cffi==1.14.6
charset-normalizer==2.0.3
charset-normalizer==2.0.4
craft-parts @ https://github.com/canonical/craft-parts/archive/refs/tags/v1.0-alpha.3.tar.gz
craft-providers==1.0.1
humanize==3.10.0
craft-providers==1.0.2
humanize==3.11.0
idna==3.2
Jinja2==3.0.1
jsonschema==3.2.0
Expand Down
75 changes: 50 additions & 25 deletions tests/test_providers.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import pytest
from craft_providers import Executor, bases
from craft_providers.actions import snap_installer
from craft_providers.lxd import LXDInstallationError
from craft_providers.lxd import LXDError, LXDInstallationError

from charmcraft import providers
from charmcraft.cmdbase import CommandError
Expand All @@ -40,7 +40,7 @@ def bypass_buildd_base_setup(monkeypatch):
@pytest.fixture()
def mock_configure_buildd_image_remote():
with mock.patch(
"charmcraft.providers.configure_buildd_image_remote",
"charmcraft.providers.lxd.configure_buildd_image_remote",
return_value="buildd-remote",
) as mock_remote:
yield mock_remote
Expand Down Expand Up @@ -73,22 +73,30 @@ def mock_lxc(monkeypatch):


@pytest.fixture
def mock_lxd(monkeypatch):
with mock.patch("charmcraft.providers.lxd", autospec=True) as mock_lxd:
yield mock_lxd
def mock_lxd_launch(monkeypatch):
with mock.patch("charmcraft.providers.lxd.launch", autospec=True) as mock_lxd_launch:
yield mock_lxd_launch


@pytest.fixture(autouse=True)
def mock_lxd_ensure_lxd_is_ready():
with mock.patch(
"charmcraft.providers.lxd.ensure_lxd_is_ready", return_value=None
) as mock_is_ready:
yield mock_is_ready


@pytest.fixture(autouse=True)
def mock_lxd_is_installed():
with mock.patch(
"charmcraft.providers.lxd_installer.is_installed", return_value=True
"charmcraft.providers.lxd.is_installed", return_value=True
) as mock_is_installed:
yield mock_is_installed


@pytest.fixture()
def mock_lxd_install():
with mock.patch("charmcraft.providers.lxd_installer.install") as mock_install:
with mock.patch("charmcraft.providers.lxd.install") as mock_install:
yield mock_install


Expand Down Expand Up @@ -354,6 +362,24 @@ def test_ensure_provider_is_available_errors_when_lxd_install_fails(
assert exc_info.value.__cause__ == mock_lxd_install.side_effect


def test_ensure_provider_is_available_errors_when_lxd_not_ready(
mock_confirm_with_user, mock_lxd_is_installed, mock_lxd_install, mock_lxd_ensure_lxd_is_ready
):
mock_confirm_with_user.return_value = True
mock_lxd_is_installed.return_value = True
mock_lxd_ensure_lxd_is_ready.side_effect = LXDError(
brief="some error", details="some details", resolution="some resolution"
)

with pytest.raises(
CommandError,
match=re.escape("some error\nsome details\nsome resolution"),
) as exc_info:
providers.ensure_provider_is_available()

assert exc_info.value.__cause__ == mock_lxd_install.side_effect


def test_get_command_environment_minimal(monkeypatch):
monkeypatch.setenv("IGNORE_ME", "or-im-failing")
monkeypatch.setenv("PATH", "not-using-host-path")
Expand Down Expand Up @@ -453,9 +479,10 @@ def test_is_base_providable(


@pytest.mark.parametrize("is_installed", [True, False])
def test_is_provider_available(is_installed):
with mock.patch("charmcraft.providers.lxd_installer.is_installed", return_value=is_installed):
assert providers.is_provider_available() == is_installed
def test_is_provider_available(is_installed, mock_lxd_is_installed):
mock_lxd_is_installed.return_value = is_installed

assert providers.is_provider_available() == is_installed


@pytest.mark.parametrize(
Expand All @@ -466,7 +493,7 @@ def test_launched_environment(
channel,
alias,
mock_configure_buildd_image_remote,
mock_lxd,
mock_lxd_launch,
monkeypatch,
tmp_path,
mock_path,
Expand All @@ -491,8 +518,8 @@ def test_launched_environment(
) as instance:
assert instance is not None
assert mock_configure_buildd_image_remote.mock_calls == [mock.call()]
assert mock_lxd.mock_calls == [
mock.call.launch(
assert mock_lxd_launch.mock_calls == [
mock.call(
name="charmcraft-test-charm-445566-1-2-host-arch",
base_configuration=mock_base_config.return_value,
image_name=channel,
Expand All @@ -504,9 +531,7 @@ def test_launched_environment(
project="charmcraft",
remote="local",
),
mock.call.launch().mount(
host_source=mock_path, target=pathlib.Path("/root/project")
),
mock.call().mount(host_source=mock_path, target=pathlib.Path("/root/project")),
]
assert mock_base_config.mock_calls == [
call(
Expand All @@ -516,16 +541,16 @@ def test_launched_environment(
)
]

mock_lxd.reset_mock()
mock_lxd_launch.reset_mock()

assert mock_lxd.mock_calls == [
mock.call.launch().unmount_all(),
mock.call.launch().stop(),
assert mock_lxd_launch.mock_calls == [
mock.call().unmount_all(),
mock.call().stop(),
]


def test_launched_environment_unmounts_and_stops_after_error(
mock_configure_buildd_image_remote, mock_lxd, monkeypatch, tmp_path
mock_configure_buildd_image_remote, mock_lxd_launch, monkeypatch, tmp_path
):
monkeypatch.setattr(providers, "get_host_architecture", lambda: "host-arch")
base = Base(name="ubuntu", channel="20.04", architectures=["host-arch"])
Expand All @@ -541,10 +566,10 @@ def test_launched_environment_unmounts_and_stops_after_error(
lxd_project="charmcraft",
lxd_remote="local",
):
mock_lxd.reset_mock()
mock_lxd_launch.reset_mock()
raise RuntimeError("this is a test")

assert mock_lxd.mock_calls == [
mock.call.launch().unmount_all(),
mock.call.launch().stop(),
assert mock_lxd_launch.mock_calls == [
mock.call().unmount_all(),
mock.call().stop(),
]