potiuk commented on code in PR #61042:
URL: https://github.com/apache/airflow/pull/61042#discussion_r2727493641
##########
airflow-ctl/src/airflowctl/api/client.py:
##########
@@ -168,7 +168,39 @@ def load(self) -> Credentials:
debug_credentials = json.load(df)
self.api_token =
debug_credentials.get(f"api_token_{self.api_environment}")
else:
- self.api_token = keyring.get_password("airflowctl",
f"api_token_{self.api_environment}")
+ try:
+ self.api_token = keyring.get_password(
+ "airflowctl", f"api_token_{self.api_environment}"
+ )
+ except ValueError as e:
+ # Incorrect keyring password
+ if self.client_kind == ClientKind.AUTH:
+ # For AUTH kind, log warning and continue (user is
logging in)
+ log.warning(
+ "Could not access keyring for environment %s:
%s", self.api_environment, e
+ )
+ self.api_token = None
+ else:
+ # For CLI kind, provide helpful error and raise
+ error_msg = (
+ f"Failed to access system keyring for
environment '{self.api_environment}'. "
+ f"This usually means an incorrect keyring
password was entered.\n"
+ f"To fix this:\n"
+ f" 1. Ensure your system keyring password is
correct\n"
+ f" 2. Or use AIRFLOW_CLI_DEBUG_MODE=true to
bypass keyring\n"
+ f" 3. Or run 'airflowctl auth login' to
re-authenticate"
+ )
+ raise
AirflowCtlCredentialNotFoundException(error_msg) from e
Review Comment:
Looks good but one nit.
Should we just print the erorr aand `sys.exit(42)` here ? cc: @bugraoz93
I see that we are already handling it elsewhere:
```
try:
function(args)
except (
AirflowCtlCredentialNotFoundException,
AirflowCtlConnectionException,
AirflowCtlNotFoundException,
) as e:
rich.print(f"command failed due to {e}")
sys.exit(1)
```
I think in case of a fatal error in CLIs, I find the default exception
handling as verbose and unnecessary - so my usual approach for fatal CLI errors
is to print as descriptive and actionable error message as possible, with
possibly even more details in debug mode, and .... exit with error.
Otherwise - at least by default - your helpful error message will be
overwhelmed by the stack trace printed by default - which usually in CLIs is
very misleading, unhelpful for the users, confusing and intimidating.
So I would generally handle that in one of the two ways:
1) print error message + sys.exit()
2) raise a deliberate "ExitKindOfException" with appropriate error message,
and handle it in one place with printing - but only printing THE MESSAGE - not
the exception itself.
I found such `sys.exit()` a bit better because you can decide how to print
he message, you can format and colour it apropriately ("actual error in red,
warning - i.e. somtething to check - in yellow, instructions to follow in
white"), this is pretty much approach we folow in breeze and it works nicely
even for technical users of breeze, who could be - in principle - interested in
stack trace and details, but in fact they really want to understand what error
it is and how they can fix it.
Approach 2 is also good (you can have some implicit dependency on rich and
format your message when you prepare it with rich directives) - also you can
use some conventions to handle it differently in case of verbose output.
WDYT @bugraoz93 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
