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

Finish UI for environments creation #2539

Merged
merged 39 commits into from
Apr 9, 2024

Conversation

bbonaby
Copy link
Contributor

@bbonaby bbonaby commented Apr 5, 2024

Summary of the pull request

This PR is PR 4 of 4 that completes the create environment flow in machine configuration flow and the UI to show the operation progress in the Environments page. It allows us to start the create compute system operation provided by an extension, keep track of the operation and show progress of the operation in the UI.

Please see previous PRs for context:

  1. Add ability for Dev Home to display settings cards from an adaptive card #2488
  2. Add Hyper-V settings card styled adaptive cards  #2489
  3. Add initial environments creation flow to the machine configuration tool #2493

Things added:

  • Updated the DotNetZipArchiveProvider in the Hyper-V extension to only send progress updates when a percentage change happens.
    • Note: The .NET core implementation of the ZipFile class and the ZipArchive classes are very slow when unzipping large zip files. For example the Windows 11 image in the Hyper-V VM gallery is 20+ GB. It can take .NET over an hour or more to unzip them.
    • I'll created an issue for this and reference this PR so I can investigate other ways to unzip archive files, Note: the difficulty here is that we need to show progress, and it would be preferable to not have to include outside binaries to unzip a file. E.g needing to include 7zip binaries instead of calling apis from a nuget package. We may need to see if we can utilize an internal Windows api.
  • Updates the review page adaptive card Json template for the Hyper-V extension
  • Created wrapper classes for the ICreateComputeSystemOperation class that we receive from the extension.
  • Created a new view model for the create compute system operation
  • Added a new base abstract class for the environment cards that will show up in the environments page. This allows us to have different DataTemplates for different types of view models that we want to show in the UI. E.g The ComputeSystemViewModel and the CreateComputeSystemOperationViewModel, share the same ComputeSystemCardBase class so we only need a single list of ComputeSystemCardBase objects to show in the view. instead of having two separate lists.
  • Added ability for the environment cards to show status of the operation and a progress bar

Here is a small video of the flow up to the start of the archive extraction

Video.Showing.start.of.creation.flow.from.Environments.page.mp4

Here is a small video of the flow when the archive extraction from the Hyper-V extension starts taking place.:

Video.showing.update.messages.from.the.hyper-v.extension.when.its.extracting.a.file.mp4

References and relevant issues

Design/PM wanted the cards that show progress to always stay at the top of the UI. To do that I'd have to change the way we sort the cards. I didn't have time to change it so I'll create an issue to track that work as a bug fix

We also want to give the users the ability to switch accounts e.g azure accounts in both the environment and setup target page. Currently we only show the first account, I'll create another issue to address that as a bug fix.

Detailed description of the pull request / Additional comments

Validation steps performed

PR checklist

  • Closes #xxx
  • Tests added/passed
  • Documentation updated

bbonaby and others added 30 commits March 29, 2024 05:10
…aby/add-hyper-v-code-for-creation-adaptive-cards
…ds' into user/bbonaby/add-initial-creation-flow-in-setup-flow
…om holding onto items view control internally
…aby/add-hyper-v-code-for-creation-adaptive-cards
…ds' into user/bbonaby/add-initial-creation-flow-in-setup-flow
…ds' into user/bbonaby/finish-ui-for-environments-creation

public Exception Exception { get; }

public event TypedEventHandler<ICreateComputeSystemOperation, CreateComputeSystemProgressEventArgs> Progress = (s, e) => { };
Copy link
Member

Choose a reason for hiding this comment

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

This hurts my eyes as it's completely unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interestingly stylecop is the reason for this one. I think it might be time to standardize which stylecop errors we care about or not in Dev Home.

{
var repoConfigTasksTotal = _setupFlowOrchestrator.GetTaskGroup<RepoConfigTaskGroup>()?.CloneTasks.Count ?? 0;
var appManagementTasksTotal = _setupFlowOrchestrator.GetTaskGroup<AppManagementTaskGroup>()?.SetupTasks.Count() ?? 0;
if (_setupFlowOrchestrator.IsSettingUpATargetMachine && repoConfigTasksTotal == 0 && appManagementTasksTotal == 0)
Copy link
Member

Choose a reason for hiding this comment

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

Coding guidelines: "Use parentheses to make clauses in an expression apparent".
This is in Windows C++ guidelines and in C# Common code conventions.

@bbonaby bbonaby changed the base branch from user/bbonaby/add-initial-creation-flow-in-setup-flow to main April 5, 2024 23:21
@bbonaby bbonaby changed the base branch from main to user/bbonaby/add-initial-creation-flow-in-setup-flow April 5, 2024 23:21
@bbonaby bbonaby changed the base branch from user/bbonaby/add-initial-creation-flow-in-setup-flow to main April 6, 2024 01:34
Copy link
Contributor

@huzaifa-d huzaifa-d left a comment

Choose a reason for hiding this comment

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

Looked at it from a high level as it's quite long.

@@ -19,10 +20,11 @@ public static class StreamExtensions
/// <param name="progressProvider">The object that progress will be reported to</param>
/// <param name="bufferSize">The size of the buffer which is used to read data from the source stream and write it to the destination stream</param>
/// <param name="cancellationToken">A cancellation token that will allow the caller to cancel the operation</param>
public static async Task CopyToAsync(this Stream source, Stream destination, IProgress<long> progressProvider, int bufferSize, CancellationToken cancellationToken)
public static async Task CopyToAsync(this Stream source, Stream destination, IProgress<ByteTransferProgress> progressProvider, int bufferSize, long totalBytesToExtract, CancellationToken cancellationToken)
{
var buffer = new byte[bufferSize];
long totalRead = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Use var everywhere.


using System;
using System.Collections.Generic;
using System.Linq;
Copy link
Contributor

Choose a reason for hiding this comment

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

Unnecessary

@bbonaby bbonaby merged commit cfd0cb0 into main Apr 9, 2024
4 checks passed
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