Skip to content

Conversation

@sangonzal
Copy link
Contributor

@sangonzal sangonzal commented Oct 1, 2019

Quoted from a conversation:

Link to API ID doc: https://microsoft.sharepoint.com/:w:/t/aad/devex/EZGF46opilBKrgC59mUFttIBebPBY6MMGLO1eGz0uXQh6w?e=TSq76f

Note that these values are not in there yet. The way that this has worked in the past for .NET and Java is that we have picked values that fit within the ranges specified which have not been taken, then we'd update the doc.

We are working with some of the other teams to make sure that most up to date values are in there. For that, we have this spreadsheet: https://microsoft-my.sharepoint-df.com/:x:/p/sagonzal/EXSrr4vM1utAqQQfD6bMln4BYKwjrqh3cagiNJWPVNjLzw?e=G5FybL

@sangonzal sangonzal requested review from abhidnya13 and rayluo October 1, 2019 19:43
Copy link
Contributor

@abhidnya13 abhidnya13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks good to me.
I dont have any suggestion than using **kwargs for adding additional parameters.

Copy link
Contributor

@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.

Thank you @sangonzal for your proper implementation on our whiteboard discussion before this PR! That already gave us a head-start. I'm adding some detail comments below.

@sangonzal
Copy link
Contributor Author

I have tested all flows and have validated that data is showing up in Kusto

@sangonzal
Copy link
Contributor Author

@rayluo @abhidnya13 are there any plans to merge this in? As far as I can tell, all feedback has been addressed.
cc: @henrik-me

Copy link
Contributor

@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.

Thanks @sangonzal for all the improvement since last review!
This time I leave only one high level question on the API IDs, and a suggestion on Device Flow. All other things are just minor editorial/code-style changes.

Copy link
Contributor

@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.

Thanks @sangonzal for improving this PR! All the previous PR feedbacks from me has been addressed.

Per off-line discussion, I modified it to share correlation id among same "batch" of acquire_token_silent(...) internal calls. And added some other coding style changes. @sangonzal please do a final review.

UPDATE from next day:
[8:47 AM] Santiago Gonzalez
Just took a look at it - looks good to me
Feel free to merge in

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.

4 participants