Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion devservices/commands/down.py
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,9 @@ def down(args: Namespace) -> None:
console = Console()
service_name = args.service_name
try:
service = find_matching_service(service_name)
service = find_matching_service(
service_name, config_path=getattr(args, "config", None)
)
except ConfigNotFoundError as e:
capture_exception(e, level="info")
console.failure(
Expand Down
2 changes: 1 addition & 1 deletion devservices/commands/foreground.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def foreground(args: Namespace) -> None:
console = Console()
program_name = args.program_name
try:
service = find_matching_service()
service = find_matching_service(config_path=getattr(args, "config", None))
except ConfigNotFoundError as e:
capture_exception(e, level="info")
console.failure(
Expand Down
4 changes: 3 additions & 1 deletion devservices/commands/list_dependencies.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ def list_dependencies(args: Namespace) -> None:
service_name = args.service_name

try:
service = find_matching_service(service_name)
service = find_matching_service(
service_name, config_path=getattr(args, "config", None)
)
except ConfigNotFoundError as e:
capture_exception(e, level="info")
console.failure(
Expand Down
4 changes: 3 additions & 1 deletion devservices/commands/logs.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,9 @@ def logs(args: Namespace) -> None:
console = Console()
service_name = args.service_name
try:
service = find_matching_service(service_name)
service = find_matching_service(
service_name, config_path=getattr(args, "config", None)
)
except ConfigNotFoundError as e:
capture_exception(e, level="info")
console.failure(
Expand Down
2 changes: 1 addition & 1 deletion devservices/commands/serve.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ def serve(args: Namespace) -> None:
console = Console()

try:
service = find_matching_service()
service = find_matching_service(config_path=getattr(args, "config", None))
except ConfigNotFoundError as e:
console.failure(
f"{str(e)}. Please run the command from a directory with a valid devservices configuration."
Expand Down
4 changes: 3 additions & 1 deletion devservices/commands/status.py
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,9 @@ def status(args: Namespace) -> None:
console = Console()
service_name = args.service_name
try:
service = find_matching_service(service_name)
service = find_matching_service(
service_name, config_path=getattr(args, "config", None)
)
except ConfigNotFoundError as e:
capture_exception(e)
console.failure(
Expand Down
4 changes: 3 additions & 1 deletion devservices/commands/toggle.py
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,9 @@ def toggle(args: Namespace) -> None:
console = Console()
service_name = args.service_name
try:
service = find_matching_service(service_name)
service = find_matching_service(
service_name, config_path=getattr(args, "config", None)
)
except ConfigNotFoundError as e:
capture_exception(e, level="info")
console.failure(
Expand Down
4 changes: 3 additions & 1 deletion devservices/commands/up.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,9 @@ def up(args: Namespace, existing_status: Status | None = None) -> None:
console = Console()
service_name = args.service_name
try:
service = find_matching_service(service_name)
service = find_matching_service(
service_name, config_path=getattr(args, "config", None)
)
except ConfigNotFoundError as e:
capture_exception(e, level="info")
console.failure(
Expand Down
7 changes: 5 additions & 2 deletions devservices/configs/service_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,11 @@ def _validate(self) -> None:
)


def load_service_config_from_file(repo_path: str) -> ServiceConfig:
config_path = os.path.join(repo_path, DEVSERVICES_DIR_NAME, CONFIG_FILE_NAME)
def load_service_config_from_file(
repo_path: str, config_path: str | None = None
) -> ServiceConfig:
if config_path is None:
config_path = os.path.join(repo_path, DEVSERVICES_DIR_NAME, CONFIG_FILE_NAME)
if not os.path.exists(config_path):
raise ConfigNotFoundError(
f"No devservices configuration found in {config_path}"
Expand Down
6 changes: 6 additions & 0 deletions devservices/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,6 +142,12 @@ def main() -> None:
usage="devservices [-h] [--version] COMMAND ...",
)
parser.add_argument("--version", action="version", version=current_version)
parser.add_argument(
"-c",
"--config",
help="Path to a custom devservices config file",
default=None,
)

subparsers = parser.add_subparsers(dest="command", title="commands", metavar="")

Expand Down
59 changes: 33 additions & 26 deletions devservices/utils/services.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from sentry_sdk import logger as sentry_logger

from devservices.configs.service_config import ServiceConfig
from devservices.configs.service_config import load_service_config_from_file
from devservices.exceptions import ConfigNotFoundError
from devservices.exceptions import ConfigParseError
from devservices.exceptions import ConfigValidationError
Expand All @@ -25,8 +26,6 @@ class Service:

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 = []
Expand All @@ -51,32 +50,40 @@ def get_local_services(coderoot: str) -> list[Service]:
return services


def find_matching_service(service_name: str | None = None) -> Service:
def find_matching_service(
service_name: str | None = None, config_path: str | None = None
) -> Service:
"""Find a service with the given name."""
if service_name is None:
from devservices.configs.service_config import load_service_config_from_file

repo_path = os.getcwd()
service_config = load_service_config_from_file(repo_path)

return Service(
name=service_config.service_name,
repo_path=repo_path,
config=service_config,
if config_path is not None and service_name is not None:
raise ConfigValidationError(
"Cannot specify both a service name and a custom config path"
)
coderoot = get_coderoot()
services = get_local_services(coderoot)
for service in services:
if service.name.lower() == service_name.lower():
return service
unique_service_names = sorted(set(service.name for service in services))
error_message = f"Service '{service_name}' not found."
if len(unique_service_names) > 0:
service_bullet_points = "\n".join(
[f"- {service_name}" for service_name in unique_service_names]
)
error_message += "\nSupported services:\n" + service_bullet_points
raise ServiceNotFoundError(error_message)
if config_path is not None:
config_path = os.path.abspath(config_path)
repo_path = os.getcwd()
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: When using the --config flag, repo_path is incorrectly set to the current working directory instead of being derived from the provided config_path, causing subsequent commands to fail.
Severity: CRITICAL

Suggested Fix

Modify the logic to derive repo_path from the absolute path of the config_path provided. The path should be the grandparent directory if the config is inside a devservices/ directory, or the parent directory otherwise, as stated in the pull request's description.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: devservices/utils/services.py#L63

Potential issue: When a user specifies a custom configuration file using the `--config`
flag, the `repo_path` variable is incorrectly set to the current working directory
(`os.getcwd()`) instead of being derived from the provided `config_path`. Downstream
commands such as `up`, `down`, and `status` rely on `repo_path` to reconstruct the path
to the configuration file. If the command is run from a directory different from the one
containing the config file, this leads to an incorrect path, causing the command to fail
with an error like `ConfigNotFoundError`. This breaks the feature's primary use case of
pointing to configurations in non-standard locations.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

repo_path uses cwd instead of config file location

High Severity

When config_path is provided, repo_path is set to os.getcwd() instead of being derived from the config file's location. The PR description explicitly states repo_path should be the grandparent directory if the config is inside a devservices/ directory, or the parent directory otherwise. All downstream commands reconstruct the config file path as os.path.join(service.repo_path, DEVSERVICES_DIR_NAME, CONFIG_FILE_NAME), so docker-compose, supervisor, and log operations will reference the wrong config file — not the one specified by --config.

Additional Locations (2)
Fix in Cursor Fix in Web

elif service_name is None:
repo_path = os.getcwd()
else:
coderoot = get_coderoot()
services = get_local_services(coderoot)
for service in services:
if service.name.lower() == service_name.lower():
return service
unique_service_names = sorted(set(service.name for service in services))
error_message = f"Service '{service_name}' not found."
if len(unique_service_names) > 0:
service_bullet_points = "\n".join(
[f"- {service_name}" for service_name in unique_service_names]
)
error_message += "\nSupported services:\n" + service_bullet_points
raise ServiceNotFoundError(error_message)

service_config = load_service_config_from_file(repo_path, config_path=config_path)
return Service(
name=service_config.service_name,
repo_path=repo_path,
config=service_config,
)


def get_active_service_names(clean_stale_entries: bool = False) -> set[str]:
Expand Down
8 changes: 6 additions & 2 deletions tests/commands/test_down.py
Original file line number Diff line number Diff line change
Expand Up @@ -391,7 +391,9 @@ def test_down_config_error(
with pytest.raises(SystemExit):
down(args)

find_matching_service_mock.assert_called_once_with("example-service")
find_matching_service_mock.assert_called_once_with(
"example-service", config_path=None
)
captured = capsys.readouterr()
assert "Config error" in captured.out.strip()

Expand All @@ -406,7 +408,9 @@ def test_down_service_not_found_error(
with pytest.raises(SystemExit):
down(args)

find_matching_service_mock.assert_called_once_with("example-service")
find_matching_service_mock.assert_called_once_with(
"example-service", config_path=None
)
captured = capsys.readouterr()
assert "Service not found" in captured.out.strip()

Expand Down
10 changes: 6 additions & 4 deletions tests/commands/test_list_dependencies.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,9 @@ def test_list_dependencies_service_not_found(

assert exc_info.value.code == 1

mock_find_matching_service.assert_called_once_with("nonexistent-service")
mock_find_matching_service.assert_called_once_with(
"nonexistent-service", config_path=None
)
captured = capsys.readouterr()
assert "Service nonexistent-service not found" in captured.out

Expand All @@ -71,7 +73,7 @@ def test_list_dependencies_config_error(

assert exc_info.value.code == 1

mock_find_matching_service.assert_called_once_with("test-service")
mock_find_matching_service.assert_called_once_with("test-service", config_path=None)
captured = capsys.readouterr()
assert "Version is required in service config" in captured.out

Expand All @@ -97,7 +99,7 @@ def test_list_dependencies_no_dependencies(

list_dependencies(args)

mock_find_matching_service.assert_called_once_with("test-service")
mock_find_matching_service.assert_called_once_with("test-service", config_path=None)
captured = capsys.readouterr()
assert "No dependencies found for test-service" in captured.out

Expand Down Expand Up @@ -130,7 +132,7 @@ def test_list_dependencies_with_dependencies(

list_dependencies(args)

mock_find_matching_service.assert_called_once_with("test-service")
mock_find_matching_service.assert_called_once_with("test-service", config_path=None)
captured = capsys.readouterr()
assert "Dependencies of test-service:" in captured.out
assert "- redis: Redis" in captured.out
Expand Down
6 changes: 4 additions & 2 deletions tests/commands/test_status.py
Original file line number Diff line number Diff line change
Expand Up @@ -729,7 +729,9 @@ def test_status_service_not_found(

assert exc_info.value.code == 1

mock_find_matching_service.assert_called_once_with("nonexistent-service")
mock_find_matching_service.assert_called_once_with(
"nonexistent-service", config_path=None
)
mock_install_and_verify_dependencies.assert_not_called()
mock_get_status_for_service.assert_not_called()

Expand Down Expand Up @@ -860,7 +862,7 @@ def test_status_service_not_running(

status(args)

mock_find_matching_service.assert_called_once_with("test-service")
mock_find_matching_service.assert_called_once_with("test-service", config_path=None)
mock_get_status_for_service.assert_not_called()

captured = capsys.readouterr()
Expand Down
6 changes: 3 additions & 3 deletions tests/commands/test_toggle.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ def test_toggle_config_not_found(
)
)

mock_find_matching_service.assert_called_once_with(None)
mock_find_matching_service.assert_called_once_with(None, config_path=None)
captured = capsys.readouterr()
assert "Config not found" in captured.out.strip()

Expand All @@ -74,7 +74,7 @@ def test_toggle_config_error(
)
)

mock_find_matching_service.assert_called_once_with(None)
mock_find_matching_service.assert_called_once_with(None, config_path=None)
captured = capsys.readouterr()
assert "Config parse error" in captured.out.strip()

Expand All @@ -93,7 +93,7 @@ def test_toggle_service_not_found(
)
)

mock_find_matching_service.assert_called_once_with(None)
mock_find_matching_service.assert_called_once_with(None, config_path=None)
captured = capsys.readouterr()
assert "Service not found" in captured.out.strip()

Expand Down
8 changes: 6 additions & 2 deletions tests/commands/test_up.py
Original file line number Diff line number Diff line change
Expand Up @@ -1407,7 +1407,9 @@ def test_up_config_error(
with pytest.raises(SystemExit):
up(args)

find_matching_service_mock.assert_called_once_with("example-service")
find_matching_service_mock.assert_called_once_with(
"example-service", config_path=None
)
mock_check_all_containers_healthy.assert_not_called()
captured = capsys.readouterr()
assert "Config error" in captured.out.strip()
Expand All @@ -1428,7 +1430,9 @@ def test_up_service_not_found_error(
with pytest.raises(SystemExit):
up(args)

find_matching_service_mock.assert_called_once_with("example-service")
find_matching_service_mock.assert_called_once_with(
"example-service", config_path=None
)
mock_check_all_containers_healthy.assert_not_called()
captured = capsys.readouterr()
assert "Service not found" in captured.out.strip()
Expand Down
26 changes: 26 additions & 0 deletions tests/utils/test_services.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,32 @@ def test_get_local_services_skips_non_devservices_repos(tmp_path: Path) -> None:
assert local_services[0].repo_path == str(mock_basic_repo_path)


def test_find_matching_service_with_config_path(tmp_path: Path) -> None:
"""Test config_path loads from the specified file with repo_path as cwd."""
devservices_dir = tmp_path / "devservices"
devservices_dir.mkdir()
config_file = devservices_dir / "config.yml"
config_file.write_text(
"x-sentry-service-config:\n"
" version: 0.1\n"
" service_name: my-service\n"
" dependencies: {}\n"
" modes:\n"
" default: []\n"
)
service = find_matching_service(config_path=str(config_file))
assert service.name == "my-service"
assert service.repo_path == os.getcwd()


def test_find_matching_service_with_config_path_not_found() -> None:
"""Test config_path with a nonexistent file."""
from devservices.exceptions import ConfigNotFoundError

with pytest.raises(ConfigNotFoundError):
find_matching_service(config_path="/nonexistent/path/config.yml")


@mock.patch(
"devservices.utils.services.get_local_services",
return_value=[],
Expand Down
Loading