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

Show correct completion state after Dev Box creation and handle deletion during creation #163

Merged

Conversation

bbonaby
Copy link
Contributor

@bbonaby bbonaby commented Apr 24, 2024

Summary of the pull request

This PR accomplishes two things:

  1. When you create a Dev Box, the Dev Center sends back the state of the Dev Box with the provisioning status set to "creating". In the past what we'd do is once the operation has completed, we'd set the provisioning state to succeeded, however now that we have many different states that the Dev Box can be in, in this method:
    public ComputeSystemState GetState()
    simply setting the provisioning state to succeeded, would result in the state returning as stopped. As the initial Dev Box state retrieved from the Dev Center has an action state of "unknown" and no powerstate. This would cause us to hit this line:
    case Constants.DevBoxActionStates.Unknown:
    This causes users to think the Dev Box is stopped, when in actuality its actually running and ready to be used. When users click the sync button they would then see the actual state of the new Dev Box, which would be "running". To fix this without them having to click the sync button we simply use the OperationCallback method already within the DevBoxInstance class that takes an operation status. When the operation status is succeeded, it will query the Dev Center for its new state then invoke the state changed event to notify Dev Home of the new state. We no longer need to set the provisioning state manually.

The second thing this PR updates is allowing long running operations to be stopped when a delete action is initiated by the user. The delete action in terms of the actions we support is the only action that can be initiated while other operations are still in progress. (We're following the same pattern as devbox.microsoft.com)

  1. When a delete action is initiated we'll remove/stop any operation that we're watching within the extension, then initiate the delete operation.

I also fixed the spelling of some of the comments

Video where we see after creation the correct state is shown:

dev.box.creation.now.with.dynamic.operation.support.mp4

References and relevant issues

Detailed description of the pull request / Additional comments

related: microsoft/devhome#2732

Validation steps performed

PR checklist

@@ -76,7 +76,7 @@ private TestOptions TestOptions
""provisioningState"": ""Succeeded"",
""actionState"": ""Stopped"",
""powerState"": ""Deallocated"",
""uniqueId"": ""28feedaeb-996b-434b-bb57-4969cbeef8f0"",
""uniqueId"": ""28feedae-996b-434b-bb57-4969cbeef8f0"",

Choose a reason for hiding this comment

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

What happened here? This looks similar to the "user' guid below. If these are meant to be different, it's a good idea to generate a totally new one her to avoid confusion.

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 if you look at the values a little closer you'll see that the original is actually an invalid guid (33 characters instead of 32). I believe we didn't have code that utilized needing to read this particular value until now. So, to utilize it the guid needs to be valid thats why I dropped the extra 'b' at the end.

@@ -142,6 +142,14 @@ public bool TryGetDevBoxInstanceIfBeingCreated(string id, out DevBoxInstance? de

lock (_creationLock)
{
// The user previously chose to delete the Dev Box while it was being created. So it is no longer being
// watched and we need to remove it from the creation map.
if (!_devBoxOperationWatcher.IsIdBeingWatched(Guid.Parse(id)) && _devBoxesBeingCreated.ContainsKey(id))
Copy link
Member

Choose a reason for hiding this comment

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

No need to check if id is in the dictionary. Remove will do it for you anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh noted for next time.

@bbonaby bbonaby merged commit d90654c into main May 1, 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.

None yet

3 participants