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.

Sign up for GitHub

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

[Graph] Fix code samples in README #4754

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
166ceb6
[Graph] Update README.md
mikesprague Aug 13, 2019
225859c
[Graph] Update README.md
mikesprague Aug 13, 2019
4a1381e
[Graph] Update README.md
mikesprague Aug 13, 2019
f1c7421
[Graph] Update README.md
mikesprague Aug 14, 2019
9e9293e
[Graph] Update REAMDE.md
mikesprague Aug 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions sdk/graphrbac/graph/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,10 +30,13 @@ import * as msRest from "@azure/ms-rest-js";
import * as msRestAzure from "@azure/ms-rest-azure-js";
import * as msRestNodeAuth from "@azure/ms-rest-nodeauth";
import { GraphRbacManagementClient, GraphRbacManagementModels, GraphRbacManagementMappers } from "@azure/graph";
const subscriptionId = process.env["AZURE_SUBSCRIPTION_ID"];
const tenantId = process.env["DOMAIN"];

msRestNodeAuth.interactiveLogin({{ tokenAudience: "https://graph.windows.net" }}).then((creds) => {
const client = new GraphRbacManagementClient(creds, subscriptionId, {
msRestNodeAuth.interactiveLogin({
tokenAudience: "https://graph.windows.net",
domain: tenantId
}).then((creds) => {
const client = new GraphRbacManagementClient(creds, tenantId, {
baseUri: "https://graph.windows.net"
});
client.signedInUser.get().then((result) => {
Expand Down Expand Up @@ -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 •
edited
Loading

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 •
edited
Loading

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.

const authManager = new msAuth.AuthManager({
clientId: "<client id for your Azure AD app>",
tenant: "<optional tenant for your organization>"
Expand All @@ -78,7 +81,7 @@ See https://github.com/Azure/ms-rest-browserauth to learn how to authenticate to
// may cause redirects
authManager.login();
}
const client = new Azure.Graph.GraphRbacManagementClient(res.creds, subscriptionId, {
const client = new Azure.Graph.GraphRbacManagementClient(res.creds, tenantId, {
baseUri: "https://graph.windows.net"
});
client.signedInUser.get().then((result) => {
Expand Down