diff --git a/docsgpt/deploy/commands.py b/docsgpt/deploy/commands.py index 140b7929..4367221b 100644 --- a/docsgpt/deploy/commands.py +++ b/docsgpt/deploy/commands.py @@ -23,6 +23,9 @@ PROJECT = "docsgpt" DATABASE_VOLUME = f"{PROJECT}_postgres_data" _MOVING_TAGS = ("latest", "develop") _KEY = re.compile(r"^[A-Za-z_][A-Za-z0-9_]*$") +# Commands that stop or remove services name every profile, so Caddy goes too +# even when COMPOSE_PROFILES no longer enables it. +_EVERY_PROFILE = ("--profile", "https") EXPOSURE_CHOICES = [ ("local", "Only this computer"), @@ -208,6 +211,9 @@ def up(args, context: Optional[Context] = None) -> int: envfile.update(env_path, updates) env = envfile.read(env_path) + if stack.exposure(existing) == "domain" and stack.exposure(env) != "domain": + # With the https profile off, `up --remove-orphans` would leave Caddy running on ports 80 and 443. + context.docker.compose(directory, *_EVERY_PROFILE, "rm", "--stop", "--force", "caddy", check=False) up_args = ["up", "-d", "--remove-orphans"] if image_tag in _MOVING_TAGS: up_args += ["--pull", "always"] @@ -252,7 +258,7 @@ def down(args, context: Optional[Context] = None) -> int: directory = stack.stack_dir(args.dir) if _installed(directory) is None: return 1 - context.docker.compose(directory, "down") + context.docker.compose(directory, *_EVERY_PROFILE, "down") return 0 @@ -369,7 +375,7 @@ def uninstall(args, context: Optional[Context] = None) -> int: if not context.prompter.confirm(f"Remove the DocsGPT {what} in {directory}?", default=False): print("Nothing removed.") return 1 - context.docker.compose(directory, "down", "--remove-orphans", *(["-v"] if args.purge else [])) + context.docker.compose(directory, *_EVERY_PROFILE, "down", "--remove-orphans", *(["-v"] if args.purge else [])) if args.purge: shutil.rmtree(directory) print(f"Removed DocsGPT and its data from {directory}.") diff --git a/docsgpt/deploy/docker.py b/docsgpt/deploy/docker.py index 0b3a1ea4..90ddeea0 100644 --- a/docsgpt/deploy/docker.py +++ b/docsgpt/deploy/docker.py @@ -59,7 +59,13 @@ class Docker: """Make sure Docker is installed and running and Compose is new enough, starting Docker Desktop on macOS.""" if not self._which("docker"): raise DeployError("Docker is not installed. Get it from https://docs.docker.com/get-docker/ and run this again.") - if not self.daemon_running(): + info = self._run(["docker", "info"], capture=True, check=False) + if info.returncode != 0: + if "permission denied" in (info.stderr or "").lower(): + raise DeployError( + "Your user cannot use Docker (permission denied on its socket). Add it to the docker group " + "with `sudo usermod -aG docker $USER`, log out and back in, and run this again." + ) self._start_daemon() result = self._run(["docker", "compose", "version", "--short"], capture=True, check=False) version = _parse_version(result.stdout) if result.returncode == 0 else None diff --git a/tests/deploy/test_commands.py b/tests/deploy/test_commands.py index 3aa80d5c..4cc963b6 100644 --- a/tests/deploy/test_commands.py +++ b/tests/deploy/test_commands.py @@ -11,6 +11,8 @@ from docsgpt import cli from docsgpt.deploy import commands, envfile, stack from docsgpt.deploy.docker import DeployError +EVERY_PROFILE = ["--profile", "https"] + class FakeDocker: def __init__(self, volumes=(), project_dirs=()): @@ -24,7 +26,7 @@ class FakeDocker: def compose(self, directory, *args, capture=False, check=True): self.calls.append((Path(directory), list(args))) - if args and args[0] == "down" and "-v" in args: + if "down" in args and "-v" in args: self.volumes.clear() return subprocess.CompletedProcess(["docker", "compose", *args], 0, stdout="", stderr="") @@ -88,7 +90,7 @@ class TestUpFirstInstall: assert env["LLM_PROVIDER"] == "docsgpt" assert env["INTERNAL_KEY"] and env["JWT_SECRET_KEY"] and env["POSTGRES_PASSWORD"] assert docker.preflights == 1 - assert (tmp_path, ["up", "-d", "--remove-orphans"]) in docker.calls + assert docker.calls == [(tmp_path, ["up", "-d", "--remove-orphans"])] record = json.loads((tmp_path / "install.json").read_text()) assert record["version"] == "0.21.0" assert "http://localhost:7091" in capsys.readouterr().out @@ -113,7 +115,8 @@ class TestUpFirstInstall: assert env["DOCSGPT_DOMAIN"] == "docs.example.com" assert env["LLM_PROVIDER"] == "openai" - def test_a_missing_api_key_is_an_error_without_a_terminal(self, tmp_path): + def test_a_missing_api_key_is_an_error_without_a_terminal(self, tmp_path, monkeypatch): + monkeypatch.delenv("DOCSGPT_API_KEY", raising=False) with pytest.raises(DeployError, match="API key"): _run(["up", "--yes", "--dir", str(tmp_path), "--provider", "openai"], _context()) @@ -159,6 +162,22 @@ class TestUpAgain: assert after[key] == before[key] assert context.prompter.questions == [], "a configured install is not asked again" + def test_leaving_the_domain_removes_caddy_before_starting(self, tmp_path): + """With the https profile off, `up --remove-orphans` alone would leave Caddy on ports 80 and 443.""" + assert _run(["up", "--yes", "--dir", str(tmp_path), "--domain", "docs.example.com"], _context()) == 0 + docker = FakeDocker(volumes={"docsgpt_postgres_data"}) + assert _run(["up", "--yes", "--dir", str(tmp_path), "--expose", "local"], _context(docker)) == 0 + assert docker.calls == [ + (tmp_path, [*EVERY_PROFILE, "rm", "--stop", "--force", "caddy"]), + (tmp_path, ["up", "-d", "--remove-orphans"]), + ] + + def test_staying_on_the_domain_leaves_caddy_alone(self, tmp_path): + assert _run(["up", "--yes", "--dir", str(tmp_path), "--domain", "docs.example.com"], _context()) == 0 + docker = FakeDocker(volumes={"docsgpt_postgres_data"}) + assert _run(["up", "--yes", "--dir", str(tmp_path)], _context(docker)) == 0 + assert docker.calls == [(tmp_path, ["up", "-d", "--remove-orphans"])] + def test_another_projects_containers_need_consent(self, tmp_path): docker = FakeDocker(project_dirs={"/srv/old-docsgpt"}) with pytest.raises(DeployError, match="--adopt"): @@ -177,11 +196,11 @@ class TestManage: def _installed(tmp_path, *extra): assert _run(["up", "--yes", "--dir", str(tmp_path), *extra], _context()) == 0 - def test_down(self, tmp_path): + def test_down_includes_caddy(self, tmp_path): self._installed(tmp_path) docker = FakeDocker() assert _run(["down", "--dir", str(tmp_path)], _context(docker)) == 0 - assert docker.calls == [(tmp_path, ["down"])] + assert docker.calls == [(tmp_path, [*EVERY_PROFILE, "down"])] def test_commands_on_a_missing_install(self, tmp_path, capsys): assert _run(["status", "--dir", str(tmp_path)], _context()) == 1 @@ -228,7 +247,7 @@ class TestManage: self._installed(tmp_path) docker = FakeDocker() assert _run(["uninstall", "--yes", "--dir", str(tmp_path)], _context(docker, installer=lambda: "uv")) == 0 - assert docker.calls == [(tmp_path, ["down", "--remove-orphans"])] + assert docker.calls == [(tmp_path, [*EVERY_PROFILE, "down", "--remove-orphans"])] assert (tmp_path / ".env").is_file(), "the database password lives there" assert not (tmp_path / "docker-compose.yaml").exists() assert not (tmp_path / "install.json").exists() @@ -239,7 +258,7 @@ class TestManage: self._installed(directory) docker = FakeDocker() assert _run(["uninstall", "--yes", "--purge", "--dir", str(directory)], _context(docker)) == 0 - assert docker.calls == [(directory, ["down", "--remove-orphans", "-v"])] + assert docker.calls == [(directory, [*EVERY_PROFILE, "down", "--remove-orphans", "-v"])] assert not directory.exists() def test_uninstall_asks_first(self, tmp_path): diff --git a/tests/deploy/test_docker.py b/tests/deploy/test_docker.py index 3354438c..d3de76b6 100644 --- a/tests/deploy/test_docker.py +++ b/tests/deploy/test_docker.py @@ -10,7 +10,7 @@ from docsgpt.deploy.docker import DeployError, Docker class FakeRunner: - """Answers docker commands from a table of (argv prefix -> result) and records every call.""" + """Answers docker commands from a table of argv prefix -> (code, stdout[, stderr]) and records every call.""" def __init__(self, answers=None): self.answers = list((answers or {}).items()) @@ -22,10 +22,10 @@ class FakeRunner: if list(args[: len(prefix)]) == list(prefix): if callable(answer): answer = answer() - code, out = answer + code, out, err = (*answer, "")[:3] if check and code != 0: raise DeployError(f"{' '.join(args)} failed") - return subprocess.CompletedProcess(args, code, stdout=out, stderr="") + return subprocess.CompletedProcess(args, code, stdout=out, stderr=err) return subprocess.CompletedProcess(args, 0, stdout="", stderr="") @@ -43,6 +43,13 @@ class TestPreflight: with pytest.raises(DeployError, match="systemctl start docker"): _docker(runner).preflight() + def test_no_permission_on_the_socket_says_how_to_get_it(self): + """A user outside the docker group is not told that Docker is down.""" + denied = "permission denied while trying to connect to the Docker daemon socket at unix:///var/run/docker.sock" + runner = FakeRunner({("docker", "info"): (1, "", denied)}) + with pytest.raises(DeployError, match="docker group"): + _docker(runner).preflight() + def test_daemon_down_on_macos_starts_docker_desktop(self): state = {"started": False} @@ -99,7 +106,7 @@ class TestQueries: class TestWaitHealthy: - def test_succeeds_once_the_api_answers(self, monkeypatch): + def test_succeeds_once_the_api_answers(self): attempts = {"n": 0} def opener(url, timeout):