Skip to content
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

[Graph] Fix code samples in README #4754

Merged
merged 5 commits into from
Aug 15, 2019
Merged

Conversation

mikesprague
Copy link
Contributor

Code samples for the Graph client do not work as written. This PR updates the samples with the necessary changes for them to run.

  • Uses tenantId vs subscriptionId in Node and browser code examples
  • Removes extraneous curly brackets from Node example
  • Adds domain to interactiveLogin call in Node example

Fix code samples - use tenantId vs subscriptionId
Remove extraneous curly brackets
Add domain (tenantId) to interactiveLogin call
@@ -68,7 +71,7 @@ See https://github.com/Azure/ms-rest-browserauth to learn how to authenticate to
<script src="node_modules/@azure/ms-rest-browserauth/dist/msAuth.js"></script>
<script src="node_modules/@azure/graph/dist/graph.js"></script>
<script type="text/javascript">
const subscriptionId = "<Subscription_Id>";
const tenantId = "<Tenant_Id>";
Copy link
Contributor

Choose a reason for hiding this comment

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

How is this tenantId different from the one in line 33?

Copy link
Contributor Author

@mikesprague mikesprague Aug 14, 2019

Choose a reason for hiding this comment

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

Hi @ramya-rao-a - they are part of separate code examples. The tenantId on line 74 is part of the browser-specific example, whereas the one on line 33 you mentioned is part of the Node.js example. In practice, they should never be used together so there wouldn't actually be redundant variables.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, thats right. Apologies, that was an oversight from my end :)

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case, I would suggest to update line 33 to const tenantId = "<Tenant_Id>"; as well because we dont talk about the environment variable DOMAIN needing to be set anywhere in the readme.

Copy link
Contributor Author

@mikesprague mikesprague Aug 14, 2019

Choose a reason for hiding this comment

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

Sure, that makes sense. I was trying to change the original code as little as possible and it referenced an env var for subscription id prior to my other updates.

There's some additional code that can be removed (the following imports on lines 29-30 aren't used and aren't necessary for the example to run, I have tested this):

import * as msRest from "@azure/ms-rest-js";
import * as msRestAzure from "@azure/ms-rest-azure-js";

I have added a couple commits with the above changes.

@ramya-rao-a
Copy link
Contributor

ramya-rao-a commented Aug 13, 2019

Thanks for getting started on this @mikesprague!
Hopefully, we should be able to close #4292 with this.

Remove unused imports
Update tenantId for consistency across browser/Node examples
Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

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

This looks good.
Before I merge, can you share some pointers to the docs that talk about tenant id for Azure Graph?

@mikesprague
Copy link
Contributor Author

The source code for the current SDK (azure-sdk-for-js) references the tenantID in the constructor for the GraphRbacManagementClient:

Also, here are the examples from the "old" SDK (azure-sdk-for-node) that also point to tenantId vs subscriptionId:

FWIW, I spent more time then I'd like to admit trying to get the examples (as currently listed on the README) to work using the subscription id. While that wasn't the only issue, I have gone back and compared using subscription id to tenant id and subscription id fails/errors out every time.

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

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

Thanks for all the time you have spent on this @mikesprague!

This library has been auto-generated and so have the samples in the README file.
We are looking at improving this process, but this has been a great first step towards that.

Thanks again!

@ramya-rao-a ramya-rao-a merged commit 125d6ef into Azure:master Aug 15, 2019
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.

2 participants