bugraoz93 commented on code in PR #61042:
URL: https://github.com/apache/airflow/pull/61042#discussion_r2733717462
##########
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:
First of all, thanks a lot for the PR @dheerajturaga!
The entire capture is great! I would like to add a couple of more granular
details on where these are in airflowctl and why to relate :)
We are doing exactly this, and even in this case, we still need to do some
improvement on printing error messages because even if it is called from a
single place, a meaningful exception handling message is really important in
specific cases, even if some exceptions are really congested over the same
error and getting flaky of one of each :)
Why is it on the CTL side and not on the API side? This is also needed
because one of the agreed approaches to keep things aligned with specific use
cases, the handling should be on the CTL (CLI Command) part, not on the API
specifically, because that is not something airflowctl controls. It only knows
how to communicate with the client, where the API do the magical part in that
component, so we need to be clear also over the CTL level if we really need to
catch and print something, rather than informing the user what is wrong, such
as `UniqueConstraintViolationException`.
Safe Call (Generic exception handling, if not caught specifically from any
of the custom commands, where we need to apply from a mapping over automated
ones, I believe, verbosity is also easier to manage from a single place. We are
planning to add similar mapping for help text. I don't want to end up huge
scroll of configuration for them but to some degree, we will need them):
https://github.com/apache/airflow/blob/1f0c2c1e43bb472076dbcfc8d966ae8fd7cda9fe/airflow-ctl/src/airflowctl/__main__.py#L34
What Safe Call do is exactly what Jarek showed in the code block.
https://github.com/apache/airflow/blob/1f0c2c1e43bb472076dbcfc8d966ae8fd7cda9fe/airflow-ctl/src/airflowctl/ctl/cli_config.py#L66-L102
Therefore, I would like to ask you not to do `sys.exit` and raise a
meaningful exception there, where we are mapping some of them here, but I
remember we agreed to follow base `AirflowException` (our case
`AirflowCtlException`, where we see that generic exception caused ambiguity),
so creating a new one if needed is okay there
https://github.com/apache/airflow/blob/main/airflow-ctl/src/airflowctl/exceptions.py
--
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]
