update dev environments feature branch with latest idl changes#2334
Conversation
bbonaby
requested review from
SohamDas2021,
huzaifa-d,
kanismohammed and
sshilov7
| public CancellationToken CancellationToken => _cancellationTokenSource.Token; | ||
|
|
||
| public event TypedEventHandler<IComputeSystem, SDK.ApplyConfigurationResult>? Completed; | ||
| public event TypedEventHandler<IApplyConfigurationOperation, ApplyConfigurationActionRequiredEventArgs> ActionRequired = (s, e) => { }; |
There was a problem hiding this comment.
Any particular reason why to initialize these events to a dummy call?
We check if it's null when we invoke it. Specifically for ActionRequired we'll need to check if it's null when we start the operation. And if it is we should fail. Also we should check that at the time of the invokation and fail if there is event handler because nobody will respond to "show adaptive card request". (Yo don't need to implement that in this PR).
There was a problem hiding this comment.
I just find it cleaner to invoke when I initiate the event this way. No other reason. doing ActionRequired(this, args) versus ActionRequired?.Invoke(this, args) but we can update it
There was a problem hiding this comment.
I see it as personal religion in this case. ?Invoke is not much code. OTOH with a dummy event handler it’s:
- Loss of functionality: can’t check for null when needed (and we need it here).
- Waste of resource and likely a small perf hit calling extra even handler
To me, as an OS developer, that overweighs a few characters in invocation code. (I wish C# had a way to do check for null internally for events, so we wouldn’t need to use ?Invoke or a dummy handler)
| { | ||
| return Task.Run(() => | ||
| { | ||
| return _virtualMachine.ApplyConfiguration(this); |
There was a problem hiding this comment.
(Not for this PR) I think we'll need to move code from ApplyConfiguration into this class. There is no need for somewhat circular cross-class calls.
There was a problem hiding this comment.
yea I was trying to make the least minimal changes I could to get this all working. I figured you were already going to refactor this, and when attempting to move whats in CreateApplyConfigurationOperation to the ApplyConfigurationOperation class, I realized there was more moving parts than just whats under the try/catch. So I did it this way as a time saver.
| /// </summary> | ||
| [TestMethod] | ||
| public void TestConfigureRequest() | ||
| public async Task TestConfigureRequest() |
There was a problem hiding this comment.
Why async? This is a manual test for now. And even if we automate it later, I am not sure it can run in parallel with other tests. What if two tests try to call Configure on the same VM? Or one test doing configuration and another one trying to update DevSetupAgent. Those are not supported scenarios and these kinds of tests are not designed to run concurrently.
There was a problem hiding this comment.
I just made it async because of the IAsyncOperation, I wasn't thinking about running tests in parallel but I guess we can think about that later, unless you want it fixed in this PR
| // Pattern to allow multiple non-service registered interfaces to be used with registered interfaces during construction. | ||
| services.AddSingleton<IPowerShellService>(psService => | ||
| ActivatorUtilities.CreateInstance<PowerShellService>(psService, new PowerShellSession())); | ||
| services.AddSingleton<HyperVVirtualMachineFactory>(sp => psObject => ActivatorUtilities.CreateInstance<HyperVVirtualMachine>(sp, psObject)); |
There was a problem hiding this comment.
sure I can update it
| }; | ||
|
|
||
| var result = operation.CompletionStatus; | ||
| var result = await operation.StartAsync(); |
There was a problem hiding this comment.
In the future we should have a time limit for this wait. If something went wrong and we never received operation completion from the VM we'll wait forever.
There was a problem hiding this comment.
thats fair, I guess we can update it the same time we remove the async Task
* add initial SDK changes (#2209) * Add UX code from private ADO branch to public feature branch (#2241) * Add initial code from private branch that will be shared between the setup flow and the Dev environments tool page. PRs: from private ADO repo: https://dev.azure.com/microsoft/Dart/_git/DevHome?version=GBDevEnv * add changes to the setup flow for dev environment configuration PRS: from private ADO repo: https://dev.azure.com/microsoft/Dart/_git/DevHome?version=GBDevEnv * Add dev environments management tool page from private ADO repo: https://dev.azure.com/microsoft/Dart/_git/DevHome?version=GBDevEnv * fix conflicts and stylecop errors due to update from merge with main --------- Co-authored-by: Huzaifa Danish <modanish@microsoft.com> * Move Hyper-V extension code from private repository to public dev environments feature branch (#2246) * Add Hyper-v extension to Dev Home from Private Hyper-v extension branch: PRS https://github.com/microsoft/DevHomeHyperVExtension * Merge changes from Dev Home main and fix style cop errors * Initial implementation of IComputeSystem::ApplyConfiguration for HyperV extension. (#2258) * Add environments to experimentation (#2260) * Add PowerShell script and a helper class to deploy DevSetupAgent service to a Hyper-V VM. (#2261) * Hyper-V extension: Add "Ask for VM credentials" and "Wait for logon" Adaptive Cards to Hyper-V Configure command. (#2289) * Changes to make DevSetupEngine work with .NET 8 (#2308) * Build DevSetupAgent separately from the main Dev Home solution and add DevSetupAgent_*.zip to MSIX package. (#2325) * Add WaitForLogin and Credentials Adaptive Cards * Address review comments. * New VS solution for DevSetupAgent * Add BuildDevSetupAgentHelper script * Fix x86 DevSetupAgent to work on x64 OS. Create DevSetupAgent zip for different VM architectures. Fixed build scripts and HyperVExtension.csproj to include DevSetupAgent zip file into Dev Home MSIX package. * Remove old comment. * Fix a comment. * Update setup target flow to allow for configuration of a dev environment. (#2321) * initial code * update messaging and adaptive cards * remove added method * update strings and names * update based on initial comments and update IsHyperVModuleLoaded with new work around that doesn't involve installing using Install-Module which installs from PsGallery * improve wording * fix merge conflicts * update dev environments feature branch with latest idl changes (#2334) * update feature branch to latest sdk * update InputJson to inputJson * update with latest changes * Update environments page UX (#2320) * add updates to ui * fix crashes due to resource name mismatch * update feature branch to latest sdk * update InputJson to inputJson * update with latest changes * update * update to remove duplicate, resize shimmer, remove id from winget file since its not needed, and add comments * Re-add correct adaptive host file for correct theming, address some initial comments, update loading page to allow scrolling when there are tasks and actions in the action center. Fix error message spelling. * improve setup target loading page progress messaging * Fix git clone's dependsOn Id to match our new Ids for setup target flow * Update build scripts to build DevSetupAgent in Azure official build. (#2339) * update dev home to use new SDK version and other projects to use win app sdk v1.5 to prevent build issues (#2344) * Fix PS DevSetupAgent deployment script. (#2345) * remove classes that don't need to be added * Change all tabs to spaces * Update tools directory to use crlf * Update HyperVExtension directory to use crlf * Update common/Environments directory to use crlf * Update more in common directory to use crlf * [Hyper-V extension]: Fixes for DevSetupEngine registration, Configure progress reporting, and GitHub build. (#2348) * remove s that was added from a previous commit causing build to fail * fix tests for build * update malformed configuration string to allow arm64 to build --------- Co-authored-by: Huzaifa Danish <modanish@microsoft.com> Co-authored-by: sshilov7 <51001703+sshilov7@users.noreply.github.com> Co-authored-by: Kristen Schau <47155823+krschau@users.noreply.github.com>
2 participants
Summary of the pull request
References and relevant issues
Detailed description of the pull request / Additional comments
Validation steps performed
PR checklist