Skip to content

Commit

Permalink
fix: Improved error message clarity (#38)
Browse files Browse the repository at this point in the history
# Motivation

The error handler would capture all error messages and obfuscate them.

# Description

Rewrote the error handler so that it prints out the error details in a
neater way (using click/rich_click) and such that the debug option
allows you to view said error

# Testing

Generated different gRPC errors and observed the error messages
generated.

# Impact

None.

# Additional Information

None.

# Checklist

- [x] My code adheres to the coding and style guidelines of the project.
- [x] I have performed a self-review of my code.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have made corresponding changes to the documentation.
- [x] I have thoroughly tested my modifications and added tests when
necessary.
- [x] Tests pass locally and in the CI.
- [x] I have assessed the performance impact of my modifications.
  • Loading branch information
qdelamea-aneo authored Jan 30, 2025
2 parents 9f21556 + cc6efe7 commit 4b0f9e7
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 26 deletions.
3 changes: 1 addition & 2 deletions src/armonik_cli/commands/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@

from armonik_cli.core import console, base_command, base_group
from armonik_cli.core.params import KeyValuePairParam, TimeDeltaParam, FilterParam, FieldParam
from armonik_cli.exceptions import InternalError

TASKS_TABLE_COLS = [("ID", "Id"), ("Status", "Status"), ("CreatedAt", "CreatedAt")]

Expand Down Expand Up @@ -238,7 +237,7 @@ def tasks_create(
"red",
)
)
raise InternalError(
raise click.MissingParameter(
"If you want to pass in additional task options please provide all three (max duration, priority, max retries)"
)
task_definition = TaskDefinition(
Expand Down
40 changes: 25 additions & 15 deletions src/armonik_cli/core/decorators.py
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
import grpc
import rich_click as click

from functools import wraps, partial
from typing import Callable, Optional, Any

from armonik_cli.core.console import console
from armonik_cli.core.common import global_cluster_config_options, global_common_options
from armonik_cli.exceptions import NotFoundError, InternalError, InternalArmoniKError
from armonik_cli.exceptions import (
InternalCliError,
InternalArmoniKError,
)


def error_handler(func: Optional[Callable[..., Any]] = None) -> Callable[..., Any]:
Expand All @@ -24,28 +26,36 @@ def error_handler(func: Optional[Callable[..., Any]] = None) -> Callable[..., An
return partial(error_handler)

@wraps(func)
def wrapper(*args: Any, **kwargs: Any) -> Any:
def wrapper(*args, **kwargs):
debug_mode = kwargs.get("debug", False)
try:
return func(*args, **kwargs)
except click.ClickException:
raise
except grpc.RpcError as err:
status_code = err.code()
error_details = f"{err.details()}."
error_details = f"{err.details()} (gRPC Code: {status_code.name})."

if debug_mode:
console.print_exception()

if status_code == grpc.StatusCode.NOT_FOUND:
raise NotFoundError(error_details)
if status_code == grpc.StatusCode.INVALID_ARGUMENT:
raise InternalCliError(error_details) from err
elif status_code == grpc.StatusCode.NOT_FOUND:
raise InternalArmoniKError(error_details) from err
elif status_code == grpc.StatusCode.ALREADY_EXISTS:
raise InternalArmoniKError(error_details) from err
elif status_code == grpc.StatusCode.DEADLINE_EXCEEDED:
raise InternalArmoniKError(error_details) from err
elif status_code == grpc.StatusCode.INTERNAL:
raise InternalArmoniKError(f"An internal exception has occurred:\n{error_details}")
raise InternalArmoniKError(error_details) from err
elif status_code == grpc.StatusCode.UNKNOWN:
raise InternalArmoniKError(f"An unknown exception has occurred:\n{error_details}")
raise InternalArmoniKError(error_details) from err
else:
raise InternalError("An internal fatal error occurred.")
except Exception:
if kwargs.get("debug", False):
raise InternalArmoniKError(error_details) from err

except Exception as e:
if debug_mode:
console.print_exception()
else:
raise InternalError("An internal fatal error occurred.")
raise InternalCliError(f"CLI errored with exception:\n{e}") from e

return wrapper

Expand Down
14 changes: 10 additions & 4 deletions src/armonik_cli/exceptions.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import rich_click as click
from rich.panel import Panel
from rich import print


class ArmoniKCLIError(click.ClickException):
Expand All @@ -7,14 +9,18 @@ class ArmoniKCLIError(click.ClickException):
def __init__(self, message: str) -> None:
super().__init__(message)

def show(self, file=None):
"""Override to display errors in a cleaner format."""
print(Panel(self.format_message(), title="Error", style="red"))

class InternalError(ArmoniKCLIError):

class InternalCliError(ArmoniKCLIError):
"""Error raised when an unknown internal error occured."""

exit_code = 3


class InternalArmoniKError(ArmoniKCLIError):
"""Error raised when there's an error in ArmoniK, you need to check the logs there for more information."""


class NotFoundError(ArmoniKCLIError):
"""Error raised when a given object of the API is not found."""
exit_code = 4
2 changes: 1 addition & 1 deletion tests/commands/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ def test_task_cancel(mocker, cmd):
),
(
f"task create --endpoint {ENDPOINT} --session-id sessionid --payload-id payloadid --expected-outputs 1 --expected-outputs 2 --data-dependencies 3 --max-duration 00:00:15.00 --priority 0",
1,
3,
),
],
)
Expand Down
9 changes: 5 additions & 4 deletions tests/core/test_decorators.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
from grpc import RpcError, StatusCode

from armonik_cli.core.decorators import error_handler, base_command, base_group
from armonik_cli.exceptions import NotFoundError, InternalError
from armonik_cli.exceptions import InternalArmoniKError, InternalCliError


class DummyRpcError(RpcError):
Expand All @@ -20,7 +20,7 @@ def details(self):

@pytest.mark.parametrize(
("exception", "code"),
[(NotFoundError, StatusCode.NOT_FOUND), (InternalError, StatusCode.UNAVAILABLE)],
[(InternalArmoniKError, StatusCode.UNAVAILABLE)],
)
def test_error_handler_rpc_error(exception, code):
@error_handler
Expand All @@ -37,7 +37,7 @@ def test_error_handler_other_no_debug(decorator):
def raise_error():
raise ValueError()

with pytest.raises(InternalError):
with pytest.raises(InternalCliError):
raise_error()


Expand All @@ -47,7 +47,8 @@ def test_error_handler_other_debug(decorator):
def raise_error(debug=None):
raise ValueError()

raise_error(debug=True)
with pytest.raises(InternalCliError):
raise_error(debug=True)


@pytest.mark.parametrize("decorator", [base_group, base_group()])
Expand Down

0 comments on commit 4b0f9e7

Please sign in to comment.