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

Update Pinning operations and error messaging for Dev Box #182

Merged
merged 6 commits into from
May 15, 2024

Conversation

bbonaby
Copy link
Contributor

@bbonaby bbonaby commented May 14, 2024

Summary of the pull request

Note: Secondary PR in Dev Home repository: microsoft/devhome#2907

This PR makes sure that the pinning operation and errors are reflected in Dev Home. Currently When a pinning operation happens there is no status updates. Dev Home expects for every operation that the Compute System supports that there be a status update to inform the user something is occuring. E.g "Starting".

Things this PR updates:

  1. updates the supported operations for Dev Boxes based on the state.
  2. Updates the error messages and points the user to the azure extension logs
  3. Updates the "DoPinActionAsync" and "GetPinStatusAsync" methods to be within a try/catch.
  4. Remove the async void and used async task in the LoadWindowsAppParameters method as it was causing the Dev Boxes to randomly not contain the required WindowsAppParameters by the time Dev Home called the GetPinStatusAsync.

Video showing me performing the pinning and unpinning operations:

pinned.to.start.operations.updating.mp4

References and relevant issues

Detailed description of the pull request / Additional comments

Note: Due to the asynchrous calls for GetPinStatusAsync, when the operation to pin/upin the dev box is completed, the pin/pinned operations don't reset until GetPinStatusAsync is complete so there is a small delay when the operations update. We can work on trying to optimize this reset in a future PR.

Validation steps performed

PR checklist

{
try
{
// This will always result in a returned value in non test scenarios. But will throw when run in a unit test as ApplicationData does not exist.
Copy link
Member

Choose a reason for hiding this comment

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

nit: You could catch that exception and use Path.GetTempPath() to account for test scenarious.

@bbonaby bbonaby merged commit 3eb4e01 into main May 15, 2024
3 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.

5 participants