-
Notifications
You must be signed in to change notification settings - Fork 604
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
Simple UI Sample #746
base: master
Are you sure you want to change the base?
Simple UI Sample #746
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.
Great work!
@@ -0,0 +1,13687 @@ | |||
{ | |||
"name": "simple_dtl_ui", |
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.
Might consider removing this from the repo if it's a generated file
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.
Other Microsoft repos I've checked have included this file -- might make sense to ensure that users get the exact same set of dependencies when they try out the sample
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.
Sure, sounds good - if it's common to include it we should just leave it
RequestId = Activity.Current?.Id ?? HttpContext.TraceIdentifier; | ||
} | ||
} | ||
} |
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.
Maybe add a new line?
1. [Follow these instructions to register your application.](https://docs.microsoft.com/en-us/azure/active-directory/develop/quickstart-register-app) You will want to select the "Single-page application" tile when configuring platform settings. | ||
2. In the management panel of your app registration, select *API permissions* > *Add a permission* > *Azure Service Management* | ||
3. Select the *user_impersonation* permission | ||
4. Select *Add permissions* to complete |
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.
Double check for locked down AAD tenants if there is an extra step for global aad admins to do... If so, we should add a note for this in the readme
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.
It seems that, like we noted, "Admin consent required" is set to false in the Portal:
However, when I try logging in via the client, I get this error:
Does this seem like the appropriate article to link to? https://docs.microsoft.com/en-us/azure/active-directory/manage-apps/grant-admin-consent The user will need to figure out who their AAD admins are, though
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.
We should talk about this in our next meeting - tenant wide consent is a big hammer and there might be a less invasive approach... Thanks!
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.
Sounds good -- I'll make a note of this so that I don't forget to bring it up!
2. Next, navigate to your DevTest Lab in the Azure portal. Copy the **lab name, resource group name, and subscription ID** into the `appsettings.json` file. Once you have deployed the application to Azure, you can also set these values in the portal itself by following the instructions [here](https://docs.microsoft.com/en-us/azure/app-service/configure-common). | ||
|
||
### Deploy the application to Azure | ||
Right-click the SimpleDtlUI project in Visual Studio, and select *Publish*. |
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.
Link to the docs on publishing with Visual Studio would be great. Also can link to docs on deploying this with GitHub Actions, or Azure Devops Pipelines
Maybe add a screenshot of the app in the readme
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.
Updated, thanks!
"ASPNETCORE_ENVIRONMENT": "Development" | ||
} | ||
}, | ||
"simple_dtl_ui": { |
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.
This launch profile can probably be removed
import React, { Component } from 'react'; | ||
import OwnershipButton from './OwnershipButton'; | ||
|
||
export default class App extends Component { |
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.
Functional component instead of Class component
…mponents instead of class components
samples/DevTestLabs/SimpleUI/ClientApp/src/VirtualMachineTable.jsx
Outdated
Show resolved
Hide resolved
samples/DevTestLabs/SimpleUI/ClientApp/src/VirtualMachineTable.jsx
Outdated
Show resolved
Hide resolved
import { useAuthContext } from './Hooks'; | ||
import { OwnershipButton, VMAction } from './OwnershipButton'; | ||
|
||
const renderOwnershipButton = (virtualMachine, loggedInUser) => { |
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.
Might be worth breaking this into it's own component and then have the render function to just something like:
(virtualMachine, loggedInuser) =>
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.
@mharlan Do you mean just including this logic in the existing OwnershipButton component? Or refactoring it into another separate one? Would need to think of a good component name, then! 😄
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.
Either would work. Not sure if you have other situations where you intend the simple OwnershipButton to be used. This just reads like a function component to me.
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.
Understood! I'll move this into OwnershipButton since I don't think there's much potential for reusability there
This sample is to accompany the SharePoint + DevTest Labs blog series. The intention is to demonstrate the ease with which users of DevTest Labs can create a simplified user experience independent of the Azure portal.
It includes: