search

microsoft / 2LCS

Lifecycle Services Companion App for administrators
MIT License
104 stars 55 forks source link

RDP instant mode doesn't work #93

Closed DjLysy closed 8 months ago

DjLysy commented 8 months ago

Since making the refresh environments methods working in async mode, RDP instant mode when launching the 2LCS with parameters doesn't work anymore

Example: execute the following path: .\2LCS.exe ms-2lcs://connectRDP/?projectId=(projectId)&environment=(envName)&user=Admin

It will fail on calling HttpClientHelper.GetCheInstancesAsync()

FH-Inway commented 8 months ago

Thanks for reporting the issue and providing the fix in pr #94 as well, much appreciated! I merged it and released 0.49.0.

I was not even aware that 2LCS could be used like that. Seems something that we should add to the documentation.

Just to provide context for the future, this functionality was added in #66, while the async stuff was added in #75

@fraga Let us know if you see any issues in undoing async for RDPConnect.

fraga commented 8 months ago

Hey this is great!

What if we add the sync version as it was and just wrap that in async instead of just removing the async? Let me know your thoughts.

FH-Inway commented 8 months ago

Could you explain a bit more on how this could be done? I'm a bit out of my depth with C# and async.

From what I can tell, since BackgroundWorker is used to call OnWorkerDoWork, it already runs asynchronously?

https://github.com/microsoft/2LCS/blob/4b89a4bad0ed9b5ed7186f28b2b2f0d42264aac5/2LCS/Forms/RDPConnect.cs#L57-L73

fraga commented 8 months ago

Never mind I overlooked the implementation and when I did the async piece that should've left as sync and we would still use the Result as @DjLysy implemented is correct.

Looking forward, I think we need to refactor the backgroundworker and replace it with Task. Since this is all probably going to go away with One experience (one admin) I would leave it for now!

Thanks @DjLysy for the contribution!

PS: I'm always out of my depth with C# too, always learning!

FH-Inway commented 8 months ago

@fraga Thanks for looking into it. I agree, the LCS days are numbered and so is 2LCS. But while we still have it, it is work keeping it alive. Will keep the Task refactor in mind in case we revisit that part again.