Handle jmp-client exceptions and introduce a exception handling module#293
Handle jmp-client exceptions and introduce a exception handling module#293
Conversation
|
Warning Rate limit exceeded@mangelajo has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (12)
WalkthroughThis pull request introduces a new command Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant CLI as Jumpstarter CLI
participant Cmd as Command Handler (client_lease, client_config, etc.)
participant EH as Exception Handler (@handle_exceptions)
U->>CLI: Execute "client_lease list"/"release"/"request"
CLI->>Cmd: Dispatch command
Cmd->>EH: Process command (wrapped in @handle_exceptions)
EH-->>Cmd: Return result or formatted error
Cmd-->>CLI: Respond with output/error message
CLI-->>U: Display final output
sequenceDiagram
participant C as ClientConfig
participant gRPC as gRPC Service
participant TE as Exception Translator (translate_grpc_exceptions)
C->>TE: Wrap gRPC call with exception translator
TE->>gRPC: Make lease/config request
gRPC-->>TE: Return result or throw gRPC error
TE-->>C: Relay translated ConnectionError/ConfigurationError
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
mangelajo
force-pushed
the
exception-handling-cli-client
branch
from
1ae4d8a to
0e06e4d
Compare
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| """Raised when a file is not found.""" | ||
| pass | ||
|
|
||
| async def async_exception_handling(func): |
There was a problem hiding this comment.
| async def async_exception_handling(func): | |
| async def async_handle_exception(func): |
mangelajo
changed the title
mangelajo
force-pushed
the
exception-handling-cli-client
branch
from
0e06e4d to
78154a7
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py (1)
18-28: 🛠️ Refactor suggestionUse click.BadParameter for consistent error handling.
For consistency with other commands, replace
ValueErrorwithclick.BadParameter.if not config: - raise ValueError("no client specified") + raise click.BadParameter( + "no client specified, and no default client set: specify a client name, or use jmp client use-config", + param_hint="name" + )
🧹 Nitpick comments (7)
packages/jumpstarter/jumpstarter/common/grpc.py (1)
49-68: Comprehensive translation of gRPC exceptions
Consider expanding to handle other gRPC status codes (like PERMISSION_DENIED or DEADLINE_EXCEEDED) if they are relevant.packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py (1)
24-25: Use f-string for better readability.Consider using an f-string instead of string concatenation for better readability.
- raise click.BadParameter("no client specified, and no default client set:" + - "specify a client name, or use jmp client use-config ", param_hint="name") + raise click.BadParameter( + f"no client specified, and no default client set: specify a client name, or use jmp client use-config", + param_hint="name" + )packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (1)
26-38: Rename for consistency with async version.For consistency with the async version, consider renaming this decorator to
handle_exception(singular).-def handle_exceptions(func): +def handle_exception(func):packages/jumpstarter/jumpstarter/common/exceptions.py (1)
4-26: Extract ANSI color codes as module-level constants.Consider moving the ANSI color codes to module-level constants for better maintainability and reusability.
+# At the top of the file +ANSI_RED = "\033[91m" +ANSI_CLEAR = "\033[0m" + class JumpstarterException(Exception): # ... existing code ... def print(self, message:str|None = None): - ANSI_RED = "\033[91m" - ANSI_CLEAR = "\033[0m" print(f"{ANSI_RED}{self}{ANSI_CLEAR}", file=sys.stderr)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py (3)
12-15: Align command group name with function name.The command group name ('lease') is inconsistent with the function name ('client_lease'). Consider aligning them for better clarity.
-@click.group(name="lease", cls=AliasedGroup, short_help="") +@click.group(name="client-lease", cls=AliasedGroup, short_help="") def client_lease():
44-45: Use f-strings for better readability.Replace string concatenation with f-strings for better readability.
- raise click.BadParameter("no client specified, and no default client set:" + - "specify a client name, or use jmp client use-config ", param_hint="name") + raise click.BadParameter( + f"no client specified, and no default client set: specify a client name, or use jmp client use-config", + param_hint="name" + ) - raise click.BadParameter("no lease specified, provide one or use --all to release all leases", - param_hint="lease") + raise click.BadParameter( + "no lease specified, provide one or use --all to release all leases", + param_hint="lease" + )Also applies to: 52-53
88-89: Use f-strings for better readability.Replace string concatenation with f-strings for better readability.
- raise click.BadParameter("no client specified, and no default client set:" + - "specify a client name, or use jmp client use-config ", param_hint="name") + raise click.BadParameter( + f"no client specified, and no default client set: specify a client name, or use jmp client use-config", + param_hint="name" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/jumpstarter-cli-client/jumpstarter_cli_client/__init__.py(2 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_config.py(4 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py(4 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py(2 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py(1 hunks)packages/jumpstarter/jumpstarter/client/exceptions.py(1 hunks)packages/jumpstarter/jumpstarter/client/lease.py(3 hunks)packages/jumpstarter/jumpstarter/common/exceptions.py(1 hunks)packages/jumpstarter/jumpstarter/common/grpc.py(2 hunks)packages/jumpstarter/jumpstarter/config/client.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-dev .devfile/Containerfile)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (24)
packages/jumpstarter/jumpstarter/common/grpc.py (4)
3-5: Imports look good
No issues found with addingsocketandcontextmanager.
10-14: Centralized exception usage and environment configuration
Invokingconfigure_grpc_env()early ensures consistent logging behavior for all gRPC calls, which is good for debugging and maintenance.
16-29: Robust error handling for insecure connections
Covers parsing errors, DNS resolution failures, and connection refusals with specific exceptions, which is a clear and maintainable approach.
40-48: Well-structured environment configuration
Conditionally settingGRPC_VERBOSITYandGLOG_minloglevelprevents excessive output, benefiting readability of logs.packages/jumpstarter/jumpstarter/client/exceptions.py (2)
1-2: Base exception import
Properly imports the sharedexceptionsmodule to extend its base exception class.
4-5: Specialized exception for lease operations
Clearly indicates lease-related failures and improves error clarity across the codebase.packages/jumpstarter-cli-client/jumpstarter_cli_client/__init__.py (2)
8-8: Imported client_lease
Successfully replaces the oldleasecommand reference with the newclient_leasecommand.
33-33: Registered the client_lease command
Ensures that the refactored command is now fully integrated into the CLI tool.packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py (2)
2-2: LGTM!The import statement is correctly placed and follows Python import conventions.
16-16: LGTM!The addition of the
@handle_exceptionsdecorator aligns with the PR's objective of standardizing error handling across CLI commands.packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py (2)
8-10: LGTM!The
ClickExceptionRedclass provides a clear visual distinction for errors by formatting messages in red.
12-24: Fix typo in decorator name.Based on previous review comments, the decorator name should be
async_handle_exception(singular) instead ofasync_handle_exceptions(plural).-async def async_handle_exceptions(func): +async def async_handle_exception(func):packages/jumpstarter/jumpstarter/common/exceptions.py (1)
27-45: LGTM!The specific exception classes are well-organized, consistently structured, and have clear docstrings explaining their purpose.
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_config.py (4)
54-54: LGTM! Good error handling improvement.The
@handle_exceptionsdecorator will ensure consistent error handling for this command.
103-103: LGTM! Good error handling improvement.The
@handle_exceptionsdecorator will ensure consistent error handling for this command, including any exceptions fromset_next_client.
111-111: LGTM! Good error handling improvement.The
@handle_exceptionsdecorator will ensure consistent error handling for this command, including any file access or user config related exceptions.
137-137: LGTM! Good error handling improvement.The
@handle_exceptionsdecorator will ensure consistent error handling for this command, including any exceptions from loading or saving user config.packages/jumpstarter/jumpstarter/client/lease.py (4)
48-56: LGTM! Good gRPC error handling improvement.The
translate_grpc_exceptionscontext manager will ensure that gRPC exceptions are properly translated into domain-specific exceptions.
60-67: LGTM! Good docstring update.The docstring now accurately reflects the new exception types that can be raised by the method.
75-77: LGTM! Good docstring update.The docstring now accurately reflects the new exception types that can be raised by the method.
93-114: LGTM! Good error handling improvements.The changes improve error handling in multiple ways:
- Added
translate_grpc_exceptionsfor gRPC calls- Updated to use domain-specific
LeaseError- Enhanced error messages for better clarity
packages/jumpstarter/jumpstarter/config/client.py (3)
93-94: LGTM! Good gRPC error handling improvement.The
translate_grpc_exceptionscontext manager will ensure that gRPC exceptions are properly translated into domain-specific exceptions.
98-99: LGTM! Good gRPC error handling improvement.The
translate_grpc_exceptionscontext manager will ensure that gRPC exceptions are properly translated into domain-specific exceptions.
103-104: LGTM! Good gRPC error handling improvement.The
translate_grpc_exceptionscontext manager will ensure that gRPC exceptions are properly translated into domain-specific exceptions.
mangelajo
force-pushed
the
exception-handling-cli-client
branch
from
78154a7 to
c9a14c7
Compare
| except JumpstarterException as e: | ||
| raise ClickExceptionRed(str(e)) from None | ||
| except click.ClickException: | ||
| raise # if it was already a click exception from the cli commands, just re-raise it |
There was a problem hiding this comment.
I am just being explicit here (as I could be leaving this to the "except Exception", but I want to make it clear that if we do the Exception later in time, we need to keep raising the ClickExceptions cleanly.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py (2)
27-27: 🛠️ Refactor suggestionUse click.BadParameter for consistent error handling.
For consistency with other error handling in the codebase, use
click.BadParameterinstead ofValueError.- raise ValueError("no client specified") + raise click.BadParameter( + "no client specified, and no default client set: specify a client name, or use jmp client use-config", + param_hint="name" + )
76-80: 🛠️ Refactor suggestionUpdate example code in docstring.
The example code in the docstring still uses the old
leasecommand instead ofclient_lease.- $ JMP_LEASE=$(jmp lease request -l label match) + $ JMP_LEASE=$(jmp client-lease request -l label match) $ jmp shell $$ j --help $$ exit - $ jmp lease release -l "${JMP_LEASE}" + $ jmp client-lease release -l "${JMP_LEASE}"
🧹 Nitpick comments (6)
packages/jumpstarter/jumpstarter/common/grpc.py (2)
40-48: Enhance function documentation.Consider adding a docstring to explain:
- The purpose of the function
- The meaning of each environment variable
- How to enable verbose logging if needed
Example docstring:
def configure_grpc_env(): + """Configure gRPC logging environment. + + Sets GRPC_VERBOSITY and GLOG_minloglevel to suppress informative logs + by default. To enable verbose logging, set: + - GRPC_VERBOSITY to INFO, DEBUG, etc. + - GLOG_minloglevel to 0 or 1 + """
49-68: Improve exception handling.Two suggestions for improvement:
- The catch-all
Exceptionhandler is redundant as it simply re-raises the exception.- Consider adding more specific error messages for different gRPC error codes.
Apply this diff to improve the error handling:
@contextmanager def translate_grpc_exceptions(): """Translate grpc exceptions to JumpstarterExceptions.""" try: yield except grpc.aio.AioRpcError as e: if e.code().name == "UNAVAILABLE": # tls or other connection errors raise ConnectionError(f"grpc error: {e.details()}") from None if e.code().name == "UNKNOWN": # an error returned from our functions raise ConnectionError(f"grpc controller responded: {e.details()}") from None + if e.code().name == "DEADLINE_EXCEEDED": + raise ConnectionError("grpc request timed out") from None + if e.code().name == "PERMISSION_DENIED": + raise ConnectionError("grpc authentication failed") from None else: - raise ConnectionError("grpc error") from e + raise ConnectionError(f"grpc error: {e.code().name}") from e except grpc.RpcError as e: raise ConnectionError("grpc error") from e except ValueError as e: raise ConfigurationError("grpc error") from e - except Exception as e: - raise epackages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py (1)
24-25: Use f-strings for better readability.Instead of string concatenation with
+, consider using f-strings for better readability.- raise click.BadParameter("no client specified, and no default client set:" + - "specify a client name, or use jmp client use-config ", param_hint="name") + raise click.BadParameter( + f"no client specified, and no default client set: specify a client name, or use jmp client use-config", + param_hint="name" + )packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py (3)
12-13: Update command group name for consistency.The command group name should be updated to match the function name for consistency.
-@click.group(name="lease", cls=AliasedGroup, short_help="") +@click.group(name="client-lease", cls=AliasedGroup, short_help="Manage leases held by the current client") def client_lease():
44-45: Use f-strings for better readability.Instead of string concatenation with
+, consider using f-strings for better readability.- raise click.BadParameter("no client specified, and no default client set:" + - "specify a client name, or use jmp client use-config ", param_hint="name") + raise click.BadParameter( + f"no client specified, and no default client set: specify a client name, or use jmp client use-config", + param_hint="name" + )
88-89: Use f-strings for better readability.Instead of string concatenation with
+, consider using f-strings for better readability.- raise click.BadParameter("no client specified, and no default client set:" + - "specify a client name, or use jmp client use-config ", param_hint="name") + raise click.BadParameter( + f"no client specified, and no default client set: specify a client name, or use jmp client use-config", + param_hint="name" + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
packages/jumpstarter-cli-client/jumpstarter_cli_client/__init__.py(2 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_config.py(4 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py(4 hunks)packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py(2 hunks)packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py(1 hunks)packages/jumpstarter/jumpstarter/client/exceptions.py(1 hunks)packages/jumpstarter/jumpstarter/client/lease.py(3 hunks)packages/jumpstarter/jumpstarter/common/exceptions.py(1 hunks)packages/jumpstarter/jumpstarter/common/grpc.py(2 hunks)packages/jumpstarter/jumpstarter/config/client.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/jumpstarter/jumpstarter/client/exceptions.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/init.py
- packages/jumpstarter-cli-common/jumpstarter_cli_common/exceptions.py
- packages/jumpstarter-cli-client/jumpstarter_cli_client/client_config.py
- packages/jumpstarter/jumpstarter/config/client.py
- packages/jumpstarter/jumpstarter/client/lease.py
- packages/jumpstarter/jumpstarter/common/exceptions.py
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: e2e
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter-devspace .devfile/Containerfile.client)
- GitHub Check: build-and-push-image (jumpstarter-dev/jumpstarter Dockerfile)
🔇 Additional comments (5)
packages/jumpstarter/jumpstarter/common/grpc.py (1)
15-29: Review the insecure flag logic.The code attempts to get the server's SSL certificate even when
tls_config.insecureis true. This seems counterintuitive as the "insecure" flag typically implies skipping certificate verification.Consider if this is the intended behavior. If not, you might want to use
grpc.insecure_channel_credentials()instead wheninsecureis true.packages/jumpstarter-cli-client/jumpstarter_cli_client/client_shell.py (1)
16-17: LGTM! Good addition of consistent error handling.The
@handle_exceptionsdecorator ensures consistent error handling across the CLI commands.packages/jumpstarter-cli-client/jumpstarter_cli_client/client_lease.py (3)
20-21: LGTM! Good addition of consistent error handling.The
@handle_exceptionsdecorator ensures consistent error handling across the CLI commands.
37-38: LGTM! Good addition of consistent error handling.The
@handle_exceptionsdecorator ensures consistent error handling across the CLI commands.
60-61: LGTM! Good addition of consistent error handling.The
@handle_exceptionsdecorator ensures consistent error handling across the CLI commands.
mangelajo
force-pushed
the
exception-handling-cli-client
branch
2 times, most recently
from
43f17bf to
cb4f26c
Compare
mangelajo
force-pushed
the
exception-handling-cli-client
branch
from
cb4f26c to
d18554f
Compare
2 participants
Summary by CodeRabbit
New Features
client_leasecommand in the CLI, replacing the previousleasecommand for a streamlined experience.Bug Fixes / Improvements