-
Notifications
You must be signed in to change notification settings - Fork 18
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
Bug fix for daprd proc bug #169 #185
Conversation
@@ -99,12 +100,23 @@ export default class ProcessBasedDaprApplicationProvider extends vscode.Disposab | |||
} | |||
|
|||
private async onRefresh(): Promise<void> { | |||
const processes = await this.processProvider.listProcesses('daprd'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any fix probably ought to be incorporated within the ProcessProvider
, if anything, because the ps-list
library doesn't generate a cmd
property on Windows so this fix wouldn't work on that platform. To a certain extent, ProcessProvider
exists to hide the switching between ps-list
on Linux/Mac and wmic
on Windows.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seemed to work when I tested it on Windows, but it's possible I was using a Linux subsystem and that's why it worked. But I'll move the changes to the ProcessProvider to keep the implementation wrapped in that class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, using VS Code with WSL would end up using the Linux codepaths. I use Parallels on my Mac in order to setup both a Windows and Linux VM, so that I can easily test across the different platforms. It's amazing how many quirks one runs into between Mac and Linux, for example, despite the similarities.
…dapr into daprdProcBug-microsoft#169 merging main into branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of last comments, then it looks ok.
Bug fix for #169. Updated application finder to locate all daprd processes (run by either the dapr cli or through daprd directly).
@philliphoff @philkedy