Skip to content

Commit

Permalink
Catch unexpected errors reading config file
Browse files Browse the repository at this point in the history
When reading the `__appsignal__.py` configuration file from the
demo command, handle unexpected errors in the file (e.g. syntax
errors) instead of only handling the expected error of the
`appsignal` variable being missing.

Unify the handling of errors when reading a config file. Provide
an `ExitError` that can be raised by command helpers to trigger
the command to exit.

Since loading the configuration file is only done by the diagnose
and demo commands, move that functionality out of the `Client` file
and into a helper for the CLI commands.
  • Loading branch information
unflxw committed Mar 5, 2024
1 parent 6a44893 commit ea8173b
Show file tree
Hide file tree
Showing 10 changed files with 77 additions and 53 deletions.
3 changes: 3 additions & 0 deletions src/appsignal/cli/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from .command import AppsignalCLICommand
from .demo import DemoCommand
from .diagnose import DiagnoseCommand
from .exit_error import ExitError
from .install import InstallCommand
from .version import VersionCommand

Expand Down Expand Up @@ -36,6 +37,8 @@ def main(argv: list[str]) -> int:
cmd = cmd_class(args=args)
try:
return cmd.run()
except ExitError as error:
return error.exit_code
except KeyboardInterrupt:
return 0

Expand Down
12 changes: 12 additions & 0 deletions src/appsignal/cli/command.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@
from dataclasses import dataclass
from functools import cached_property

from ..client import Client
from .config import _client_from_config_file
from .exit_error import ExitError


@dataclass(frozen=True)
class AppsignalCLICommand(ABC):
Expand Down Expand Up @@ -66,3 +70,11 @@ def _environment(self) -> str | None:
"Please enter the application environment (development/production): "
)
return environment

def _client_from_config_file(self) -> Client | None:
try:
return _client_from_config_file()
except Exception as error:
print(f"Error loading the __appsignal__.py configuration file:\n{error}\n")
print("Exiting.")
raise ExitError(1) from error
41 changes: 41 additions & 0 deletions src/appsignal/cli/config.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
from __future__ import annotations

import os
from runpy import run_path

from ..client import Client


# Load the AppSignal client from the app specific `__appsignal__.py` client
# file. This loads the user config, rather than our default config.
# If no client file is found it return `None`.
# If there's a problem with the client file it will raise an
# `InvalidClientFileError` with a message containing more details.
def _client_from_config_file() -> Client | None:
cwd = os.getcwd()
app_config_path = os.path.join(cwd, "__appsignal__.py")
if os.path.exists(app_config_path):
try:
client = run_path(app_config_path)["appsignal"]
if not isinstance(client, Client):
raise InvalidClientFileError(
"The `appsignal` variable does not contain an AppSignal client. "
"Please define the configuration file as described in "
"our documentation: "
"https://docs.appsignal.com/python/configuration.html"
)

return client
except KeyError as error:
raise InvalidClientFileError(
"No `appsignal` variable found. "
"Please define the configuration file as described in "
"our documentation: "
"https://docs.appsignal.com/python/configuration.html"
) from error

return None


class InvalidClientFileError(Exception):
pass
11 changes: 3 additions & 8 deletions src/appsignal/cli/demo.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,13 @@

from opentelemetry import trace

from ..client import Client, InvalidClientFileError
from ..client import Client
from ..tracing import set_category, set_error, set_params, set_tag
from .command import AppsignalCLICommand


class DemoCommand(AppsignalCLICommand):
"""Run demo application."""
"""Send demonstration data to AppSignal."""

@staticmethod
def init_parser(parser: ArgumentParser) -> None:
Expand All @@ -20,12 +20,7 @@ def init_parser(parser: ArgumentParser) -> None:
AppsignalCLICommand._push_api_key_argument(parser)

def run(self) -> int:
try:
client = Client._load__appsignal__file()
except InvalidClientFileError as error:
print(f"Error: {error}")
print("Exiting.")
return 1
client = self._client_from_config_file()

if client:
# For demo CLI purposes, AppSignal is always active
Expand Down
14 changes: 7 additions & 7 deletions src/appsignal/cli/diagnose.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@
import urllib
from argparse import ArgumentParser
from pathlib import Path
from sys import stderr
from typing import Any

import requests

from ..__about__ import __version__
from ..agent import Agent
from ..client import Client, InvalidClientFileError
from ..config import Config
from ..push_api_key_validator import PushApiKeyValidator
from .command import AppsignalCLICommand
Expand Down Expand Up @@ -172,16 +172,16 @@ def run(self) -> int:
print("Error: Cannot use --send-report and --no-send-report together.")
return 1

try:
client = Client._load__appsignal__file()
except InvalidClientFileError as error:
print(f"Error: {error}")
print("Exiting.")
return 1
client = self._client_from_config_file()

if client:
self.config = client._config
else:
print(
"Could not load the configuration from the `__appsignal__.py` "
"configuration file. Some configuration options may be missing.",
file=stderr,
)
self.config = Config()

agent = Agent()
Expand Down
8 changes: 8 additions & 0 deletions src/appsignal/cli/exit_error.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
from __future__ import annotations


class ExitError(Exception):
exit_code: int

def __init__(self, exit_code: int) -> None:
self.exit_code = exit_code
29 changes: 0 additions & 29 deletions src/appsignal/client.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
from __future__ import annotations

import logging
import os
import sys
from logging import DEBUG, ERROR, INFO, WARNING, Logger
from runpy import run_path
from typing import TYPE_CHECKING, ClassVar

from .agent import agent
Expand All @@ -28,29 +26,6 @@ class Client:
"trace": DEBUG,
}

# Load the AppSignal client from the app specific `__appsignal__.py` client
# file. This loads the user config, rather than our default config.
# If no client file is found it return `None`.
# If there's a problem with the client file it will raise an
# `InvalidClientFileError` with a message containing more details.
@staticmethod
def _load__appsignal__file() -> Client | None:
cwd = os.getcwd()
app_config_path = os.path.join(cwd, "__appsignal__.py")
if os.path.exists(app_config_path):
try:
return run_path(app_config_path)["appsignal"]
except KeyError as error:
raise InvalidClientFileError(
"No `appsignal` variable was exported by the "
"__appsignal__.py config file. "
"Please update the __appsignal__.py file as described in "
"our documentation: "
"https://docs.appsignal.com/python/configuration.html"
) from error

return None

def __init__(self, **options: Unpack[Options]) -> None:
self._config = Config(options)
self.start_logger()
Expand Down Expand Up @@ -95,7 +70,3 @@ def _start_stdout_logger(self) -> None:
)
)
self._logger.addHandler(handler)


class InvalidClientFileError(Exception):
pass
5 changes: 1 addition & 4 deletions tests/cli/test_demo.py
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,4 @@ def test_demo_with_invalid_config_file(request, mocker, capfd):
os.chdir(request.config.invocation_params.dir)

out, err = capfd.readouterr()
assert (
"No `appsignal` variable was exported by the __appsignal__.py config file."
in out
)
assert "No `appsignal` variable found" in out
5 changes: 1 addition & 4 deletions tests/cli/test_diagnose.py
Original file line number Diff line number Diff line change
Expand Up @@ -115,10 +115,7 @@ def test_diagnose_with_invalid_config_file(request, mocker, capfd):
os.chdir(request.config.invocation_params.dir)

out, err = capfd.readouterr()
assert (
"No `appsignal` variable was exported by the __appsignal__.py config file."
in out
)
assert "No `appsignal` variable found" in out


def test_diagnose_with_missing_paths(mocker, capfd):
Expand Down
2 changes: 1 addition & 1 deletion tests/diagnose

0 comments on commit ea8173b

Please sign in to comment.