From 5a82809630a665219cba15a85d57d681b05e7087 Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Mon, 12 May 2025 16:58:16 -0700 Subject: [PATCH 01/20] add validation for non remote dependencies --- devservices/configs/service_config.py | 12 ++++ devservices/utils/services.py | 8 +-- tests/configs/test_service_config.py | 100 +++++++++++++++++++++++--- 3 files changed, 103 insertions(+), 17 deletions(-) diff --git a/devservices/configs/service_config.py b/devservices/configs/service_config.py index 6b1fcf34..62e459b6 100644 --- a/devservices/configs/service_config.py +++ b/devservices/configs/service_config.py @@ -84,6 +84,8 @@ def load_service_config_from_file(repo_path: str) -> ServiceConfig: ) service_config_data = config.get("x-sentry-service-config") + docker_compose_services = config.get("services", {}).keys() + valid_dependency_keys = {field.name for field in fields(Dependency)} dependencies = {} @@ -106,6 +108,16 @@ def load_service_config_from_file(repo_path: str) -> ServiceConfig: f"Error parsing service dependencies: {type_error}" ) from type_error + # Validate that all non-remote dependencies are defined in docker-compose services + for dependency_name, dependency in dependencies.items(): + if ( + dependency.remote is None + and dependency_name not in docker_compose_services + ): + raise ConfigValidationError( + f"Dependency '{dependency_name}' is not remote but is not defined in docker-compose services" + ) + service_config = ServiceConfig( version=service_config_data.get("version"), service_name=service_config_data.get("service_name"), diff --git a/devservices/utils/services.py b/devservices/utils/services.py index f8bb72a3..005bbd2b 100644 --- a/devservices/utils/services.py +++ b/devservices/utils/services.py @@ -8,7 +8,6 @@ from devservices.exceptions import ConfigParseError from devservices.exceptions import ConfigValidationError from devservices.exceptions import ServiceNotFoundError -from devservices.utils.console import Console from devservices.utils.devenv import get_coderoot @@ -23,16 +22,13 @@ def get_local_services(coderoot: str) -> list[Service]: """Get a list of services in the coderoot.""" from devservices.configs.service_config import load_service_config_from_file - console = Console() - services = [] for repo in os.listdir(coderoot): repo_path = os.path.join(coderoot, repo) try: service_config = load_service_config_from_file(repo_path) - except (ConfigParseError, ConfigValidationError) as e: - console.warning(f"{repo} was found with an invalid config: {e}") - continue + except (ConfigParseError, ConfigValidationError): + raise except ConfigNotFoundError: # Ignore repos that don't have devservices configs continue diff --git a/tests/configs/test_service_config.py b/tests/configs/test_service_config.py index bcc02933..e6797630 100644 --- a/tests/configs/test_service_config.py +++ b/tests/configs/test_service_config.py @@ -70,7 +70,13 @@ def test_load_service_config_from_file( "service_name": service_name, "dependencies": {key: value for key, value in dependencies.items()}, "modes": {key: value for key, value in modes.items()}, - } + }, + "services": { + key: { + "image": key, + } + for key in dependencies.keys() + }, } create_config_file(tmp_path, config) @@ -95,7 +101,8 @@ def test_load_service_config_from_file_no_dependencies(tmp_path: Path) -> None: "version": 0.1, "service_name": "example-service", "modes": {"default": []}, - } + }, + "services": {}, } create_config_file(tmp_path, config) @@ -126,7 +133,12 @@ def test_load_service_config_from_file_invalid_version(tmp_path: Path) -> None: "example-dependency": {"description": "Example dependency"} }, "modes": {"default": ["example-dependency"]}, - } + }, + "services": { + "example-dependency": { + "image": "example-dependency", + } + }, } create_config_file(tmp_path, config) @@ -142,7 +154,12 @@ def test_load_service_config_from_file_missing_version(tmp_path: Path) -> None: "example-dependency": {"description": "Example dependency"} }, "modes": {"default": ["example-dependency"]}, - } + }, + "services": { + "example-dependency": { + "image": "example-dependency", + } + }, } create_config_file(tmp_path, config) @@ -159,7 +176,12 @@ def test_load_service_config_from_file_missing_service_name(tmp_path: Path) -> N "example-dependency": {"description": "Example dependency"} }, "modes": {"default": ["example-dependency"]}, - } + }, + "services": { + "example-dependency": { + "image": "example-dependency", + } + }, } create_config_file(tmp_path, config) @@ -177,7 +199,12 @@ def test_load_service_config_from_file_invalid_dependency(tmp_path: Path) -> Non "example-dependency": {"description": "Example dependency"} }, "modes": {"default": ["example-dependency", "unknown-dependency"]}, - } + }, + "services": { + "example-dependency": { + "image": "example-dependency", + } + }, } create_config_file(tmp_path, config) @@ -198,7 +225,12 @@ def test_load_service_config_from_file_missing_default_mode(tmp_path: Path) -> N "example-dependency": {"description": "Example dependency"} }, "modes": {"custom": ["example-dependency"]}, - } + }, + "services": { + "example-dependency": { + "image": "example-dependency", + } + }, } create_config_file(tmp_path, config) @@ -215,7 +247,12 @@ def test_load_service_config_from_file_no_modes(tmp_path: Path) -> None: "dependencies": { "example-dependency": {"description": "Example dependency"} }, - } + }, + "services": { + "example-dependency": { + "image": "example-dependency", + } + }, } create_config_file(tmp_path, config) @@ -224,6 +261,32 @@ def test_load_service_config_from_file_no_modes(tmp_path: Path) -> None: assert str(e.value) == "Default mode is required in service config" +def test_load_service_config_from_file_remote_dependency_not_in_services( + tmp_path: Path, +) -> None: + config = { + "x-sentry-service-config": { + "version": 0.1, + "service_name": "example-service", + "dependencies": { + "example-dependency": { + "description": "Example dependency", + "remote": { + "repo_name": "example-dependency", + "repo_link": "https://github.com/example/example-dependency", + "branch": "main", + }, + }, + }, + "modes": {"default": ["example-dependency"]}, + }, + "services": {}, + } + create_config_file(tmp_path, config) + + load_service_config_from_file(str(tmp_path)) + + def test_load_service_config_from_file_invalid_dependencies(tmp_path: Path) -> None: config = { "x-sentry-service-config": { @@ -236,7 +299,12 @@ def test_load_service_config_from_file_invalid_dependencies(tmp_path: Path) -> N } }, "modes": {"default": ["example-dependency"]}, - } + }, + "services": { + "example-dependency": { + "image": "example-dependency", + } + }, } create_config_file(tmp_path, config) @@ -260,7 +328,12 @@ def test_load_service_config_from_file_invalid_modes(tmp_path: Path) -> None: "default": ["example-dependency"], "custom": "example-dependency", }, - } + }, + "services": { + "example-dependency": { + "image": "example-dependency", + } + }, } create_config_file(tmp_path, config) @@ -280,7 +353,12 @@ def test_load_service_config_from_file_no_x_sentry_service_config( "example-dependency": {"description": "Example dependency"} }, "modes": {"default": ["example-dependency"]}, - } + }, + "services": { + "example-dependency": { + "image": "example-dependency", + } + }, } create_config_file(tmp_path, config) From 6ecedd55a3b9c410a5ca527ac84b9fdfe3bcf7a9 Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Mon, 12 May 2025 17:09:26 -0700 Subject: [PATCH 02/20] fix some tests --- devservices/utils/services.py | 4 ++++ tests/utils/test_services.py | 11 +++++------ 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/devservices/utils/services.py b/devservices/utils/services.py index 005bbd2b..6c96d9a7 100644 --- a/devservices/utils/services.py +++ b/devservices/utils/services.py @@ -8,6 +8,7 @@ from devservices.exceptions import ConfigParseError from devservices.exceptions import ConfigValidationError from devservices.exceptions import ServiceNotFoundError +from devservices.utils.console import Console from devservices.utils.devenv import get_coderoot @@ -22,12 +23,15 @@ def get_local_services(coderoot: str) -> list[Service]: """Get a list of services in the coderoot.""" from devservices.configs.service_config import load_service_config_from_file + console = Console() + services = [] for repo in os.listdir(coderoot): repo_path = os.path.join(coderoot, repo) try: service_config = load_service_config_from_file(repo_path) except (ConfigParseError, ConfigValidationError): + console.warning(f"{repo} was found with an invalid config") raise except ConfigNotFoundError: # Ignore repos that don't have devservices configs diff --git a/tests/utils/test_services.py b/tests/utils/test_services.py index e17f6a31..1f5a136f 100644 --- a/tests/utils/test_services.py +++ b/tests/utils/test_services.py @@ -7,6 +7,7 @@ import pytest from devservices.configs.service_config import ServiceConfig +from devservices.exceptions import ConfigParseError from devservices.exceptions import ServiceNotFoundError from devservices.utils.services import find_matching_service from devservices.utils.services import get_local_services @@ -32,13 +33,11 @@ def test_get_local_services_with_invalid_config( mock_repo_path = mock_code_root / "example" create_mock_git_repo("invalid_repo", mock_repo_path) - local_services = get_local_services(str(mock_code_root)) + with pytest.raises(ConfigParseError): + local_services = get_local_services(str(mock_code_root)) + assert not local_services captured = capsys.readouterr() - assert not local_services - assert ( - "example was found with an invalid config: Error parsing config file:" - in captured.out - ) + assert "example was found with an invalid config" in captured.out def test_get_local_services_with_valid_config(tmp_path: Path) -> None: From eccadb2505e3cea9351a115723fd773815802cd3 Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Tue, 13 May 2025 12:09:20 -0700 Subject: [PATCH 03/20] add supervisor config validation --- devservices/configs/service_config.py | 23 +++++++++- tests/configs/test_service_config.py | 64 +++++++++++++++++++++++++++ 2 files changed, 86 insertions(+), 1 deletion(-) diff --git a/devservices/configs/service_config.py b/devservices/configs/service_config.py index 62e459b6..1cdfde54 100644 --- a/devservices/configs/service_config.py +++ b/devservices/configs/service_config.py @@ -5,12 +5,15 @@ from dataclasses import fields import yaml +from supervisor.options import ServerOptions from devservices.constants import CONFIG_FILE_NAME from devservices.constants import DEVSERVICES_DIR_NAME +from devservices.constants import PROGRAMS_CONF_FILE_NAME from devservices.exceptions import ConfigNotFoundError from devservices.exceptions import ConfigParseError from devservices.exceptions import ConfigValidationError +from devservices.utils.supervisor import SupervisorManager VALID_VERSIONS = [0.1] @@ -86,6 +89,10 @@ def load_service_config_from_file(repo_path: str) -> ServiceConfig: docker_compose_services = config.get("services", {}).keys() + supervisor_programs = load_supervisor_programs_from_file( + repo_path, service_config_data.get("service_name") + ) + valid_dependency_keys = {field.name for field in fields(Dependency)} dependencies = {} @@ -113,9 +120,10 @@ def load_service_config_from_file(repo_path: str) -> ServiceConfig: if ( dependency.remote is None and dependency_name not in docker_compose_services + and dependency_name not in supervisor_programs ): raise ConfigValidationError( - f"Dependency '{dependency_name}' is not remote but is not defined in docker-compose services" + f"Dependency '{dependency_name}' is not remote but is not defined in docker-compose services or programs file" ) service_config = ServiceConfig( @@ -126,3 +134,16 @@ def load_service_config_from_file(repo_path: str) -> ServiceConfig: ) return service_config + + +def load_supervisor_programs_from_file(repo_path: str, service_name: str) -> set[str]: + programs_config_path = os.path.join( + repo_path, DEVSERVICES_DIR_NAME, PROGRAMS_CONF_FILE_NAME + ) + if not os.path.exists(programs_config_path): + return set() + manager = SupervisorManager(programs_config_path, service_name=service_name) + opts = ServerOptions() + opts.configfile = manager.config_file_path + opts.process_config() + return set([program.name for program in opts.process_group_configs]) diff --git a/tests/configs/test_service_config.py b/tests/configs/test_service_config.py index e6797630..e63ff9f6 100644 --- a/tests/configs/test_service_config.py +++ b/tests/configs/test_service_config.py @@ -10,6 +10,7 @@ from devservices.exceptions import ConfigParseError from devservices.exceptions import ConfigValidationError from testing.utils import create_config_file +from testing.utils import create_programs_conf_file @pytest.mark.parametrize( @@ -412,3 +413,66 @@ def test_load_service_config_from_file_invalid_yaml_tag(tmp_path: Path) -> None: str(e.value) == f"Error parsing config file: could not determine a constructor for the tag 'tag:yaml.org,2002:invalid_tag'\n in \"{tmp_path / 'devservices' / 'config.yml'}\", line 7, column 19" ) + + +def test_load_service_config_from_file_no_programs_file(tmp_path: Path) -> None: + config = { + "x-sentry-service-config": { + "version": 0.1, + "service_name": "example-service", + "dependencies": { + "example-dependency": { + "description": "Example dependency", + }, + "example-program": { + "description": "Example program", + }, + }, + "modes": {"default": ["example-dependency", "example-program"]}, + }, + "services": { + "example-dependency": { + "image": "example-dependency", + } + }, + } + create_config_file(tmp_path, config) + + with pytest.raises(ConfigValidationError) as e: + load_service_config_from_file(str(tmp_path)) + assert ( + str(e.value) + == "Dependency 'example-program' is not remote but is not defined in docker-compose services or programs file" + ) + + +def test_load_service_config_from_file_valid_programs_file(tmp_path: Path) -> None: + service_config = { + "x-sentry-service-config": { + "version": 0.1, + "service_name": "example-service", + "dependencies": { + "example-dependency": { + "description": "Example dependency", + }, + "example-program": { + "description": "Example program", + }, + }, + "modes": {"default": ["example-dependency", "example-program"]}, + }, + "services": { + "example-dependency": { + "image": "example-dependency", + } + }, + } + create_config_file(tmp_path, service_config) + + programs_config = """[program:example-program] +command=echo "Hello, World!" +autostart=true +""" + create_programs_conf_file(tmp_path, programs_config) + + load_service_config_from_file(str(tmp_path)) From b1e2dc70e7f8527622be8a8b4ec4406029e98f07 Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Tue, 13 May 2025 12:13:06 -0700 Subject: [PATCH 04/20] add two more tests --- tests/configs/test_service_config.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/configs/test_service_config.py b/tests/configs/test_service_config.py index e63ff9f6..8d473de7 100644 --- a/tests/configs/test_service_config.py +++ b/tests/configs/test_service_config.py @@ -6,6 +6,7 @@ import pytest from devservices.configs.service_config import load_service_config_from_file +from devservices.configs.service_config import load_supervisor_programs_from_file from devservices.exceptions import ConfigNotFoundError from devservices.exceptions import ConfigParseError from devservices.exceptions import ConfigValidationError @@ -476,3 +477,19 @@ def test_load_service_config_from_file_valid_programs_file(tmp_path: Path) -> No create_programs_conf_file(tmp_path, programs_config) load_service_config_from_file(str(tmp_path)) + + +def test_load_supervisor_programs_from_file_no_programs_file(tmp_path: Path) -> None: + programs = load_supervisor_programs_from_file(str(tmp_path), "example-service") + assert programs == set() + + +def test_load_supervisor_programs_from_file_valid_programs_file(tmp_path: Path) -> None: + programs_config = """[program:example-program] +command=echo "Hello, World!" +autostart=true +""" + create_programs_conf_file(tmp_path, programs_config) + + programs = load_supervisor_programs_from_file(str(tmp_path), "example-service") + assert programs == {"example-program"} From d9c5fc824ab64fb050c3ae3208ec06db6ff8f771 Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Tue, 13 May 2025 14:06:01 -0700 Subject: [PATCH 05/20] change structure of dependeny to also pass in type --- devservices/configs/service_config.py | 28 ++++++++++++++++++------- tests/configs/test_service_config.py | 30 +++++++++++++++++++++++---- 2 files changed, 46 insertions(+), 12 deletions(-) diff --git a/devservices/configs/service_config.py b/devservices/configs/service_config.py index 1cdfde54..b45b73a6 100644 --- a/devservices/configs/service_config.py +++ b/devservices/configs/service_config.py @@ -3,6 +3,7 @@ import os from dataclasses import dataclass from dataclasses import fields +from enum import Enum import yaml from supervisor.options import ServerOptions @@ -18,6 +19,11 @@ VALID_VERSIONS = [0.1] +class DependencyType(Enum): + DOCKER_COMPOSE = "docker-compose" + SUPERVISOR = "supervisor" + + @dataclass class RemoteConfig: repo_name: str @@ -30,6 +36,7 @@ class RemoteConfig: class Dependency: description: str remote: RemoteConfig | None = None + dependency_type: str | None = None @dataclass @@ -117,14 +124,19 @@ def load_service_config_from_file(repo_path: str) -> ServiceConfig: # Validate that all non-remote dependencies are defined in docker-compose services for dependency_name, dependency in dependencies.items(): - if ( - dependency.remote is None - and dependency_name not in docker_compose_services - and dependency_name not in supervisor_programs - ): - raise ConfigValidationError( - f"Dependency '{dependency_name}' is not remote but is not defined in docker-compose services or programs file" - ) + if dependency.remote is None: + if dependency_name in supervisor_programs: + dependencies[ + dependency_name + ].dependency_type = DependencyType.SUPERVISOR.value + elif dependency_name in docker_compose_services: + dependencies[ + dependency_name + ].dependency_type = DependencyType.DOCKER_COMPOSE.value + else: + raise ConfigValidationError( + f"Dependency '{dependency_name}' is not remote but is not defined in docker-compose services or programs file" + ) service_config = ServiceConfig( version=service_config_data.get("version"), diff --git a/tests/configs/test_service_config.py b/tests/configs/test_service_config.py index 8d473de7..1b3f326e 100644 --- a/tests/configs/test_service_config.py +++ b/tests/configs/test_service_config.py @@ -5,6 +5,7 @@ import pytest +from devservices.configs.service_config import DependencyType from devservices.configs.service_config import load_service_config_from_file from devservices.configs.service_config import load_supervisor_programs_from_file from devservices.exceptions import ConfigNotFoundError @@ -15,12 +16,15 @@ @pytest.mark.parametrize( - "service_name, dependencies, modes", + "service_name, dependencies, modes, dependency_types", [ ( "example-service", {"example-dependency": {"description": "Example dependency"}}, {"default": ["example-dependency"]}, + { + "example-dependency": DependencyType.DOCKER_COMPOSE.value, + }, ), ( "example-service", @@ -39,6 +43,10 @@ }, }, {"default": ["example-dependency-1", "example-dependency-2"]}, + { + "example-dependency-1": None, + "example-dependency-2": DependencyType.DOCKER_COMPOSE.value, + }, ), ( "example-service", @@ -57,6 +65,10 @@ }, }, {"default": ["example-dependency-1"], "custom": ["example-dependency-2"]}, + { + "example-dependency-1": None, + "example-dependency-2": DependencyType.DOCKER_COMPOSE.value, + }, ), ], ) @@ -65,6 +77,7 @@ def test_load_service_config_from_file( service_name: str, dependencies: dict[str, dict[str, object]], modes: dict[str, list[str]], + dependency_types: dict[str, DependencyType], ) -> None: config = { "x-sentry-service-config": { @@ -90,6 +103,7 @@ def test_load_service_config_from_file( key: { "description": value["description"], "remote": value.get("remote"), + "dependency_type": dependency_types[key], } for key, value in dependencies.items() }, @@ -448,7 +462,7 @@ def test_load_service_config_from_file_no_programs_file(tmp_path: Path) -> None: def test_load_service_config_from_file_valid_programs_file(tmp_path: Path) -> None: - service_config = { + devservices_config = { "x-sentry-service-config": { "version": 0.1, "service_name": "example-service", @@ -468,7 +482,7 @@ def test_load_service_config_from_file_valid_programs_file(tmp_path: Path) -> No } }, } - create_config_file(tmp_path, service_config) + create_config_file(tmp_path, devservices_config) programs_config = """[program:example-program] command=echo "Hello, World!" @@ -476,7 +490,15 @@ def test_load_service_config_from_file_valid_programs_file(tmp_path: Path) -> No """ create_programs_conf_file(tmp_path, programs_config) - load_service_config_from_file(str(tmp_path)) + service_config = load_service_config_from_file(str(tmp_path)) + assert ( + service_config.dependencies["example-program"].dependency_type + == DependencyType.SUPERVISOR.value + ) + assert ( + service_config.dependencies["example-dependency"].dependency_type + == DependencyType.DOCKER_COMPOSE.value + ) def test_load_supervisor_programs_from_file_no_programs_file(tmp_path: Path) -> None: From 7e4beff67936ee582a2afc569115fa49d9b27644 Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Tue, 13 May 2025 14:30:10 -0700 Subject: [PATCH 06/20] fix more stuff --- devservices/configs/service_config.py | 46 ++++++++++++--------------- devservices/constants.py | 7 ++++ devservices/utils/dependencies.py | 16 ++++------ tests/configs/test_service_config.py | 16 +++++----- 4 files changed, 42 insertions(+), 43 deletions(-) diff --git a/devservices/configs/service_config.py b/devservices/configs/service_config.py index b45b73a6..bb91a7bc 100644 --- a/devservices/configs/service_config.py +++ b/devservices/configs/service_config.py @@ -3,12 +3,12 @@ import os from dataclasses import dataclass from dataclasses import fields -from enum import Enum import yaml from supervisor.options import ServerOptions from devservices.constants import CONFIG_FILE_NAME +from devservices.constants import DependencyType from devservices.constants import DEVSERVICES_DIR_NAME from devservices.constants import PROGRAMS_CONF_FILE_NAME from devservices.exceptions import ConfigNotFoundError @@ -19,11 +19,6 @@ VALID_VERSIONS = [0.1] -class DependencyType(Enum): - DOCKER_COMPOSE = "docker-compose" - SUPERVISOR = "supervisor" - - @dataclass class RemoteConfig: repo_name: str @@ -35,8 +30,8 @@ class RemoteConfig: @dataclass class Dependency: description: str + dependency_type: DependencyType remote: RemoteConfig | None = None - dependency_type: str | None = None @dataclass @@ -111,33 +106,32 @@ def load_service_config_from_file(repo_path: str) -> ServiceConfig: raise ConfigParseError( f"Unexpected key(s) in dependency '{key}': {unexpected_keys}" ) + if value.get("remote") is None: + if key in supervisor_programs: + dependency_type = DependencyType.SUPERVISOR + elif key in docker_compose_services: + dependency_type = DependencyType.COMPOSE + else: + raise ConfigValidationError( + f"Dependency '{key}' is not remote but is not defined in docker-compose services or programs file" + ) + else: + dependency_type = DependencyType.SERVICE + dependencies[key] = Dependency( description=value.get("description"), - remote=RemoteConfig(**value.get("remote")) - if "remote" in value - else None, + remote=( + RemoteConfig(**value.get("remote")) + if "remote" in value + else None + ), + dependency_type=dependency_type, ) except TypeError as type_error: raise ConfigParseError( f"Error parsing service dependencies: {type_error}" ) from type_error - # Validate that all non-remote dependencies are defined in docker-compose services - for dependency_name, dependency in dependencies.items(): - if dependency.remote is None: - if dependency_name in supervisor_programs: - dependencies[ - dependency_name - ].dependency_type = DependencyType.SUPERVISOR.value - elif dependency_name in docker_compose_services: - dependencies[ - dependency_name - ].dependency_type = DependencyType.DOCKER_COMPOSE.value - else: - raise ConfigValidationError( - f"Dependency '{dependency_name}' is not remote but is not defined in docker-compose services or programs file" - ) - service_config = ServiceConfig( version=service_config_data.get("version"), service_name=service_config_data.get("service_name"), diff --git a/devservices/constants.py b/devservices/constants.py index 80e8433b..ce6b96ed 100644 --- a/devservices/constants.py +++ b/devservices/constants.py @@ -2,6 +2,7 @@ import os from datetime import timedelta +from enum import Enum class Color: @@ -15,6 +16,12 @@ class Color: RESET = "\033[0m" +class DependencyType(str, Enum): + SERVICE = "service" + COMPOSE = "compose" + SUPERVISOR = "supervisor" + + MINIMUM_DOCKER_COMPOSE_VERSION = "2.29.7" DEVSERVICES_DIR_NAME = "devservices" CONFIG_FILE_NAME = "config.yml" diff --git a/devservices/utils/dependencies.py b/devservices/utils/dependencies.py index 018fe714..dcae655f 100644 --- a/devservices/utils/dependencies.py +++ b/devservices/utils/dependencies.py @@ -10,7 +10,6 @@ from concurrent.futures import as_completed from concurrent.futures import ThreadPoolExecutor from dataclasses import dataclass -from enum import Enum from typing import TextIO from typing import TypeGuard @@ -24,6 +23,7 @@ from devservices.constants import CONFIG_FILE_NAME from devservices.constants import DEPENDENCY_CONFIG_VERSION from devservices.constants import DEPENDENCY_GIT_PARTIAL_CLONE_CONFIG_OPTIONS +from devservices.constants import DependencyType from devservices.constants import DEVSERVICES_DEPENDENCIES_CACHE_DIR from devservices.constants import DEVSERVICES_DIR_NAME from devservices.constants import LOGGER_NAME @@ -55,11 +55,6 @@ ] -class DependencyType(str, Enum): - SERVICE = "service" - COMPOSE = "compose" - - @dataclass(frozen=True, eq=True) class DependencyNode: name: str @@ -751,6 +746,7 @@ def _construct_dependency_graph( for mode in modes: service_mode_dependencies.update(service_config.modes.get(mode, [])) for dependency_name, dependency in service_config.dependencies.items(): + print(dependency_name, dependency) # Skip the dependency if it's not in the modes (since it may not be installed and we don't care about it) if dependency_name not in service_mode_dependencies: continue @@ -761,9 +757,11 @@ def _construct_dependency_graph( ), DependencyNode( name=dependency_name, - dependency_type=DependencyType.SERVICE - if _has_remote_config(dependency.remote) - else DependencyType.COMPOSE, + dependency_type=( + DependencyType.SERVICE + if _has_remote_config(dependency.remote) + else dependency.dependency_type + ), ), ) if _has_remote_config(dependency.remote): diff --git a/tests/configs/test_service_config.py b/tests/configs/test_service_config.py index 1b3f326e..7809e0b3 100644 --- a/tests/configs/test_service_config.py +++ b/tests/configs/test_service_config.py @@ -5,9 +5,9 @@ import pytest -from devservices.configs.service_config import DependencyType from devservices.configs.service_config import load_service_config_from_file from devservices.configs.service_config import load_supervisor_programs_from_file +from devservices.constants import DependencyType from devservices.exceptions import ConfigNotFoundError from devservices.exceptions import ConfigParseError from devservices.exceptions import ConfigValidationError @@ -23,7 +23,7 @@ {"example-dependency": {"description": "Example dependency"}}, {"default": ["example-dependency"]}, { - "example-dependency": DependencyType.DOCKER_COMPOSE.value, + "example-dependency": DependencyType.COMPOSE, }, ), ( @@ -44,8 +44,8 @@ }, {"default": ["example-dependency-1", "example-dependency-2"]}, { - "example-dependency-1": None, - "example-dependency-2": DependencyType.DOCKER_COMPOSE.value, + "example-dependency-1": DependencyType.SERVICE, + "example-dependency-2": DependencyType.COMPOSE, }, ), ( @@ -66,8 +66,8 @@ }, {"default": ["example-dependency-1"], "custom": ["example-dependency-2"]}, { - "example-dependency-1": None, - "example-dependency-2": DependencyType.DOCKER_COMPOSE.value, + "example-dependency-1": DependencyType.SERVICE, + "example-dependency-2": DependencyType.COMPOSE, }, ), ], @@ -493,11 +493,11 @@ def test_load_service_config_from_file_valid_programs_file(tmp_path: Path) -> No service_config = load_service_config_from_file(str(tmp_path)) assert ( service_config.dependencies["example-program"].dependency_type - == DependencyType.SUPERVISOR.value + == DependencyType.SUPERVISOR ) assert ( service_config.dependencies["example-dependency"].dependency_type - == DependencyType.DOCKER_COMPOSE.value + == DependencyType.COMPOSE ) From eb39a558eef1403f8c7bb617b66f0d38c402efcd Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Tue, 13 May 2025 14:30:52 -0700 Subject: [PATCH 07/20] remove print statement --- devservices/utils/dependencies.py | 1 - 1 file changed, 1 deletion(-) diff --git a/devservices/utils/dependencies.py b/devservices/utils/dependencies.py index dcae655f..bc1f01d2 100644 --- a/devservices/utils/dependencies.py +++ b/devservices/utils/dependencies.py @@ -746,7 +746,6 @@ def _construct_dependency_graph( for mode in modes: service_mode_dependencies.update(service_config.modes.get(mode, [])) for dependency_name, dependency in service_config.dependencies.items(): - print(dependency_name, dependency) # Skip the dependency if it's not in the modes (since it may not be installed and we don't care about it) if dependency_name not in service_mode_dependencies: continue From 6d226757dcb73300cac4b9c01c0c631b29ffffb0 Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Tue, 13 May 2025 14:42:34 -0700 Subject: [PATCH 08/20] fix tests --- tests/utils/test_dependencies.py | 38 +++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/tests/utils/test_dependencies.py b/tests/utils/test_dependencies.py index f5ae14ae..af229246 100644 --- a/tests/utils/test_dependencies.py +++ b/tests/utils/test_dependencies.py @@ -15,6 +15,7 @@ from devservices.constants import CONFIG_FILE_NAME from devservices.constants import DEPENDENCY_CONFIG_VERSION from devservices.constants import DEPENDENCY_GIT_PARTIAL_CLONE_CONFIG_OPTIONS +from devservices.constants import DependencyType from devservices.constants import DEVSERVICES_DIR_NAME from devservices.exceptions import DependencyError from devservices.exceptions import DependencyNotInstalledError @@ -23,7 +24,6 @@ from devservices.exceptions import ModeDoesNotExistError from devservices.utils.dependencies import construct_dependency_graph from devservices.utils.dependencies import DependencyNode -from devservices.utils.dependencies import DependencyType from devservices.utils.dependencies import get_installed_remote_dependencies from devservices.utils.dependencies import get_non_shared_remote_dependencies from devservices.utils.dependencies import GitConfigManager @@ -203,6 +203,7 @@ def test_verify_local_dependencies_no_remote_dependencies(tmp_path: Path) -> Non ): dependency = Dependency( description="Test dependency", + dependency_type=DependencyType.COMPOSE, ) assert verify_local_dependencies([dependency]) @@ -221,6 +222,7 @@ def test_verify_local_dependencies_with_remote_dependencies(tmp_path: Path) -> N dependency = Dependency( description="Test dependency", remote=remote_config, + dependency_type=DependencyType.COMPOSE, ) assert not verify_local_dependencies([dependency]) @@ -255,6 +257,7 @@ def test_get_installed_remote_dependencies_single_dep_not_installed( branch="main", repo_link=f"file://{tmp_path / 'test-repo'}", ), + dependency_type=DependencyType.SERVICE, ) with pytest.raises(DependencyNotInstalledError): get_installed_remote_dependencies(dependencies=[mock_dependency]) @@ -273,6 +276,7 @@ def test_get_installed_remote_dependencies_single_dep_installed(tmp_path: Path) branch="main", repo_link=f"file://{tmp_path / 'test-repo'}", ), + dependency_type=DependencyType.SERVICE, ) installed_remote_dependencies_initial = install_dependencies([mock_dependency]) installed_remote_dependencies = get_installed_remote_dependencies( @@ -1963,6 +1967,7 @@ def test_install_dependencies_nested_dependency_file_contention(tmp_path: Path) branch="main", repo_link=f"file://{repo_a_path}", ), + dependency_type=DependencyType.SERVICE, ) repo_b_dependency = Dependency( description="repo b", @@ -1971,6 +1976,7 @@ def test_install_dependencies_nested_dependency_file_contention(tmp_path: Path) branch="main", repo_link=f"file://{repo_b_path}", ), + dependency_type=DependencyType.SERVICE, ) dependencies = [repo_a_dependency, repo_b_dependency] @@ -2080,6 +2086,7 @@ def test_get_non_shared_remote_dependencies_no_shared_dependencies( repo_link="file://path/to/dependency-1", branch="main", ), + dependency_type=DependencyType.SERVICE, ) }, modes={"default": ["dependency-1"]}, @@ -2136,6 +2143,7 @@ def test_get_non_shared_remote_dependencies_no_shared_dependencies( repo_link="file://path/to/dependency-1", branch="main", ), + dependency_type=DependencyType.SERVICE, ) }, modes={"default": ["dependency-1"]}, @@ -2171,6 +2179,7 @@ def test_get_non_shared_remote_dependencies_shared_dependencies( repo_link="file://path/to/dependency-1", branch="main", ), + dependency_type=DependencyType.SERVICE, ) }, modes={"default": ["dependency-1"]}, @@ -2200,6 +2209,7 @@ def test_get_non_shared_remote_dependencies_shared_dependencies( repo_link="file://path/to/dependency-1", branch="main", ), + dependency_type=DependencyType.SERVICE, ) ] ) @@ -2234,6 +2244,7 @@ def test_get_non_shared_remote_dependencies_shared_dependencies( repo_link="file://path/to/dependency-3", branch="main", ), + dependency_type=DependencyType.SERVICE, ), "dependency-4": Dependency( description="dependency-4", @@ -2242,6 +2253,7 @@ def test_get_non_shared_remote_dependencies_shared_dependencies( repo_link="file://path/to/dependency-4", branch="main", ), + dependency_type=DependencyType.SERVICE, ), }, modes={"default": ["dependency-3"], "other": ["dependency-4"]}, @@ -2278,6 +2290,7 @@ def test_get_non_shared_remote_dependencies_nested_shared_dependencies( repo_link="file://path/to/dependency-1", branch="main", ), + dependency_type=DependencyType.SERVICE, ), "dependency-2": Dependency( description="dependency-2", @@ -2286,6 +2299,7 @@ def test_get_non_shared_remote_dependencies_nested_shared_dependencies( repo_link="file://path/to/dependency-2", branch="main", ), + dependency_type=DependencyType.SERVICE, ), }, modes={"default": ["dependency-1", "dependency-2"]}, @@ -2327,6 +2341,7 @@ def test_get_non_shared_remote_dependencies_nested_shared_dependencies( repo_link="file://path/to/dependency-3", branch="main", ), + dependency_type=DependencyType.SERVICE, ) ] ) @@ -2352,6 +2367,7 @@ def test_get_non_shared_remote_dependencies_nested_shared_dependencies( repo_link="file://path/to/dependency-3", branch="main", ), + dependency_type=DependencyType.SERVICE, ), }, modes={"default": ["dependency-3"]}, @@ -2388,6 +2404,7 @@ def test_get_non_shared_remote_dependencies_with_local_runtime_dependency( repo_link="file://path/to/dependency-1", branch="main", ), + dependency_type=DependencyType.SERVICE, ), "service-2": Dependency( description="service-2", @@ -2396,6 +2413,7 @@ def test_get_non_shared_remote_dependencies_with_local_runtime_dependency( repo_link="file://path/to/service-2", branch="main", ), + dependency_type=DependencyType.SERVICE, ), }, modes={"default": ["dependency-1", "service-2"]}, @@ -2455,6 +2473,7 @@ def test_get_non_shared_remote_dependencies_with_local_runtime_dependency( repo_link="file://path/to/dependency-3", branch="main", ), + dependency_type=DependencyType.SERVICE, ) ] ) @@ -2478,6 +2497,7 @@ def test_install_and_verify_dependencies_simple( repo_link="file://path/to/dependency-1", branch="main", ), + dependency_type=DependencyType.SERVICE, ), "dependency-2": Dependency( description="dependency-2", @@ -2486,6 +2506,7 @@ def test_install_and_verify_dependencies_simple( repo_link="file://path/to/dependency-2", branch="main", ), + dependency_type=DependencyType.SERVICE, ), }, modes={"default": ["dependency-1", "dependency-2"]}, @@ -2519,6 +2540,7 @@ def test_install_and_verify_dependencies_mode_simple( repo_link="file://path/to/dependency-1", branch="main", ), + dependency_type=DependencyType.SERVICE, ), "dependency-2": Dependency( description="dependency-2", @@ -2527,6 +2549,7 @@ def test_install_and_verify_dependencies_mode_simple( repo_link="file://path/to/dependency-2", branch="main", ), + dependency_type=DependencyType.SERVICE, ), }, modes={ @@ -2557,6 +2580,7 @@ def test_install_and_verify_dependencies_mode_does_not_exist(tmp_path: Path) -> repo_link="file://path/to/dependency-1", branch="main", ), + dependency_type=DependencyType.SERVICE, ), "dependency-2": Dependency( description="dependency-2", @@ -2565,6 +2589,7 @@ def test_install_and_verify_dependencies_mode_does_not_exist(tmp_path: Path) -> repo_link="file://path/to/dependency-2", branch="main", ), + dependency_type=DependencyType.SERVICE, ), }, modes={"default": ["dependency-1", "dependency-2"]}, @@ -2615,6 +2640,7 @@ def test_construct_dependency_graph_simple( repo_link=f"file://{dependency_service_repo_path}", branch="main", ), + dependency_type=DependencyType.SERVICE, ), }, modes={ @@ -2729,9 +2755,11 @@ def test_construct_dependency_graph_one_nested_dependency( repo_link=f"file://{parent_service_repo_path}", branch="main", ), + dependency_type=DependencyType.SERVICE, ), "grandparent-service": Dependency( description="grandparent-service", + dependency_type=DependencyType.COMPOSE, ), }, modes={ @@ -2877,9 +2905,11 @@ def test_construct_dependency_graph_shared_dependency( repo_link=f"file://{parent_service_repo_path}", branch="main", ), + dependency_type=DependencyType.SERVICE, ), "grandparent-service": Dependency( description="grandparent-service", + dependency_type=DependencyType.COMPOSE, ), "child-service": Dependency( description="child-service", @@ -2888,6 +2918,7 @@ def test_construct_dependency_graph_shared_dependency( repo_link=f"file://{child_service_repo_path}", branch="main", ), + dependency_type=DependencyType.SERVICE, ), }, modes={ @@ -3037,9 +3068,11 @@ def test_construct_dependency_graph_non_self_reference( repo_link=f"file://{parent_service_repo_path}", branch="main", ), + dependency_type=DependencyType.SERVICE, ), "grandparent-service-container": Dependency( description="grandparent-service-container", + dependency_type=DependencyType.COMPOSE, ), }, modes={ @@ -3252,6 +3285,7 @@ def test_construct_dependency_graph_complex( repo_link=f"file://{child_service_repo_path}", branch="main", ), + dependency_type=DependencyType.SERVICE, ), "grandparent-service": Dependency( description="grandparent-service", @@ -3260,9 +3294,11 @@ def test_construct_dependency_graph_complex( repo_link=f"file://{grandparent_service_repo_path}", branch="main", ), + dependency_type=DependencyType.SERVICE, ), "complex-service": Dependency( description="complex-service", + dependency_type=DependencyType.COMPOSE, ), }, modes={ From 03b77d411cf67368dde96445ffaeaf56ce1e06e8 Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Tue, 13 May 2025 15:32:06 -0700 Subject: [PATCH 09/20] actually fix tests --- devservices/commands/down.py | 2 +- devservices/commands/reset.py | 2 +- devservices/commands/status.py | 2 +- devservices/commands/up.py | 2 +- tests/commands/test_down.py | 6 +++ tests/commands/test_list_dependencies.py | 9 ++++- tests/commands/test_logs.py | 17 +++++++-- tests/commands/test_status.py | 22 ++++++++--- tests/commands/test_toggle.py | 48 +++++++++++++++++++++--- 9 files changed, 90 insertions(+), 20 deletions(-) diff --git a/devservices/commands/down.py b/devservices/commands/down.py index 8d34134c..0836ecb6 100644 --- a/devservices/commands/down.py +++ b/devservices/commands/down.py @@ -11,6 +11,7 @@ from devservices.constants import CONFIG_FILE_NAME from devservices.constants import DEPENDENCY_CONFIG_VERSION +from devservices.constants import DependencyType from devservices.constants import DEVSERVICES_DEPENDENCIES_CACHE_DIR from devservices.constants import DEVSERVICES_DEPENDENCIES_CACHE_DIR_KEY from devservices.constants import DEVSERVICES_DIR_NAME @@ -23,7 +24,6 @@ from devservices.utils.console import Status from devservices.utils.dependencies import construct_dependency_graph from devservices.utils.dependencies import DependencyNode -from devservices.utils.dependencies import DependencyType from devservices.utils.dependencies import get_non_shared_remote_dependencies from devservices.utils.dependencies import install_and_verify_dependencies from devservices.utils.dependencies import InstalledRemoteDependency diff --git a/devservices/commands/reset.py b/devservices/commands/reset.py index 1e936c5f..f0c909a0 100644 --- a/devservices/commands/reset.py +++ b/devservices/commands/reset.py @@ -7,6 +7,7 @@ from sentry_sdk import capture_exception from devservices.commands.down import down +from devservices.constants import DependencyType from devservices.constants import DEVSERVICES_ORCHESTRATOR_LABEL from devservices.exceptions import DockerDaemonNotRunningError from devservices.exceptions import DockerError @@ -14,7 +15,6 @@ from devservices.utils.console import Status from devservices.utils.dependencies import construct_dependency_graph from devservices.utils.dependencies import DependencyNode -from devservices.utils.dependencies import DependencyType from devservices.utils.docker import get_matching_containers from devservices.utils.docker import get_volumes_for_containers from devservices.utils.docker import remove_docker_resources diff --git a/devservices/commands/status.py b/devservices/commands/status.py index f04f4f58..59db155b 100644 --- a/devservices/commands/status.py +++ b/devservices/commands/status.py @@ -15,6 +15,7 @@ from devservices.constants import Color from devservices.constants import CONFIG_FILE_NAME from devservices.constants import DEPENDENCY_CONFIG_VERSION +from devservices.constants import DependencyType from devservices.constants import DEVSERVICES_DEPENDENCIES_CACHE_DIR from devservices.constants import DEVSERVICES_DEPENDENCIES_CACHE_DIR_KEY from devservices.constants import DEVSERVICES_DIR_NAME @@ -27,7 +28,6 @@ from devservices.utils.dependencies import construct_dependency_graph from devservices.utils.dependencies import DependencyGraph from devservices.utils.dependencies import DependencyNode -from devservices.utils.dependencies import DependencyType from devservices.utils.dependencies import install_and_verify_dependencies from devservices.utils.dependencies import InstalledRemoteDependency from devservices.utils.docker_compose import get_docker_compose_commands_to_run diff --git a/devservices/commands/up.py b/devservices/commands/up.py index 0d3cb765..23a0014a 100644 --- a/devservices/commands/up.py +++ b/devservices/commands/up.py @@ -11,6 +11,7 @@ from devservices.constants import CONFIG_FILE_NAME from devservices.constants import DEPENDENCY_CONFIG_VERSION +from devservices.constants import DependencyType from devservices.constants import DEVSERVICES_DEPENDENCIES_CACHE_DIR from devservices.constants import DEVSERVICES_DEPENDENCIES_CACHE_DIR_KEY from devservices.constants import DEVSERVICES_DIR_NAME @@ -25,7 +26,6 @@ from devservices.utils.console import Status from devservices.utils.dependencies import construct_dependency_graph from devservices.utils.dependencies import DependencyNode -from devservices.utils.dependencies import DependencyType from devservices.utils.dependencies import install_and_verify_dependencies from devservices.utils.dependencies import InstalledRemoteDependency from devservices.utils.docker import check_all_containers_healthy diff --git a/tests/commands/test_down.py b/tests/commands/test_down.py index 36b7ee67..125c205a 100644 --- a/tests/commands/test_down.py +++ b/tests/commands/test_down.py @@ -13,6 +13,7 @@ from devservices.configs.service_config import RemoteConfig from devservices.configs.service_config import ServiceConfig from devservices.constants import CONFIG_FILE_NAME +from devservices.constants import DependencyType from devservices.constants import DEVSERVICES_DIR_NAME from devservices.exceptions import ConfigError from devservices.exceptions import ServiceNotFoundError @@ -632,6 +633,7 @@ def test_down_does_not_stop_service_being_used_by_another_service( dependencies={ "redis": Dependency( description="Redis", + dependency_type=DependencyType.SERVICE, remote=RemoteConfig( repo_name="redis", repo_link=f"file://{redis_repo_path}", @@ -641,6 +643,7 @@ def test_down_does_not_stop_service_being_used_by_another_service( ), "example-service": Dependency( description="Example service", + dependency_type=DependencyType.SERVICE, remote=RemoteConfig( repo_name="example-service", repo_link=f"file://{example_repo_path}", @@ -814,6 +817,7 @@ def test_down_does_not_stop_nested_service_being_used_by_another_service( dependencies={ "parent-service": Dependency( description="Parent service", + dependency_type=DependencyType.SERVICE, remote=RemoteConfig( repo_name="parent-service", repo_link=f"file://{parent_repo_path}", @@ -1094,6 +1098,7 @@ def test_down_local_service_with_dependent_service_running( dependencies={ "redis": Dependency( description="Redis", + dependency_type=DependencyType.SERVICE, remote=RemoteConfig( repo_name="redis", repo_link=f"file://{redis_repo_path}", @@ -1103,6 +1108,7 @@ def test_down_local_service_with_dependent_service_running( ), "local-runtime-service": Dependency( description="Local runtime service", + dependency_type=DependencyType.SERVICE, remote=RemoteConfig( repo_name="local-runtime-service", repo_link=f"file://{local_runtime_repo_path}", diff --git a/tests/commands/test_list_dependencies.py b/tests/commands/test_list_dependencies.py index d866d6a1..84e247fd 100644 --- a/tests/commands/test_list_dependencies.py +++ b/tests/commands/test_list_dependencies.py @@ -10,6 +10,7 @@ from devservices.commands.list_dependencies import list_dependencies from devservices.configs.service_config import Dependency from devservices.configs.service_config import ServiceConfig +from devservices.constants import DependencyType from devservices.exceptions import ConfigValidationError from devservices.exceptions import ServiceNotFoundError from devservices.utils.services import Service @@ -115,8 +116,12 @@ def test_list_dependencies_with_dependencies( version=0.1, service_name="test-service", dependencies={ - "redis": Dependency(description="Redis"), - "postgres": Dependency(description="Postgres"), + "redis": Dependency( + description="Redis", dependency_type=DependencyType.COMPOSE + ), + "postgres": Dependency( + description="Postgres", dependency_type=DependencyType.COMPOSE + ), }, modes={"default": ["redis", "postgres"]}, ), diff --git a/tests/commands/test_logs.py b/tests/commands/test_logs.py index b5418ef0..9eda8ad5 100644 --- a/tests/commands/test_logs.py +++ b/tests/commands/test_logs.py @@ -12,6 +12,7 @@ from devservices.configs.service_config import Dependency from devservices.configs.service_config import ServiceConfig from devservices.constants import CONFIG_FILE_NAME +from devservices.constants import DependencyType from devservices.constants import DEVSERVICES_DIR_NAME from devservices.exceptions import ConfigError from devservices.exceptions import ServiceNotFoundError @@ -42,8 +43,12 @@ def test_logs_no_specified_service_not_running( version=0.1, service_name="example-service", dependencies={ - "redis": Dependency(description="Redis"), - "clickhouse": Dependency(description="Clickhouse"), + "redis": Dependency( + description="Redis", dependency_type=DependencyType.COMPOSE + ), + "clickhouse": Dependency( + description="Clickhouse", dependency_type=DependencyType.COMPOSE + ), }, modes={"default": ["redis", "clickhouse"]}, ), @@ -98,8 +103,12 @@ def test_logs_no_specified_service_success( version=0.1, service_name="example-service", dependencies={ - "redis": Dependency(description="Redis"), - "clickhouse": Dependency(description="Clickhouse"), + "redis": Dependency( + description="Redis", dependency_type=DependencyType.COMPOSE + ), + "clickhouse": Dependency( + description="Clickhouse", dependency_type=DependencyType.COMPOSE + ), }, modes={"default": ["redis", "clickhouse"]}, ), diff --git a/tests/commands/test_status.py b/tests/commands/test_status.py index cf21a3c5..52a577b3 100644 --- a/tests/commands/test_status.py +++ b/tests/commands/test_status.py @@ -20,13 +20,13 @@ from devservices.configs.service_config import ServiceConfig from devservices.constants import Color from devservices.constants import CONFIG_FILE_NAME +from devservices.constants import DependencyType from devservices.constants import DEVSERVICES_DIR_NAME from devservices.exceptions import DependencyError from devservices.exceptions import DockerComposeError from devservices.exceptions import ServiceNotFoundError from devservices.utils.dependencies import DependencyGraph from devservices.utils.dependencies import DependencyNode -from devservices.utils.dependencies import DependencyType from devservices.utils.services import Service from devservices.utils.state import State from devservices.utils.state import StateTables @@ -79,8 +79,14 @@ def test_get_status_json_results( version=0.1, service_name="test-service", dependencies={ - "redis": Dependency(description="Redis"), - "clickhouse": Dependency(description="Clickhouse"), + "redis": Dependency( + description="Redis", + dependency_type=DependencyType.COMPOSE, + ), + "clickhouse": Dependency( + description="Clickhouse", + dependency_type=DependencyType.COMPOSE, + ), }, modes={"default": ["redis", "clickhouse"], "test": ["redis"]}, ), @@ -541,8 +547,14 @@ def test_handle_started_service( version=0.1, service_name="test-service", dependencies={ - "redis": Dependency(description="Redis"), - "clickhouse": Dependency(description="Clickhouse"), + "redis": Dependency( + description="Redis", + dependency_type=DependencyType.COMPOSE, + ), + "clickhouse": Dependency( + description="Clickhouse", + dependency_type=DependencyType.COMPOSE, + ), }, modes={"default": ["redis", "clickhouse"], "test": ["redis"]}, ), diff --git a/tests/commands/test_toggle.py b/tests/commands/test_toggle.py index f397ee61..f15752f2 100644 --- a/tests/commands/test_toggle.py +++ b/tests/commands/test_toggle.py @@ -17,6 +17,7 @@ from devservices.configs.service_config import RemoteConfig from devservices.configs.service_config import ServiceConfig from devservices.constants import CONFIG_FILE_NAME +from devservices.constants import DependencyType from devservices.constants import DEVSERVICES_DIR_NAME from devservices.exceptions import CannotToggleNonRemoteServiceError from devservices.exceptions import ConfigNotFoundError @@ -333,6 +334,7 @@ def test_toggle_dependent_service_running( branch="main", mode="default", ), + dependency_type=DependencyType.SERVICE, ), "example-service": Dependency( description="Example service", @@ -342,6 +344,7 @@ def test_toggle_dependent_service_running( branch="main", mode="default", ), + dependency_type=DependencyType.SERVICE, ), }, modes={"default": ["redis", "example-service"]}, @@ -416,6 +419,7 @@ def test_toggle_to_local_runtime_no_runtime_specified( "clickhouse": Dependency( description="Clickhouse", remote=None, + dependency_type=DependencyType.COMPOSE, ), }, modes={"default": ["clickhouse"]}, @@ -439,6 +443,7 @@ def test_toggle_to_local_runtime_no_runtime_specified( "clickhouse": Dependency( description="Clickhouse", remote=None, + dependency_type=DependencyType.COMPOSE, ), }, modes={"default": ["clickhouse"]}, @@ -468,6 +473,7 @@ def test_toggle_cannot_toggle_non_remote_service( "clickhouse": Dependency( description="Clickhouse", remote=None, + dependency_type=DependencyType.COMPOSE, ), }, modes={"default": ["clickhouse"]}, @@ -497,6 +503,7 @@ def test_toggle_cannot_toggle_non_remote_service( "clickhouse": Dependency( description="Clickhouse", remote=None, + dependency_type=DependencyType.COMPOSE, ), }, modes={"default": ["clickhouse"]}, @@ -536,6 +543,7 @@ def test_handle_transition_to_local_runtime_currently_running_standalone( "clickhouse": Dependency( description="Clickhouse", remote=None, + dependency_type=DependencyType.COMPOSE, ), }, modes={"default": ["clickhouse"]}, @@ -616,6 +624,7 @@ def test_handle_transition_to_local_runtime_naming_conflict( "clickhouse": Dependency( description="Clickhouse", remote=None, + dependency_type=DependencyType.COMPOSE, ), }, modes={"default": ["clickhouse"]}, @@ -640,6 +649,7 @@ def test_handle_transition_to_local_runtime_naming_conflict( "clickhouse": Dependency( description="Clickhouse", remote=None, + dependency_type=DependencyType.COMPOSE, ), }, modes={"default": ["clickhouse"]}, @@ -694,6 +704,7 @@ def test_handle_transition_to_containerized_runtime_no_dependent_services( "redis": Dependency( description="Redis", remote=None, + dependency_type=DependencyType.COMPOSE, ), }, modes={"default": ["redis"]}, @@ -745,7 +756,11 @@ def test_handle_transition_to_containerized_runtime_with_service_running( version=0.1, service_name="redis", dependencies={ - "redis": Dependency(description="Redis", remote=None), + "redis": Dependency( + description="Redis", + remote=None, + dependency_type=DependencyType.COMPOSE, + ), }, modes={"default": ["redis"]}, ), @@ -872,6 +887,7 @@ def test_handle_transition_to_containerized_runtime_with_dependent_services( branch="main", mode="default", ), + dependency_type=DependencyType.SERVICE, ), "example-service": Dependency( description="Example service", @@ -881,6 +897,7 @@ def test_handle_transition_to_containerized_runtime_with_dependent_services( branch="main", mode="default", ), + dependency_type=DependencyType.SERVICE, ), }, modes={"default": ["redis", "example-service"]}, @@ -911,10 +928,12 @@ def test_handle_transition_to_containerized_runtime_with_dependent_services( branch="main", mode="default", ), + dependency_type=DependencyType.SERVICE, ), "clickhouse": Dependency( description="Clickhouse", remote=None, + dependency_type=DependencyType.COMPOSE, ), }, modes={"default": ["redis", "clickhouse"]}, @@ -1162,6 +1181,7 @@ def test_bring_down_containerized_service_install_and_verify_dependencies_failur "clickhouse": Dependency( description="Clickhouse", remote=None, + dependency_type=DependencyType.COMPOSE, ), }, modes={"default": ["clickhouse"]}, @@ -1214,6 +1234,7 @@ def test_bring_down_containerized_service_no_remote_dependencies( "clickhouse": Dependency( description="Clickhouse", remote=None, + dependency_type=DependencyType.COMPOSE, ), }, modes={"default": ["clickhouse"]}, @@ -1230,7 +1251,11 @@ def test_bring_down_containerized_service_no_remote_dependencies( version=0.1, service_name="example-service", dependencies={ - "clickhouse": Dependency(description="Clickhouse", remote=None), + "clickhouse": Dependency( + description="Clickhouse", + remote=None, + dependency_type=DependencyType.COMPOSE, + ), }, modes={"default": ["clickhouse"]}, ), @@ -1319,7 +1344,11 @@ def test_bring_down_containerized_service_with_remote_dependency( version=0.1, service_name="example-service", dependencies={ - "clickhouse": Dependency(description="Clickhouse", remote=None), + "clickhouse": Dependency( + description="Clickhouse", + remote=None, + dependency_type=DependencyType.COMPOSE, + ), "redis": Dependency( description="Redis", remote=RemoteConfig( @@ -1328,6 +1357,7 @@ def test_bring_down_containerized_service_with_remote_dependency( branch="main", mode="default", ), + dependency_type=DependencyType.SERVICE, ), }, modes={"default": ["clickhouse", "redis"]}, @@ -1347,7 +1377,9 @@ def test_bring_down_containerized_service_with_remote_dependency( service_name="example-service", dependencies={ "clickhouse": Dependency( - description="Clickhouse", remote=None + description="Clickhouse", + remote=None, + dependency_type=DependencyType.COMPOSE, ), "redis": Dependency( description="Redis", @@ -1357,6 +1389,7 @@ def test_bring_down_containerized_service_with_remote_dependency( branch="main", mode="default", ), + dependency_type=DependencyType.SERVICE, ), }, modes={"default": ["clickhouse", "redis"]}, @@ -1410,6 +1443,7 @@ def test_bring_down_containerized_service_get_non_shared_remote_dependencies_err branch="main", mode="default", ), + dependency_type=DependencyType.SERVICE, ), }, modes={"default": ["redis"]}, @@ -1434,6 +1468,7 @@ def test_bring_down_containerized_service_get_non_shared_remote_dependencies_err branch="main", mode="default", ), + dependency_type=DependencyType.SERVICE, ), }, modes={"default": ["redis"]}, @@ -1458,6 +1493,7 @@ def test_bring_down_containerized_service_get_non_shared_remote_dependencies_err branch="main", mode="default", ), + dependency_type=DependencyType.SERVICE, ), }, modes={"default": ["redis"]}, @@ -1519,7 +1555,9 @@ def test_bring_down_containerized_service_docker_compose_error( service_name="example-service", dependencies={ "clickhouse": Dependency( - description="Clickhouse", remote=None + description="Clickhouse", + remote=None, + dependency_type=DependencyType.COMPOSE, ), }, modes={"default": ["clickhouse"]}, From c92b0c16639910ce9bf76b899a6435aa9e1e623d Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Wed, 14 May 2025 09:39:16 -0700 Subject: [PATCH 10/20] fix dependency_type --- devservices/utils/dependencies.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/devservices/utils/dependencies.py b/devservices/utils/dependencies.py index bc1f01d2..6fe0b8d8 100644 --- a/devservices/utils/dependencies.py +++ b/devservices/utils/dependencies.py @@ -755,12 +755,7 @@ def _construct_dependency_graph( dependency_type=DependencyType.SERVICE, ), DependencyNode( - name=dependency_name, - dependency_type=( - DependencyType.SERVICE - if _has_remote_config(dependency.remote) - else dependency.dependency_type - ), + name=dependency_name, dependency_type=dependency.dependency_type ), ) if _has_remote_config(dependency.remote): From 0a8c34f3fd81baf318c6e42bd94e47035f75ac86 Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Thu, 15 May 2025 16:04:09 -0700 Subject: [PATCH 11/20] add supervisor program to up --- devservices/commands/up.py | 50 ++++- devservices/utils/supervisor.py | 56 ++++++ tests/commands/test_up.py | 319 ++++++++++++++++++++++++++++++++ tests/utils/test_supervisor.py | 106 ++++++++++- 4 files changed, 527 insertions(+), 4 deletions(-) diff --git a/devservices/commands/up.py b/devservices/commands/up.py index 23a0014a..bd023086 100644 --- a/devservices/commands/up.py +++ b/devservices/commands/up.py @@ -15,6 +15,7 @@ from devservices.constants import DEVSERVICES_DEPENDENCIES_CACHE_DIR from devservices.constants import DEVSERVICES_DEPENDENCIES_CACHE_DIR_KEY from devservices.constants import DEVSERVICES_DIR_NAME +from devservices.constants import PROGRAMS_CONF_FILE_NAME from devservices.exceptions import ConfigError from devservices.exceptions import ConfigNotFoundError from devservices.exceptions import ContainerHealthcheckFailedError @@ -22,6 +23,8 @@ from devservices.exceptions import DockerComposeError from devservices.exceptions import ModeDoesNotExistError from devservices.exceptions import ServiceNotFoundError +from devservices.exceptions import SupervisorConfigError +from devservices.exceptions import SupervisorError from devservices.utils.console import Console from devservices.utils.console import Status from devservices.utils.dependencies import construct_dependency_graph @@ -38,6 +41,7 @@ from devservices.utils.state import ServiceRuntime from devservices.utils.state import State from devservices.utils.state import StateTables +from devservices.utils.supervisor import SupervisorManager def add_parser(subparsers: _SubParsersAction[ArgumentParser]) -> None: @@ -151,6 +155,15 @@ def up(args: Namespace, existing_status: Status | None = None) -> None: mode_dependencies = [ dep for dep in mode_dependencies if dep not in services_with_local_runtime ] + + supervisor_programs = [ + dep + for dep in mode_dependencies + if dep in service.config.dependencies + and service.config.dependencies[dep].dependency_type + == DependencyType.SUPERVISOR + ] + remote_dependencies = { dep for dep in remote_dependencies @@ -169,12 +182,18 @@ def up(args: Namespace, existing_status: Status | None = None) -> None: ) status.warning(f"Continuing with service '{service.name}'") try: - _up(service, [mode], remote_dependencies, mode_dependencies, status) + bring_up_docker_compose_services( + service, [mode], remote_dependencies, mode_dependencies, status + ) except DockerComposeError as dce: capture_exception(dce, level="info") status.failure(f"Failed to start {service.name}: {dce.stderr}") exit(1) - # TODO: We should factor in healthchecks here before marking service as running + try: + bring_up_supervisor_programs(supervisor_programs, service, status) + except SupervisorError as se: + status.failure(str(se)) + exit(1) state.remove_service_entry(service.name, StateTables.STARTING_SERVICES) state.update_service_entry(service.name, mode, StateTables.STARTED_SERVICES) @@ -195,7 +214,7 @@ def _bring_up_dependency( run_cmd(cmd.full_command, current_env) -def _up( +def bring_up_docker_compose_services( service: Service, modes: list[str], remote_dependencies: set[InstalledRemoteDependency], @@ -286,6 +305,31 @@ def _up( exit(1) +def bring_up_supervisor_programs( + supervisor_programs: list[str], service: Service, status: Status +) -> None: + if len(supervisor_programs) == 0: + return + programs_config_path = os.path.join( + service.repo_path, f"{DEVSERVICES_DIR_NAME}/{PROGRAMS_CONF_FILE_NAME}" + ) + if not os.path.exists(programs_config_path): + raise SupervisorConfigError( + f"No programs.conf file found in {programs_config_path}." + ) + manager = SupervisorManager( + programs_config_path, + service_name=service.name, + ) + + status.info("Starting supervisor daemon") + manager.start_supervisor_daemon() + + for program in supervisor_programs: + status.info(f"Starting {program}") + manager.start_process(program) + + def _create_devservices_network() -> None: subprocess.run( ["docker", "network", "create", "devservices"], diff --git a/devservices/utils/supervisor.py b/devservices/utils/supervisor.py index 6d4dc39b..579929d8 100644 --- a/devservices/utils/supervisor.py +++ b/devservices/utils/supervisor.py @@ -5,6 +5,7 @@ import os import socket import subprocess +import time import xmlrpc.client from enum import IntEnum @@ -35,6 +36,19 @@ class SupervisorProcessState(IntEnum): UNKNOWN = 1000 +class SupervisorDaemonState(IntEnum): + """ + Supervisor daemon states. + + https://supervisord.org/api.html#supervisor.rpcinterface.SupervisorNamespaceRPCInterface.getState + """ + + FATAL = 2 + RUNNING = 1 + RESTARTING = 0 + SHUTDOWN = -1 + + class UnixSocketHTTPConnection(http.client.HTTPConnection): """HTTP connection over Unix sockets.""" @@ -137,9 +151,51 @@ def _is_program_running(self, program_name: str) -> bool: # If we can't get the process info, assume it's not running return False + def _wait_for_supervisor_ready( + self, timeout: int = 10, interval: float = 0.5 + ) -> None: + for _ in range(int(timeout / interval)): + try: + client = self._get_rpc_client() + state = client.supervisor.getState() + # Unfortunately supervisor is untyped, so we need to assert the types + assert isinstance(state, dict) + assert "statecode" in state + if state.get("statecode") == SupervisorDaemonState.RUNNING: + return + time.sleep(interval) + except ( + SupervisorConnectionError, + socket.error, + ConnectionRefusedError, + xmlrpc.client.Fault, + ): + time.sleep(interval) + + raise SupervisorError( + f"Supervisor didn't become ready within {timeout} seconds" + ) + def start_supervisor_daemon(self) -> None: + # Check if supervisor is already running by attempting to connect to it + try: + client = self._get_rpc_client() + client.supervisor.getState() + # Supervisor is already running, restart it since config may have changed + client.supervisor.restart() + + # Wait for supervisor to be ready after restart + self._wait_for_supervisor_ready() + return + except (SupervisorConnectionError, socket.error, ConnectionRefusedError): + # Supervisor is not running, so we need to start it + pass + try: subprocess.run(["supervisord", "-c", self.config_file_path], check=True) + + # Wait for supervisor to be ready after starting + self._wait_for_supervisor_ready() except subprocess.CalledProcessError as e: raise SupervisorError(f"Failed to start supervisor: {str(e)}") except FileNotFoundError: diff --git a/tests/commands/test_up.py b/tests/commands/test_up.py index 1274671b..d66a460b 100644 --- a/tests/commands/test_up.py +++ b/tests/commands/test_up.py @@ -8,21 +8,30 @@ import pytest +from devservices.commands.up import bring_up_supervisor_programs from devservices.commands.up import up +from devservices.configs.service_config import Dependency +from devservices.configs.service_config import ServiceConfig from devservices.constants import CONFIG_FILE_NAME +from devservices.constants import DependencyType from devservices.constants import DEVSERVICES_DIR_NAME from devservices.constants import HEALTHCHECK_TIMEOUT +from devservices.constants import PROGRAMS_CONF_FILE_NAME from devservices.exceptions import ConfigError from devservices.exceptions import ContainerHealthcheckFailedError from devservices.exceptions import DependencyError from devservices.exceptions import DockerComposeError from devservices.exceptions import ServiceNotFoundError +from devservices.exceptions import SupervisorConfigError +from devservices.exceptions import SupervisorError from devservices.utils.docker_compose import DockerComposeCommand +from devservices.utils.services import Service from devservices.utils.state import ServiceRuntime from devservices.utils.state import State from devservices.utils.state import StateTables from testing.utils import create_config_file from testing.utils import create_mock_git_repo +from testing.utils import create_programs_conf_file from testing.utils import run_git_command @@ -2211,3 +2220,313 @@ def test_up_does_not_bring_up_dependency_if_set_to_local_and_mode_does_not_conta "Skipping 'local-runtime-service' as it is set to run locally" not in captured.out.strip() ), "This shouldn't be printed since other-service isn't being brought up in a mode that includes local-runtime-service" + + +@mock.patch("devservices.utils.state.State.remove_service_entry") +@mock.patch("devservices.utils.state.State.update_service_entry") +@mock.patch("devservices.commands.up._create_devservices_network") +@mock.patch("devservices.commands.up.check_all_containers_healthy") +@mock.patch( + "devservices.utils.docker_compose.get_non_remote_services", + return_value={"clickhouse", "redis"}, +) +@mock.patch("devservices.utils.supervisor.SupervisorManager.start_supervisor_daemon") +@mock.patch("devservices.utils.supervisor.SupervisorManager.start_process") +def test_up_supervisor_program( + mock_start_process: mock.Mock, + mock_start_supervisor_daemon: mock.Mock, + mock_get_non_remote_services: mock.Mock, + mock_check_all_containers_healthy: mock.Mock, + mock_create_devservices_network: mock.Mock, + mock_update_service_entry: mock.Mock, + mock_remove_service_entry: mock.Mock, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + with ( + mock.patch( + "devservices.commands.up.DEVSERVICES_DEPENDENCIES_CACHE_DIR", + str(tmp_path / "dependency-dir"), + ), + mock.patch("devservices.utils.state.STATE_DB_FILE", str(tmp_path / "state")), + ): + config = { + "x-sentry-service-config": { + "version": 0.1, + "service_name": "example-service", + "dependencies": { + "supervisor-program": {"description": "Supervisor program"}, + }, + "modes": {"default": ["supervisor-program"]}, + }, + "services": {}, + } + + service_path = tmp_path / "example-service" + create_config_file(service_path, config) + os.chdir(service_path) + + supervisor_program_config = """ +[program:supervisor-program] +command=echo "Hello, world!" +""" + + create_programs_conf_file(service_path, supervisor_program_config) + + args = Namespace( + service_name=None, debug=False, mode="default", exclude_local=False + ) + + up(args) + + mock_create_devservices_network.assert_called_once() + + mock_get_non_remote_services.assert_has_calls( + [ + mock.call( + f"{service_path}/{DEVSERVICES_DIR_NAME}/{CONFIG_FILE_NAME}", + mock.ANY, + ), + mock.call( + f"{service_path}/{DEVSERVICES_DIR_NAME}/{CONFIG_FILE_NAME}", + mock.ANY, + ), + ] + ) + + mock_update_service_entry.assert_has_calls( + [ + mock.call("example-service", "default", StateTables.STARTING_SERVICES), + mock.call("example-service", "default", StateTables.STARTED_SERVICES), + ] + ) + mock_remove_service_entry.assert_called_once_with( + "example-service", StateTables.STARTING_SERVICES + ) + mock_check_all_containers_healthy.assert_called_once() + mock_start_supervisor_daemon.assert_called_once() + mock_start_process.assert_called_once_with("supervisor-program") + captured = capsys.readouterr() + assert "Retrieving dependencies" in captured.out.strip() + assert ( + "Starting dependencies with local runtimes..." not in captured.out.strip() + ), "This shouldn't be printed since we don't have any dependencies with local runtimes" + assert "Starting 'example-service' in mode: 'default'" in captured.out.strip() + assert "Starting supervisor daemon" in captured.out.strip() + assert "Starting supervisor-program" in captured.out.strip() + + +@mock.patch("devservices.utils.state.State.remove_service_entry") +@mock.patch("devservices.utils.state.State.update_service_entry") +@mock.patch("devservices.commands.up._create_devservices_network") +@mock.patch("devservices.commands.up.check_all_containers_healthy") +@mock.patch( + "devservices.utils.docker_compose.get_non_remote_services", + return_value={"clickhouse", "redis"}, +) +@mock.patch( + "devservices.utils.supervisor.SupervisorManager.start_supervisor_daemon", + side_effect=SupervisorError("Error starting supervisor daemon"), +) +@mock.patch("devservices.utils.supervisor.SupervisorManager.start_process") +def test_up_supervisor_program_error( + mock_start_process: mock.Mock, + mock_start_supervisor_daemon: mock.Mock, + mock_get_non_remote_services: mock.Mock, + mock_check_all_containers_healthy: mock.Mock, + mock_create_devservices_network: mock.Mock, + mock_update_service_entry: mock.Mock, + mock_remove_service_entry: mock.Mock, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + with ( + mock.patch( + "devservices.commands.up.DEVSERVICES_DEPENDENCIES_CACHE_DIR", + str(tmp_path / "dependency-dir"), + ), + mock.patch("devservices.utils.state.STATE_DB_FILE", str(tmp_path / "state")), + ): + config = { + "x-sentry-service-config": { + "version": 0.1, + "service_name": "example-service", + "dependencies": { + "supervisor-program": {"description": "Supervisor program"}, + }, + "modes": {"default": ["supervisor-program"]}, + }, + "services": {}, + } + + service_path = tmp_path / "example-service" + create_config_file(service_path, config) + os.chdir(service_path) + + supervisor_program_config = """ +[program:supervisor-program] +command=echo "Hello, world!" +""" + + create_programs_conf_file(service_path, supervisor_program_config) + + args = Namespace( + service_name=None, debug=False, mode="default", exclude_local=False + ) + + with pytest.raises(SystemExit): + up(args) + + mock_create_devservices_network.assert_called_once() + + mock_get_non_remote_services.assert_has_calls( + [ + mock.call( + f"{service_path}/{DEVSERVICES_DIR_NAME}/{CONFIG_FILE_NAME}", + mock.ANY, + ), + mock.call( + f"{service_path}/{DEVSERVICES_DIR_NAME}/{CONFIG_FILE_NAME}", + mock.ANY, + ), + ] + ) + + mock_update_service_entry.assert_has_calls( + [ + mock.call("example-service", "default", StateTables.STARTING_SERVICES), + ] + ) + mock_remove_service_entry.assert_not_called() + mock_check_all_containers_healthy.assert_called_once() + mock_start_supervisor_daemon.assert_called_once() + mock_start_process.assert_not_called() + captured = capsys.readouterr() + assert "Retrieving dependencies" in captured.out.strip() + assert ( + "Starting dependencies with local runtimes..." not in captured.out.strip() + ), "This shouldn't be printed since we don't have any dependencies with local runtimes" + assert "Starting 'example-service' in mode: 'default'" in captured.out.strip() + assert "Starting supervisor daemon" in captured.out.strip() + assert "Error starting supervisor daemon" in captured.out.strip() + + +@mock.patch("devservices.utils.supervisor.SupervisorManager.start_supervisor_daemon") +@mock.patch("devservices.utils.supervisor.SupervisorManager.start_process") +def test_bring_up_supervisor_programs_no_programs_config( + mock_start_process: mock.Mock, + mock_start_supervisor_daemon: mock.Mock, + tmp_path: Path, +) -> None: + service_config = ServiceConfig( + version=0.1, + service_name="test-service", + dependencies={ + "supervisor-program": Dependency( + description="Supervisor program", + dependency_type=DependencyType.SUPERVISOR, + ), + }, + modes={"default": ["supervisor-program"]}, + ) + service = Service( + name="test-service", + repo_path=str(tmp_path), + config=service_config, + ) + + status = mock.MagicMock() + + with pytest.raises( + SupervisorConfigError, + match=f"No programs.conf file found in {tmp_path / DEVSERVICES_DIR_NAME / PROGRAMS_CONF_FILE_NAME}", + ): + bring_up_supervisor_programs(["supervisor-program"], service, status) + + mock_start_supervisor_daemon.assert_not_called() + mock_start_process.assert_not_called() + + +@mock.patch("devservices.utils.supervisor.SupervisorManager.start_supervisor_daemon") +@mock.patch("devservices.utils.supervisor.SupervisorManager.start_process") +def test_bring_up_supervisor_programs_empty_list( + mock_start_process: mock.Mock, + mock_start_supervisor_daemon: mock.Mock, + tmp_path: Path, +) -> None: + service_config = ServiceConfig( + version=0.1, + service_name="test-service", + dependencies={ + "supervisor-program": Dependency( + description="Supervisor program", + dependency_type=DependencyType.SUPERVISOR, + ), + }, + modes={"default": ["supervisor-program"]}, + ) + service = Service( + name="test-service", + repo_path=str(tmp_path), + config=service_config, + ) + + status = mock.MagicMock() + + bring_up_supervisor_programs([], service, status) + + status.info.assert_not_called() + status.failure.assert_not_called() + + mock_start_supervisor_daemon.assert_not_called() + mock_start_process.assert_not_called() + + +@mock.patch("devservices.utils.supervisor.SupervisorManager.start_supervisor_daemon") +@mock.patch("devservices.utils.supervisor.SupervisorManager.start_process") +def test_bring_up_supervisor_programs_success( + mock_start_process: mock.Mock, + mock_start_supervisor_daemon: mock.Mock, + tmp_path: Path, +) -> None: + service_config = ServiceConfig( + version=0.1, + service_name="test-service", + dependencies={ + "supervisor-program": Dependency( + description="Supervisor program", + dependency_type=DependencyType.SUPERVISOR, + ), + }, + modes={"default": ["supervisor-program"]}, + ) + service = Service( + name="test-service", + repo_path=str(tmp_path), + config=service_config, + ) + + programs_conf_path = tmp_path / DEVSERVICES_DIR_NAME / PROGRAMS_CONF_FILE_NAME + + create_programs_conf_file( + programs_conf_path, + """ +[program:supervisor-program] +command=echo "Hello, world!" +""", + ) + + status = mock.MagicMock() + + bring_up_supervisor_programs(["supervisor-program"], service, status) + + status.info.assert_has_calls( + [ + mock.call("Starting supervisor daemon"), + mock.call("Starting supervisor-program"), + ] + ) + status.failure.assert_not_called() + + mock_start_supervisor_daemon.assert_called_once() + mock_start_process.assert_called_once_with("supervisor-program") diff --git a/tests/utils/test_supervisor.py b/tests/utils/test_supervisor.py index b0b61a9e..e0e3e055 100644 --- a/tests/utils/test_supervisor.py +++ b/tests/utils/test_supervisor.py @@ -7,12 +7,14 @@ from unittest import mock import pytest +from freezegun import freeze_time from devservices.constants import DEVSERVICES_DIR_NAME from devservices.exceptions import SupervisorConfigError from devservices.exceptions import SupervisorConnectionError from devservices.exceptions import SupervisorError from devservices.exceptions import SupervisorProcessError +from devservices.utils.supervisor import SupervisorDaemonState from devservices.utils.supervisor import SupervisorManager from devservices.utils.supervisor import SupervisorProcessState from devservices.utils.supervisor import UnixSocketHTTPConnection @@ -158,15 +160,45 @@ def test_is_program_running_failure( @mock.patch("devservices.utils.supervisor.subprocess.run") +@mock.patch("devservices.utils.supervisor.xmlrpc.client.ServerProxy") def test_start_supervisor_daemon_success( - mock_subprocess_run: mock.MagicMock, supervisor_manager: SupervisorManager + mock_rpc_client: mock.MagicMock, + mock_subprocess_run: mock.MagicMock, + supervisor_manager: SupervisorManager, ) -> None: + mock_rpc_client.return_value.supervisor.getState.side_effect = [ + SupervisorConnectionError("Connection refused"), + { + "statecode": SupervisorDaemonState.RUNNING, + "statename": "RUNNING", + }, + ] supervisor_manager.start_supervisor_daemon() mock_subprocess_run.assert_called_once_with( ["supervisord", "-c", supervisor_manager.config_file_path], check=True ) +@mock.patch("devservices.utils.supervisor.subprocess.run") +@mock.patch( + "devservices.utils.supervisor.xmlrpc.client.ServerProxy", + return_value=mock.MagicMock(), +) +def test_start_supervisor_daemon_already_running( + mock_rpc_client: mock.MagicMock, + mock_subprocess_run: mock.MagicMock, + supervisor_manager: SupervisorManager, +) -> None: + mock_rpc_client.return_value.supervisor.getState.return_value = { + "statecode": SupervisorDaemonState.RUNNING, + "statename": "RUNNING", + } + supervisor_manager.start_supervisor_daemon() + assert mock_rpc_client.return_value.supervisor.getState.call_count == 2 + assert mock_rpc_client.return_value.supervisor.restart.call_count == 1 + assert mock_subprocess_run.call_count == 0 + + @mock.patch("devservices.utils.supervisor.subprocess.run") def test_start_supervisor_daemon_subprocess_failure( mock_subprocess_run: mock.MagicMock, supervisor_manager: SupervisorManager @@ -404,3 +436,75 @@ def test_tail_program_logs_keyboard_interrupt( ], check=True, ) + + +@mock.patch("xmlrpc.client.ServerProxy", return_value=mock.MagicMock()) +@mock.patch("devservices.utils.supervisor.time.sleep") +def test_wait_for_supervisor_ready_success( + mock_sleep: mock.MagicMock, + mock_rpc_client: mock.MagicMock, + supervisor_manager: SupervisorManager, +) -> None: + # Mock client that returns a running state + mock_rpc_client.return_value.supervisor.getState.return_value = { + "statename": "RUNNING", + "statecode": SupervisorDaemonState.RUNNING, + } + mock_rpc_client.return_value = supervisor_manager._get_rpc_client() + # Should not raise an exception + supervisor_manager._wait_for_supervisor_ready() + + # Should not have needed to sleep + mock_sleep.assert_not_called() + + +@mock.patch("xmlrpc.client.ServerProxy", return_value=mock.MagicMock()) +@mock.patch("devservices.utils.supervisor.time.sleep") +def test_wait_for_supervisor_ready_retries( + mock_sleep: mock.MagicMock, + mock_rpc_client: mock.MagicMock, + supervisor_manager: SupervisorManager, +) -> None: + # First attempt fails with connection error, second has wrong state, third succeeds + call1 = SupervisorConnectionError("Connection refused") + call2 = {"statename": "RUNNING", "statecode": SupervisorDaemonState.RESTARTING} + call3 = {"statename": "RUNNING", "statecode": SupervisorDaemonState.RUNNING} + + mock_rpc_client.return_value.supervisor.getState.side_effect = [ + call1, + call2, + call3, + ] + + with freeze_time() as frozen_time: + mock_sleep.side_effect = lambda x: frozen_time.tick(x) + supervisor_manager._wait_for_supervisor_ready() + + assert mock_sleep.call_count == 2 + + assert mock_rpc_client.return_value.supervisor.getState.call_count == 3 + + +@mock.patch("xmlrpc.client.ServerProxy") +@mock.patch("devservices.utils.supervisor.time.sleep") +def test_wait_for_supervisor_ready_timeout( + mock_sleep: mock.MagicMock, + mock_rpc_client: mock.MagicMock, + supervisor_manager: SupervisorManager, +) -> None: + mock_rpc_client.return_value.supervisor.getState.side_effect = ( + SupervisorConnectionError("Connection refused") + ) + + with ( + freeze_time() as frozen_time, + pytest.raises( + SupervisorError, match="Supervisor didn't become ready within 5 seconds" + ), + ): + mock_sleep.side_effect = lambda x: frozen_time.tick(x) + supervisor_manager._wait_for_supervisor_ready(5, 1) + + assert mock_sleep.call_count == 5 + + assert mock_rpc_client.return_value.supervisor.getState.call_count == 5 From 879295e2dee8ddefc212f099720be3a828e5c56d Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Thu, 22 May 2025 15:08:30 -0700 Subject: [PATCH 12/20] add supervisor to down --- devservices/commands/down.py | 43 ++++++ tests/commands/test_down.py | 263 +++++++++++++++++++++++++++++++++++ 2 files changed, 306 insertions(+) diff --git a/devservices/commands/down.py b/devservices/commands/down.py index 0836ecb6..163ffed2 100644 --- a/devservices/commands/down.py +++ b/devservices/commands/down.py @@ -15,11 +15,14 @@ from devservices.constants import DEVSERVICES_DEPENDENCIES_CACHE_DIR from devservices.constants import DEVSERVICES_DEPENDENCIES_CACHE_DIR_KEY from devservices.constants import DEVSERVICES_DIR_NAME +from devservices.constants import PROGRAMS_CONF_FILE_NAME from devservices.exceptions import ConfigError from devservices.exceptions import ConfigNotFoundError from devservices.exceptions import DependencyError from devservices.exceptions import DockerComposeError from devservices.exceptions import ServiceNotFoundError +from devservices.exceptions import SupervisorConfigError +from devservices.exceptions import SupervisorError from devservices.utils.console import Console from devservices.utils.console import Status from devservices.utils.dependencies import construct_dependency_graph @@ -35,6 +38,7 @@ from devservices.utils.state import ServiceRuntime from devservices.utils.state import State from devservices.utils.state import StateTables +from devservices.utils.supervisor import SupervisorManager def add_parser(subparsers: _SubParsersAction[ArgumentParser]) -> None: @@ -105,6 +109,14 @@ def down(args: Namespace) -> None: active_mode_dependencies = modes.get(active_mode, []) mode_dependencies.update(active_mode_dependencies) + supervisor_programs = [ + dep + for dep in mode_dependencies + if dep in service.config.dependencies + and service.config.dependencies[dep].dependency_type + == DependencyType.SUPERVISOR + ] + with Status( lambda: console.warning(f"Stopping {service.name}"), ) as status: @@ -129,6 +141,12 @@ def down(args: Namespace) -> None: ) exit(1) + try: + bring_down_supervisor_programs(supervisor_programs, service, status) + except SupervisorError as se: + status.failure(str(se)) + exit(1) + # Check if any service depends on the service we are trying to bring down # TODO: We should also take into account the active modes of the other services (this is not trivial to do) other_started_services = active_services.difference({service.name}) @@ -285,3 +303,28 @@ def _bring_down_dependency( for dependency in cmd.services: status.info(f"Stopping {dependency}") return run_cmd(cmd.full_command, current_env) + + +def bring_down_supervisor_programs( + supervisor_programs: list[str], service: Service, status: Status +) -> None: + if len(supervisor_programs) == 0: + return + programs_config_path = os.path.join( + service.repo_path, f"{DEVSERVICES_DIR_NAME}/{PROGRAMS_CONF_FILE_NAME}" + ) + if not os.path.exists(programs_config_path): + raise SupervisorConfigError( + f"No programs.conf file found in {programs_config_path}." + ) + manager = SupervisorManager( + programs_config_path, + service_name=service.name, + ) + + for program in supervisor_programs: + status.info(f"Stopping {program}") + manager.stop_process(program) + + status.info("Stopping supervisor daemon") + manager.stop_supervisor_daemon() diff --git a/tests/commands/test_down.py b/tests/commands/test_down.py index 125c205a..0abb9374 100644 --- a/tests/commands/test_down.py +++ b/tests/commands/test_down.py @@ -8,6 +8,7 @@ import pytest +from devservices.commands.down import bring_down_supervisor_programs from devservices.commands.down import down from devservices.configs.service_config import Dependency from devservices.configs.service_config import RemoteConfig @@ -15,8 +16,11 @@ from devservices.constants import CONFIG_FILE_NAME from devservices.constants import DependencyType from devservices.constants import DEVSERVICES_DIR_NAME +from devservices.constants import PROGRAMS_CONF_FILE_NAME from devservices.exceptions import ConfigError from devservices.exceptions import ServiceNotFoundError +from devservices.exceptions import SupervisorConfigError +from devservices.exceptions import SupervisorError from devservices.utils.dependencies import install_and_verify_dependencies from devservices.utils.docker_compose import DockerComposeCommand from devservices.utils.services import Service @@ -25,6 +29,7 @@ from devservices.utils.state import StateTables from testing.utils import create_config_file from testing.utils import create_mock_git_repo +from testing.utils import create_programs_conf_file from testing.utils import run_git_command @@ -1334,3 +1339,261 @@ def test_down_shared_and_local_dependencies( mock.call("other-service", StateTables.STARTED_SERVICES), ] ) + + +@mock.patch("devservices.utils.state.State.remove_service_entry") +@mock.patch( + "devservices.utils.supervisor.SupervisorManager.stop_supervisor_daemon", + side_effect=SupervisorError("Error stopping supervisor daemon"), +) +@mock.patch("devservices.utils.supervisor.SupervisorManager.stop_process") +def test_down_supervisor_program_error( + mock_stop_process: mock.Mock, + mock_stop_supervisor_daemon: mock.Mock, + mock_remove_service_entry: mock.Mock, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + with mock.patch( + "devservices.commands.down.DEVSERVICES_DEPENDENCIES_CACHE_DIR", + str(tmp_path / "dependency-dir"), + ): + config = { + "x-sentry-service-config": { + "version": 0.1, + "service_name": "example-service", + "dependencies": { + "supervisor-program": {"description": "Supervisor program"}, + }, + "modes": {"default": ["supervisor-program"]}, + }, + "services": { + "redis": {"image": "redis:6.2.14-alpine"}, + "clickhouse": { + "image": "altinity/clickhouse-server:23.8.11.29.altinitystable" + }, + }, + } + + service_path = tmp_path / "example-service" + create_config_file(service_path, config) + os.chdir(service_path) + + supervisor_program_config = """ +[program:supervisor-program] +command=echo "Hello, world!" +""" + create_programs_conf_file(service_path, supervisor_program_config) + + args = Namespace(service_name=None, debug=False, exclude_local=False) + + with ( + mock.patch( + "devservices.utils.state.STATE_DB_FILE", str(tmp_path / "state") + ), + pytest.raises(SystemExit), + ): + state = State() + state.update_service_entry( + "example-service", "default", StateTables.STARTING_SERVICES + ) + down(args) + + mock_remove_service_entry.assert_not_called() + + captured = capsys.readouterr() + mock_stop_process.assert_called_once_with("supervisor-program") + mock_stop_supervisor_daemon.assert_called_once() + assert "Stopping supervisor-program" in captured.out.strip() + assert "Stopping supervisor daemon" in captured.out.strip() + assert "Error stopping supervisor daemon" in captured.out.strip() + + +@mock.patch("devservices.utils.state.State.remove_service_entry") +@mock.patch("devservices.utils.supervisor.SupervisorManager.stop_supervisor_daemon") +@mock.patch("devservices.utils.supervisor.SupervisorManager.stop_process") +def test_down_supervisor_program_success( + mock_stop_process: mock.Mock, + mock_stop_supervisor_daemon: mock.Mock, + mock_remove_service_entry: mock.Mock, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + with mock.patch( + "devservices.commands.down.DEVSERVICES_DEPENDENCIES_CACHE_DIR", + str(tmp_path / "dependency-dir"), + ): + config = { + "x-sentry-service-config": { + "version": 0.1, + "service_name": "example-service", + "dependencies": { + "supervisor-program": {"description": "Supervisor program"}, + }, + "modes": {"default": ["supervisor-program"]}, + }, + "services": { + "redis": {"image": "redis:6.2.14-alpine"}, + "clickhouse": { + "image": "altinity/clickhouse-server:23.8.11.29.altinitystable" + }, + }, + } + + service_path = tmp_path / "example-service" + create_config_file(service_path, config) + os.chdir(service_path) + + supervisor_program_config = """ +[program:supervisor-program] +command=echo "Hello, world!" +""" + create_programs_conf_file(service_path, supervisor_program_config) + + args = Namespace(service_name=None, debug=False, exclude_local=False) + + with ( + mock.patch( + "devservices.utils.state.STATE_DB_FILE", str(tmp_path / "state") + ), + ): + state = State() + state.update_service_entry( + "example-service", "default", StateTables.STARTING_SERVICES + ) + down(args) + + mock_remove_service_entry.assert_has_calls( + [ + mock.call("example-service", StateTables.STARTING_SERVICES), + mock.call("example-service", StateTables.STARTED_SERVICES), + ] + ) + + captured = capsys.readouterr() + mock_stop_process.assert_called_once_with("supervisor-program") + mock_stop_supervisor_daemon.assert_called_once() + assert "Stopping supervisor-program" in captured.out.strip() + assert "Stopping supervisor daemon" in captured.out.strip() + assert "example-service stopped" in captured.out.strip() + + +@mock.patch("devservices.utils.supervisor.SupervisorManager.stop_supervisor_daemon") +@mock.patch("devservices.utils.supervisor.SupervisorManager.stop_process") +def test_bring_down_supervisor_programs_no_programs_config( + mock_stop_process: mock.Mock, + mock_stop_supervisor_daemon: mock.Mock, + tmp_path: Path, +) -> None: + service_config = ServiceConfig( + version=0.1, + service_name="test-service", + dependencies={ + "supervisor-program": Dependency( + description="Supervisor program", + dependency_type=DependencyType.SUPERVISOR, + ), + }, + modes={"default": ["supervisor-program"]}, + ) + service = Service( + name="test-service", + repo_path=str(tmp_path), + config=service_config, + ) + + status = mock.MagicMock() + + with pytest.raises( + SupervisorConfigError, + match=f"No programs.conf file found in {tmp_path / DEVSERVICES_DIR_NAME / PROGRAMS_CONF_FILE_NAME}", + ): + bring_down_supervisor_programs(["supervisor-program"], service, status) + + mock_stop_supervisor_daemon.assert_not_called() + mock_stop_process.assert_not_called() + + +@mock.patch("devservices.utils.supervisor.SupervisorManager.stop_supervisor_daemon") +@mock.patch("devservices.utils.supervisor.SupervisorManager.stop_process") +def test_bring_down_supervisor_programs_empty_list( + mock_stop_process: mock.Mock, + mock_stop_supervisor_daemon: mock.Mock, + tmp_path: Path, +) -> None: + service_config = ServiceConfig( + version=0.1, + service_name="test-service", + dependencies={ + "supervisor-program": Dependency( + description="Supervisor program", + dependency_type=DependencyType.SUPERVISOR, + ), + }, + modes={"default": ["supervisor-program"]}, + ) + service = Service( + name="test-service", + repo_path=str(tmp_path), + config=service_config, + ) + + status = mock.MagicMock() + + bring_down_supervisor_programs([], service, status) + + status.info.assert_not_called() + status.failure.assert_not_called() + + mock_stop_supervisor_daemon.assert_not_called() + mock_stop_process.assert_not_called() + + +@mock.patch("devservices.utils.supervisor.SupervisorManager.stop_supervisor_daemon") +@mock.patch("devservices.utils.supervisor.SupervisorManager.stop_process") +def test_bring_down_supervisor_programs_success( + mock_stop_process: mock.Mock, + mock_stop_supervisor_daemon: mock.Mock, + tmp_path: Path, +) -> None: + service_config = ServiceConfig( + version=0.1, + service_name="test-service", + dependencies={ + "supervisor-program": Dependency( + description="Supervisor program", + dependency_type=DependencyType.SUPERVISOR, + ), + }, + modes={"default": ["supervisor-program"]}, + ) + service = Service( + name="test-service", + repo_path=str(tmp_path), + config=service_config, + ) + + programs_conf_path = tmp_path / DEVSERVICES_DIR_NAME / PROGRAMS_CONF_FILE_NAME + + create_programs_conf_file( + programs_conf_path, + """ +[program:supervisor-program] +command=echo "Hello, world!" +""", + ) + + status = mock.MagicMock() + + bring_down_supervisor_programs(["supervisor-program"], service, status) + + status.info.assert_has_calls( + [ + mock.call("Stopping supervisor-program"), + mock.call("Stopping supervisor daemon"), + ] + ) + status.failure.assert_not_called() + + mock_stop_supervisor_daemon.assert_called_once() + mock_stop_process.assert_called_once_with("supervisor-program") From 4394c3c125daf4eff7e96b5ce272435511a6e773 Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Tue, 27 May 2025 14:03:40 -0700 Subject: [PATCH 13/20] add case to not allow bringing up supervisor programs from outside the service repo directory --- devservices/commands/up.py | 5 ++++ tests/commands/test_up.py | 60 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/devservices/commands/up.py b/devservices/commands/up.py index f29d224e..46627f5d 100644 --- a/devservices/commands/up.py +++ b/devservices/commands/up.py @@ -311,6 +311,11 @@ def bring_up_supervisor_programs( ) -> None: if len(supervisor_programs) == 0: return + if not os.path.samefile(os.getcwd(), service.repo_path): + status.warning( + f"Cannot bring up supervisor programs from outside the service repository. Please run the command from the service repository ({service.repo_path})" + ) + return programs_config_path = os.path.join( service.repo_path, f"{DEVSERVICES_DIR_NAME}/{PROGRAMS_CONF_FILE_NAME}" ) diff --git a/tests/commands/test_up.py b/tests/commands/test_up.py index 939df892..36bdff32 100644 --- a/tests/commands/test_up.py +++ b/tests/commands/test_up.py @@ -2656,6 +2656,7 @@ def test_bring_up_supervisor_programs_no_programs_config( SupervisorConfigError, match=f"No programs.conf file found in {tmp_path / DEVSERVICES_DIR_NAME / PROGRAMS_CONF_FILE_NAME}", ): + os.chdir(tmp_path) bring_up_supervisor_programs(["supervisor-program"], service, status) mock_start_supervisor_daemon.assert_not_called() @@ -2733,6 +2734,7 @@ def test_bring_up_supervisor_programs_success( status = mock.MagicMock() + os.chdir(tmp_path) bring_up_supervisor_programs(["supervisor-program"], service, status) status.info.assert_has_calls( @@ -2745,3 +2747,61 @@ def test_bring_up_supervisor_programs_success( mock_start_supervisor_daemon.assert_called_once() mock_start_process.assert_called_once_with("supervisor-program") + + +@mock.patch("devservices.utils.supervisor.SupervisorManager.start_supervisor_daemon") +@mock.patch("devservices.utils.supervisor.SupervisorManager.start_process") +def test_bring_up_supervisor_programs_wrong_directory( + mock_start_process: mock.Mock, + mock_start_supervisor_daemon: mock.Mock, + tmp_path: Path, +) -> None: + service_config = ServiceConfig( + version=0.1, + service_name="test-service", + dependencies={ + "supervisor-program": Dependency( + description="Supervisor program", + dependency_type=DependencyType.SUPERVISOR, + ), + }, + modes={"default": ["supervisor-program"]}, + ) + + service_repo_path = tmp_path / "service-repo" + service_repo_path.mkdir() + + service = Service( + name="test-service", + repo_path=str(service_repo_path), + config=service_config, + ) + + programs_conf_path = ( + service_repo_path / DEVSERVICES_DIR_NAME / PROGRAMS_CONF_FILE_NAME + ) + create_programs_conf_file( + programs_conf_path, + """ +[program:supervisor-program] +command=echo "Hello, world!" +""", + ) + + status = mock.MagicMock() + + different_dir = tmp_path / "different-dir" + different_dir.mkdir() + os.chdir(different_dir) + + # Call the function + bring_up_supervisor_programs(["supervisor-program"], service, status) + + status.warning.assert_called_once_with( + f"Cannot bring up supervisor programs from outside the service repository. Please run the command from the service repository ({service_repo_path})" + ) + status.info.assert_not_called() + status.failure.assert_not_called() + + mock_start_supervisor_daemon.assert_not_called() + mock_start_process.assert_not_called() From 8fe28d8dd383e4331a0754bf8c163c1fa3abd79f Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Thu, 29 May 2025 22:02:56 -0700 Subject: [PATCH 14/20] rename and move around function parameters --- devservices/commands/up.py | 4 ++-- devservices/constants.py | 1 + devservices/utils/supervisor.py | 3 ++- tests/commands/test_up.py | 8 ++++---- 4 files changed, 9 insertions(+), 7 deletions(-) diff --git a/devservices/commands/up.py b/devservices/commands/up.py index 46627f5d..a42535e0 100644 --- a/devservices/commands/up.py +++ b/devservices/commands/up.py @@ -190,7 +190,7 @@ def up(args: Namespace, existing_status: Status | None = None) -> None: status.failure(f"Failed to start {service.name}: {dce.stderr}") exit(1) try: - bring_up_supervisor_programs(supervisor_programs, service, status) + bring_up_supervisor_programs(service, supervisor_programs, status) except SupervisorError as se: status.failure(str(se)) exit(1) @@ -307,7 +307,7 @@ def bring_up_docker_compose_services( def bring_up_supervisor_programs( - supervisor_programs: list[str], service: Service, status: Status + service: Service, supervisor_programs: list[str], status: Status ) -> None: if len(supervisor_programs) == 0: return diff --git a/devservices/constants.py b/devservices/constants.py index bdbd7666..91292aba 100644 --- a/devservices/constants.py +++ b/devservices/constants.py @@ -69,3 +69,4 @@ class DependencyType(StrEnum): # Healthcheck timeout set to 2 minutes to account for slow healthchecks HEALTHCHECK_TIMEOUT = 120 HEALTHCHECK_INTERVAL = 5 +SUPERVISOR_TIMEOUT = 10 diff --git a/devservices/utils/supervisor.py b/devservices/utils/supervisor.py index 579929d8..0bc4539c 100644 --- a/devservices/utils/supervisor.py +++ b/devservices/utils/supervisor.py @@ -12,6 +12,7 @@ from supervisor.options import ServerOptions from devservices.constants import DEVSERVICES_SUPERVISOR_CONFIG_DIR +from devservices.constants import SUPERVISOR_TIMEOUT from devservices.exceptions import SupervisorConfigError from devservices.exceptions import SupervisorConnectionError from devservices.exceptions import SupervisorError @@ -152,7 +153,7 @@ def _is_program_running(self, program_name: str) -> bool: return False def _wait_for_supervisor_ready( - self, timeout: int = 10, interval: float = 0.5 + self, timeout: int = SUPERVISOR_TIMEOUT, interval: float = 0.5 ) -> None: for _ in range(int(timeout / interval)): try: diff --git a/tests/commands/test_up.py b/tests/commands/test_up.py index 36bdff32..9d020769 100644 --- a/tests/commands/test_up.py +++ b/tests/commands/test_up.py @@ -2657,7 +2657,7 @@ def test_bring_up_supervisor_programs_no_programs_config( match=f"No programs.conf file found in {tmp_path / DEVSERVICES_DIR_NAME / PROGRAMS_CONF_FILE_NAME}", ): os.chdir(tmp_path) - bring_up_supervisor_programs(["supervisor-program"], service, status) + bring_up_supervisor_programs(service, ["supervisor-program"], status) mock_start_supervisor_daemon.assert_not_called() mock_start_process.assert_not_called() @@ -2689,7 +2689,7 @@ def test_bring_up_supervisor_programs_empty_list( status = mock.MagicMock() - bring_up_supervisor_programs([], service, status) + bring_up_supervisor_programs(service, [], status) status.info.assert_not_called() status.failure.assert_not_called() @@ -2735,7 +2735,7 @@ def test_bring_up_supervisor_programs_success( status = mock.MagicMock() os.chdir(tmp_path) - bring_up_supervisor_programs(["supervisor-program"], service, status) + bring_up_supervisor_programs(service, ["supervisor-program"], status) status.info.assert_has_calls( [ @@ -2795,7 +2795,7 @@ def test_bring_up_supervisor_programs_wrong_directory( os.chdir(different_dir) # Call the function - bring_up_supervisor_programs(["supervisor-program"], service, status) + bring_up_supervisor_programs(service, ["supervisor-program"], status) status.warning.assert_called_once_with( f"Cannot bring up supervisor programs from outside the service repository. Please run the command from the service repository ({service_repo_path})" From 6e1eb2c7616f621e1c8e02b3f44b5334bb4749b7 Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Fri, 30 May 2025 14:58:07 -0700 Subject: [PATCH 15/20] address some comments --- devservices/commands/up.py | 1 + devservices/utils/supervisor.py | 7 ++++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/devservices/commands/up.py b/devservices/commands/up.py index 61d90889..1ed6d25f 100644 --- a/devservices/commands/up.py +++ b/devservices/commands/up.py @@ -216,6 +216,7 @@ def up(args: Namespace, existing_status: Status | None = None) -> None: try: bring_up_supervisor_programs(service, supervisor_programs, status) except SupervisorError as se: + capture_exception(se, level="info") status.failure(str(se)) exit(1) diff --git a/devservices/utils/supervisor.py b/devservices/utils/supervisor.py index 0bc4539c..5e2a0317 100644 --- a/devservices/utils/supervisor.py +++ b/devservices/utils/supervisor.py @@ -9,6 +9,7 @@ import xmlrpc.client from enum import IntEnum +from sentry_sdk import capture_exception from supervisor.options import ServerOptions from devservices.constants import DEVSERVICES_SUPERVISOR_CONFIG_DIR @@ -155,7 +156,8 @@ def _is_program_running(self, program_name: str) -> bool: def _wait_for_supervisor_ready( self, timeout: int = SUPERVISOR_TIMEOUT, interval: float = 0.5 ) -> None: - for _ in range(int(timeout / interval)): + start_time = time.time() + while time.time() - start_time < timeout: try: client = self._get_rpc_client() state = client.supervisor.getState() @@ -188,6 +190,9 @@ def start_supervisor_daemon(self) -> None: # Wait for supervisor to be ready after restart self._wait_for_supervisor_ready() return + except xmlrpc.client.Fault as e: + capture_exception(e, level="info") + pass except (SupervisorConnectionError, socket.error, ConnectionRefusedError): # Supervisor is not running, so we need to start it pass From b4d992ded5396fa282a7cf16a7930b7c8f7d24ba Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Fri, 30 May 2025 14:59:19 -0700 Subject: [PATCH 16/20] get rid of duplicate check for programs conf file --- devservices/commands/up.py | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/devservices/commands/up.py b/devservices/commands/up.py index 1ed6d25f..a803f5f7 100644 --- a/devservices/commands/up.py +++ b/devservices/commands/up.py @@ -25,7 +25,6 @@ from devservices.exceptions import DockerComposeError from devservices.exceptions import ModeDoesNotExistError from devservices.exceptions import ServiceNotFoundError -from devservices.exceptions import SupervisorConfigError from devservices.exceptions import SupervisorError from devservices.utils.console import Console from devservices.utils.console import Status @@ -395,10 +394,7 @@ def bring_up_supervisor_programs( programs_config_path = os.path.join( service.repo_path, f"{DEVSERVICES_DIR_NAME}/{PROGRAMS_CONF_FILE_NAME}" ) - if not os.path.exists(programs_config_path): - raise SupervisorConfigError( - f"No programs.conf file found in {programs_config_path}." - ) + manager = SupervisorManager( programs_config_path, service_name=service.name, From 9ca79248c87e34a4d3e917602493494793ac3d78 Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Fri, 30 May 2025 15:02:07 -0700 Subject: [PATCH 17/20] fix tests --- tests/commands/test_up.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/commands/test_up.py b/tests/commands/test_up.py index 8ed539ae..126f82a1 100644 --- a/tests/commands/test_up.py +++ b/tests/commands/test_up.py @@ -2654,7 +2654,7 @@ def test_bring_up_supervisor_programs_no_programs_config( with pytest.raises( SupervisorConfigError, - match=f"No programs.conf file found in {tmp_path / DEVSERVICES_DIR_NAME / PROGRAMS_CONF_FILE_NAME}", + match=f"Config file {tmp_path / DEVSERVICES_DIR_NAME / PROGRAMS_CONF_FILE_NAME} does not exist", ): os.chdir(tmp_path) bring_up_supervisor_programs(service, ["supervisor-program"], status) From 295119514b4854ea75dbe6d18f0b740082735179 Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Fri, 30 May 2025 15:14:34 -0700 Subject: [PATCH 18/20] indent span for starting supervisor programs correctly --- devservices/commands/up.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/devservices/commands/up.py b/devservices/commands/up.py index a803f5f7..98554ef0 100644 --- a/devservices/commands/up.py +++ b/devservices/commands/up.py @@ -212,12 +212,12 @@ def up(args: Namespace, existing_status: Status | None = None) -> None: ) as span: span.set_data("service_name", service.name) span.set_data("supervisor_programs", supervisor_programs) - try: - bring_up_supervisor_programs(service, supervisor_programs, status) - except SupervisorError as se: - capture_exception(se, level="info") - status.failure(str(se)) - exit(1) + try: + bring_up_supervisor_programs(service, supervisor_programs, status) + except SupervisorError as se: + capture_exception(se, level="info") + status.failure(str(se)) + exit(1) state.remove_service_entry(service.name, StateTables.STARTING_SERVICES) state.update_service_entry(service.name, mode, StateTables.STARTED_SERVICES) From 4cf89e9376c9699af7f668b6883687c397d0009d Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Fri, 30 May 2025 15:18:56 -0700 Subject: [PATCH 19/20] remove redundant code --- devservices/commands/down.py | 5 ----- tests/commands/test_down.py | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/devservices/commands/down.py b/devservices/commands/down.py index 163ffed2..c3b3ec48 100644 --- a/devservices/commands/down.py +++ b/devservices/commands/down.py @@ -21,7 +21,6 @@ from devservices.exceptions import DependencyError from devservices.exceptions import DockerComposeError from devservices.exceptions import ServiceNotFoundError -from devservices.exceptions import SupervisorConfigError from devservices.exceptions import SupervisorError from devservices.utils.console import Console from devservices.utils.console import Status @@ -313,10 +312,6 @@ def bring_down_supervisor_programs( programs_config_path = os.path.join( service.repo_path, f"{DEVSERVICES_DIR_NAME}/{PROGRAMS_CONF_FILE_NAME}" ) - if not os.path.exists(programs_config_path): - raise SupervisorConfigError( - f"No programs.conf file found in {programs_config_path}." - ) manager = SupervisorManager( programs_config_path, service_name=service.name, diff --git a/tests/commands/test_down.py b/tests/commands/test_down.py index 0abb9374..61c6f5eb 100644 --- a/tests/commands/test_down.py +++ b/tests/commands/test_down.py @@ -1506,7 +1506,7 @@ def test_bring_down_supervisor_programs_no_programs_config( with pytest.raises( SupervisorConfigError, - match=f"No programs.conf file found in {tmp_path / DEVSERVICES_DIR_NAME / PROGRAMS_CONF_FILE_NAME}", + match=f"Config file {tmp_path / DEVSERVICES_DIR_NAME / PROGRAMS_CONF_FILE_NAME} does not exist", ): bring_down_supervisor_programs(["supervisor-program"], service, status) From e60c7cc6fcdc0eeff7f8ebb2d1b403eec0155be4 Mon Sep 17 00:00:00 2001 From: Hubert Deng Date: Mon, 2 Jun 2025 10:01:36 -0700 Subject: [PATCH 20/20] capture exception for supervisor failures --- devservices/commands/down.py | 1 + 1 file changed, 1 insertion(+) diff --git a/devservices/commands/down.py b/devservices/commands/down.py index c3b3ec48..039c788c 100644 --- a/devservices/commands/down.py +++ b/devservices/commands/down.py @@ -143,6 +143,7 @@ def down(args: Namespace) -> None: try: bring_down_supervisor_programs(supervisor_programs, service, status) except SupervisorError as se: + capture_exception(se) status.failure(str(se)) exit(1)