Added new VSCodeInsiders class with Pester tests #60
Conversation
| # Keeps the path of the code.exe CLI path. | ||
| $VSCodeCLIPath = Get-VSCodeCLIPath | ||
|
|
There was a problem hiding this comment.
I think it might be better to add a boolean parameter like $UseInsiders instead of creating a completely new DSC resource. That way we don't have to maintain two resources that are essentially doing the same thing.
You can use the boolean parameter inside the static constructor to check if $UseInsiders = $true and set $VSCodeCLIPath with the correct insiders code.exe path. So basically:
if $UseInsiders
{
$VSCodeCLIPath = Get-VSCodeInsidersCLIPath
}There was a problem hiding this comment.
@ryfu-msft Thanks for the review. This is exactly what I wanted to do in the first place, but couldn't grasp my head around it. My class-based PowerShell development is a bit frisky.
Wouldn't it require to add an additional property in the class, allowing the ability to choose between Insiders or Stable?
There was a problem hiding this comment.
Yup, you would need to add another boolean DSCProperty and use that to determine whether we point to the Insiders or Stable VSCodeCLI path.
There was a problem hiding this comment.
@ryfu-msft Hey Ryan, I've rewritten the Get-VSCodeCLIPath (also started on the custom path one from #61 ). I'm struggling with how to get it initialized. Whenever Initialize is called, a loop occurs. I'm seeing something over the head. Do you mind taking a look?
There was a problem hiding this comment.
After struggling for some time now, I reconstructed the constructors. Because of the following logic when calling Invoke-DscResource:
The call is made later to determine the CLI path. The export has an overload added. It's good to check if this is possible with dsc.exe I guess.
| { | ||
| [VSCodeExtension]::GetInstalledExtensions() | ||
| VSCodeExtension() { | ||
| } |
There was a problem hiding this comment.
I'm okay with moving GetInstalledExtensions() to the get method. Since we are no longer using a static constructor, this can be removed.
There was a problem hiding this comment.
@ryfu-msft I didn't totally get what you mean what is required to be removed.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Can you run the pipeline once more? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Please rename this to Microsoft.VSCode.Dsc.Tests.ps1
Sorry Ryan, was struggling with the Pester version and getting it running. Should be fixed now. Can you run it once more? |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
And you thanks for the checks. Looking forward to seeing the custom path coming :) |
Addresses issue #59