{Core} az login: Add error handling when finding subscriptions #15776
Conversation
evelyn-ys
requested review from
Juliehzl,
arrownj,
fengzhou-msft,
haroldrandom,
jiasli and
jsntcy
as code owners
|
Core |
There was a problem hiding this comment.
For some weird cases(CX upgrade or degrade MGMT_RESOURCE_SUBSCRIPTIONS themselves, I guess.), get_client_class will fail and return None. If not verified, line 829 will throw TypeError: _NoneType_ object is not callable.
@jiasli Any idea on the real reason?
There was a problem hiding this comment.
I am ok with it, currently.
But we may need go deeper to see what are the exact reasons that cause the errrors and provide the corresponding suggestions. Because our goal is to resolve all the CLIInternalErrors
There was a problem hiding this comment.
This is because get_client_class returned None. In such case azure-mgmt-resource SDK is somehow missing.
get_client_class should actually raise an Exception instead of returning None. This is a design flaw and need further discussion.
There was a problem hiding this comment.
In some cases from the telemetry, CX tried acquiring token from strange authority url. I don't know how it happens, maybe they use private cloud misconfigured. I just add catch logic here.
There was a problem hiding this comment.
Users can register their own cloud with az cloud register and provide invalid URL for AAD. Consider mark that as a UserFault.
There was a problem hiding this comment.
I actually use UnknownError, which is a subclass of UserFault
There was a problem hiding this comment.
I suggest we define a new error type InvalidURL error. What do you think?
There was a problem hiding this comment.
replaced with UnknownError
There was a problem hiding this comment.
I am ok with it, currently.
But we may need go deeper to see what are the exact reasons that cause the errrors and provide the corresponding suggestions. Because our goal is to resolve all the CLIInternalErrors
There was a problem hiding this comment.
Always use lazy import to avoid performance impact. See #13843
There was a problem hiding this comment.
Users can register their own cloud with az cloud register and provide invalid URL for AAD. Consider mark that as a UserFault.
There was a problem hiding this comment.
Consider unifying the code in one place to avoid duplication.
There was a problem hiding this comment.
As the acquire_token fuction diffs for each scenario, the debug log and error msg are also customized. So I didn't unify the code.
There was a problem hiding this comment.
Does this mean status and reason can be missing? Kindly provide more context.
There was a problem hiding this comment.
That's right. The HttpError.response returned somehow doesn't have status and reason attribute. In that case, I use the whole response as err_msg
|
Fix #15836 |
evelyn-ys
force-pushed
the
err_improvement
branch
from
b2e09ad to
bc51668
Compare
5 participants
Description
This PR is a supplement to #15574 , also works as a part of fixing the none-actionable issues in #14981
Fix the left uncaught errors with
az loginFix #15836
Testing Guide
see the unit tests in test_profile.py