Skip to content

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

Merged
merged 15 commits into from
Jun 22, 2020

Conversation

abhidnya13
Copy link
Contributor

@abhidnya13 abhidnya13 commented Jun 9, 2020

UPDATE: This will fix #187

Copy link
Collaborator

@rayluo rayluo left a 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! :-)

@abhidnya13 abhidnya13 requested a review from rayluo June 16, 2020 19:27
@abhidnya13 abhidnya13 requested a review from rayluo June 17, 2020 16:32
@abhidnya13 abhidnya13 requested a review from rayluo June 17, 2020 20:36
@abhidnya13 abhidnya13 marked this pull request as ready for review June 17, 2020 20:47
Copy link
Collaborator

@rayluo rayluo left a 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.

@abhidnya13 abhidnya13 requested a review from rayluo June 17, 2020 23:56
@abhidnya13
Copy link
Contributor Author

Tested the public api and added a few more changes

@abhidnya13 abhidnya13 requested a review from rayluo June 22, 2020 19:17
@abhidnya13
Copy link
Contributor Author

@rayluo summarized our offline conversation in the comment to give a context on the changes made!

Copy link
Collaborator

@rayluo rayluo left a 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! 🎉 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClientApplication initializer sends HTTP requests
2 participants