-
Notifications
You must be signed in to change notification settings - Fork 206
Application initializer does not make tenant discovery calls #205
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
Conversation
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.
Commented inline. Also thanks for getting the test cases passed, which gives us a clear understanding on the impact. Keep up your good work! :-)
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.
The current modification looks clean. Good job!
For such a change that would impact literally every existing MSAL feature, have we tested all the scenarios, especially those not currently automated? I would suggest we scan through every MSAL public api, and run a test invoking that api. And then you can reply back with a summary of your test coverage.
Tested the public api and added a few more changes |
@rayluo summarized our offline conversation in the comment to give a context on the changes made! |
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.
Looks great! Time for squash and merge! 🎉 😄
UPDATE: This will fix #187