-
Notifications
You must be signed in to change notification settings - Fork 15
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Cleaner exit when user credentials are incorrect #2296
Cleaner exit when user credentials are incorrect #2296
Conversation
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is how we should solve this. We want to catch the problem and raise a suitable exception, which is handled by the command function.
This makes the script more flexible, you can add options without having to specify the test dir.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@craddm Could you take a look at this and say what you think?
I can't request a review from you as you opened the PR 😆
LGTM |
✅ Checklist
Enable foobar integration
rather than515 foobar
).develop
.🚦 Depends on
When the user rejects the current credentials when asked to confirm they are correct, code would still continue to run, resulting in numerous additional, unnecessary and misleading error messages.
In addition, when the user rejects the Graph API credentials as incorrect, there is currently no clear way to change those credentials.
Graph API log-in is, unlike Az CLI log-in, not done by the user at the command line.
This PR simply exits when the user confirms that the credentials are incorrect, and prints only informative error messages.
In addition, it advises the user how to remove the cached Graph API credentials so as to be able to login with other credentials.
🌂 Related issues
Closes #2280
Closes #2153
🔬 Tests
Tested locally