From c8cd3269c0a9265516fa13834f1ce0f77f732e3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20N=C3=BCst?= Date: Sat, 7 Sep 2019 09:22:00 +0200 Subject: [PATCH 1/2] add explicit log message on failing Docker connection, see #774 --- repo2docker/app.py | 5 ++++- tests/unit/test_app.py | 22 ++++++++++++++++++++++ tests/unit/test_subdir.py | 2 +- 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/repo2docker/app.py b/repo2docker/app.py index e3fc79ee..00a4ad4e 100644 --- a/repo2docker/app.py +++ b/repo2docker/app.py @@ -611,8 +611,11 @@ class Repo2Docker(Application): try: docker_client = docker.APIClient(version="auto", **kwargs_from_env()) except DockerException as e: - self.log.exception(e) + self.log.error( + "\nDocker client initialization error. Check if docker is running on the host.\n\n" + ) raise + # If the source to be executed is a directory, continue using the # directory. In the case of a local directory, it is used as both the # source and target. Reusing a local directory seems better than diff --git a/tests/unit/test_app.py b/tests/unit/test_app.py index 55845092..65b6438f 100644 --- a/tests/unit/test_app.py +++ b/tests/unit/test_app.py @@ -8,6 +8,7 @@ import escapism from repo2docker.app import Repo2Docker from repo2docker.__main__ import make_r2d +from repo2docker.utils import chdir def test_find_image(): @@ -124,3 +125,24 @@ def test_root_not_allowed(): builds.return_value = [] app.build() builds.assert_called_once() + + +def test_dryrun_works_without_docker(tmpdir): + with chdir(tmpdir): + with patch.object(docker, "APIClient") as client: + client.side_effect = docker.errors.DockerException("Error: no Docker") + app = Repo2Docker(dry_run=True) + app.build() + + +def test_error_log_without_docker(tmpdir, caplog): + with chdir(tmpdir): + with patch.object(docker, "APIClient") as client: + client.side_effect = docker.errors.DockerException("Error: no Docker") + app = Repo2Docker() + + with pytest.raises(docker.errors.DockerException): + app.build() + + captured = capsys.readouterr() + assert "Check if docker is running" in captured.err diff --git a/tests/unit/test_subdir.py b/tests/unit/test_subdir.py index 05a77af2..43d08fce 100644 --- a/tests/unit/test_subdir.py +++ b/tests/unit/test_subdir.py @@ -33,7 +33,7 @@ def test_subdir_in_image_name(): assert escaped_dirname in app.output_image_spec -def test_subdir_invalid(caplog): +def test_subdir_invalid(): # test an error is raised when requesting a non existent subdir app = Repo2Docker(repo=TEST_REPO, subdir="invalid-sub-dir") app.initialize() From b3f82db55841640b52b6f9077d32379df9ee3f0e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20N=C3=BCst?= Date: Sat, 7 Sep 2019 12:07:34 +0200 Subject: [PATCH 2/2] exit and show clean error message (no log) if docker connection fails --- repo2docker/app.py | 5 +++-- tests/unit/test_app.py | 11 ++++++----- tests/unit/test_argumentvalidation.py | 10 ++++++---- 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/repo2docker/app.py b/repo2docker/app.py index 00a4ad4e..e2e40141 100644 --- a/repo2docker/app.py +++ b/repo2docker/app.py @@ -612,9 +612,10 @@ class Repo2Docker(Application): docker_client = docker.APIClient(version="auto", **kwargs_from_env()) except DockerException as e: self.log.error( - "\nDocker client initialization error. Check if docker is running on the host.\n\n" + "\nDocker client initialization error: %s.\nCheck if docker is running on the host.\n", + e, ) - raise + self.exit(1) # If the source to be executed is a directory, continue using the # directory. In the case of a local directory, it is used as both the diff --git a/tests/unit/test_app.py b/tests/unit/test_app.py index 65b6438f..d45c28a4 100644 --- a/tests/unit/test_app.py +++ b/tests/unit/test_app.py @@ -127,22 +127,23 @@ def test_root_not_allowed(): builds.assert_called_once() -def test_dryrun_works_without_docker(tmpdir): +def test_dryrun_works_without_docker(tmpdir, capsys): with chdir(tmpdir): with patch.object(docker, "APIClient") as client: client.side_effect = docker.errors.DockerException("Error: no Docker") app = Repo2Docker(dry_run=True) app.build() + captured = capsys.readouterr() + assert "Error: no Docker" not in captured.err -def test_error_log_without_docker(tmpdir, caplog): +def test_error_log_without_docker(tmpdir, capsys): with chdir(tmpdir): with patch.object(docker, "APIClient") as client: client.side_effect = docker.errors.DockerException("Error: no Docker") app = Repo2Docker() - with pytest.raises(docker.errors.DockerException): + with pytest.raises(SystemExit): app.build() - captured = capsys.readouterr() - assert "Check if docker is running" in captured.err + assert "Error: no Docker" in captured.err diff --git a/tests/unit/test_argumentvalidation.py b/tests/unit/test_argumentvalidation.py index 1cdc203a..11647e37 100644 --- a/tests/unit/test_argumentvalidation.py +++ b/tests/unit/test_argumentvalidation.py @@ -223,7 +223,6 @@ def test_invalid_container_port_protocol_mapping_fail(temp_cwd): assert not validate_arguments(builddir, args_list, "Port specification") -@pytest.mark.xfail(reason="Regression in new arg parsing") def test_docker_handle_fail(temp_cwd): """ Test to check if r2d fails with minimal error message on not being able to connect to docker daemon @@ -233,19 +232,22 @@ def test_docker_handle_fail(temp_cwd): assert not validate_arguments( builddir, args_list, - "Docker client initialization error. Check if docker is running on the host.", + "Check if docker is running on the host.", disable_dockerd=True, ) def test_docker_handle_debug_fail(temp_cwd): """ - Test to check if r2d fails with stack trace on not being able to connect to docker daemon and debug enabled + Test to check if r2d fails with helpful error message on not being able to connect to docker daemon and debug enabled """ args_list = ["--debug"] assert not validate_arguments( - builddir, args_list, "docker.errors.DockerException", disable_dockerd=True + builddir, + args_list, + "Check if docker is running on the host.", + disable_dockerd=True, )