PowerShell adapter PR - Githubissues

SteveL-MSFT commented 3 months ago

@mgreenegit there's 6 test failures due to the PS adapter, can you take a look and address before I re-review?

mgreenegit commented 3 months ago

Just fyi, working out a few changes in advance of starting the Windows adapter

From: Steve Lee @.> Sent: Tuesday, April 2, 2024 7:03:50 PM To: PowerShell/DSC @.> Cc: Michael Greene (POWERSHELL) @.>; Mention @.> Subject: Re: [PowerShell/DSC] PowerShell adapter PR (PR #363)

@SteveL-MSFT approved this pull request.

LGTM, @anmenagahttps://github.com/anmenaga can you review?

— Reply to this email directly, view it on GitHubhttps://github.com/PowerShell/DSC/pull/363#pullrequestreview-1975258574, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABHQMO2FKE7NTSFRT63XFQ3Y3NBONAVCNFSM6AAAAABEQ3DVPCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNZVGI2TQNJXGQ. You are receiving this because you were mentioned.Message ID: @.***>

mgreenegit commented 3 months ago

@anmenaga ready!

mgreenegit commented 3 months ago

Sounds good to me

From: Steve Lee @.> Sent: Wednesday, April 3, 2024 4:44:24 PM To: PowerShell/DSC @.> Cc: Michael Greene (POWERSHELL) @.>; Mention @.> Subject: Re: [PowerShell/DSC] PowerShell adapter PR (PR #363)

@SteveL-MSFT commented on this pull request.

In powershell-adapter/windowspowershell.resource.ps1https://github.com/PowerShell/DSC/pull/363#discussion_r1550549455:

@@ -0,0 +1,165 @@ +# Copyright (c) Microsoft Corporation.

It may make sense to separate once they start to diverge, but I think they are the same right now so it may be better to avoid confusion why there's two same scripts to maintain (like when we find bugs)

— Reply to this email directly, view it on GitHubhttps://github.com/PowerShell/DSC/pull/363#discussion_r1550549455, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABHQMO7FKIQ4FLFG3ZLQ7Y3Y3RZ3RAVCNFSM6AAAAABEQ3DVPCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSNZYGEZDEMJWGI. You are receiving this because you were mentioned.Message ID: @.***>

mgreenegit commented 3 months ago

:shipit:

mgreenegit commented 3 months ago

HOLD OFF working on windows powershell support

mgreenegit commented 3 months ago

@anmenaga @SteveL-MSFT added WindowsPowerShell adapter. ready for another review.

mgreenegit commented 3 months ago

@SteveL-MSFT @anmenaga I don't think we can work around this? https://github.com/PowerShell/DSC/actions/runs/8608080015/job/23589768568#step:4:1043

SteveL-MSFT commented 2 months ago

@anmenaga can you help @mgreenegit figure out the test failures?

anmenaga commented 2 months ago

Failing tests involve Binary resources. I was under impression that we do not declare support for those in the fist version of upcoming release of DSC? If that is the case, then these tests should be removed.

mgreenegit commented 2 months ago

The underlying reason is for v1, it needed to run winrm quickconfig. I don’t think we can do that within a build runner?

From: Andrew @.> Sent: Wednesday, April 10, 2024 1:12:34 PM To: PowerShell/DSC @.> Cc: Michael Greene (POWERSHELL) @.>; Mention @.> Subject: Re: [PowerShell/DSC] PowerShell adapter PR (PR #363)

Failing tests involve Binary resources. I was under impression that we do not declare support for those in the fist version of upcoming release of DSC? If that is the case, then these tests should be removed.

— Reply to this email directly, view it on GitHubhttps://github.com/PowerShell/DSC/pull/363#issuecomment-2048358962, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABHQMO2TFNYH3ZHKED4FTD3Y4WMLFAVCNFSM6AAAAABEQ3DVPCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANBYGM2TQOJWGI. You are receiving this because you were mentioned.Message ID: @.***>

anmenaga commented 2 months ago

The underlying reason is for v1, it needed to run winrm quickconfig. I don’t think we can do that within a build runner?

I don't see a technical problem with that; we can run anything within a build script; However, the real question is - do we intend to declare support for old binary resources? That will require additional work (and we already have plenty with the need to remove script-based and class resources dependency on PSDesiredStateConfiguration module/PS DSC subsystem.

mgreenegit commented 2 months ago

I didn’t know you could perform admin level actions. Thanks @anmenaga. Ready for review.

mgreenegit commented 2 months ago

Good point

From: Steve Lee @.> Sent: Thursday, April 11, 2024 9:10:44 AM To: PowerShell/DSC @.> Cc: Michael Greene (POWERSHELL) @.>; Mention @.> Subject: Re: [PowerShell/DSC] PowerShell adapter PR (PR #363)

@SteveL-MSFT requested changes on this pull request.

In build.ps1https://github.com/PowerShell/DSC/pull/363#discussion_r1561272858:

@@ -14,6 +14,10 @@ param( [switch]$SkipLinkCheck )

+if ($isWindows) {

winrm quickconfig -quiet

We should do this in the test and not the build script. Probably in a BeforeAll {} block

In powershell-adapter/Tests/TestClassResource/0.0.1/TestClassResource.psd1https://github.com/PowerShell/DSC/pull/363#discussion_r1561275438:

+GUID = 'b267fa32-e77d-48e6-9248-676cc6f2327f' + +# Author of this module +Author = 'Microsoft' + +# Company or vendor of this module +CompanyName = 'Microsoft Corporation' + +# Copyright statement for this module +Copyright = '(c) Microsoft. All rights reserved.' + +# Functions to export from this module, for best performance, do not use wildcards and do not delete the entry, use an empty array if there are no functions to export. +FunctionsToExport = @() + +# Cmdlets to export from this module, for best performance, do not use wildcards and do not delete the entry, use an empty array if there are no cmdlets to export. +CmdletsToExport = '*'

⬇️ Suggested change

-CmdletsToExport = '' +# this wildcard is required due to https://github.com/PowerShell/PSDesiredStateConfiguration/issues/101 +CmdletsToExport = ''

In powershell-adapter/Tests/powershellgroup.resource.tests.ps1https://github.com/PowerShell/DSC/pull/363#discussion_r1561276601:

     ($resources | ? {$_.Type -eq 'PSTestModule/TestPSRepository'}).Count | Should -Be 1

}

It 'Windows PowerShell adapter supports File resource' -Skip:(!$IsWindows){
$r = dsc resource list --adapter Microsoft.DSC/WindowsPowerShell
$LASTEXITCODE | Should -Be 0
$resources = $r | ConvertFrom-Json
($resources | ? {$_.Type -eq 'PSDesiredStateConfiguration/File'}).Count | Should -Be 1
}
It 'Get works on Binary "File" resource' -Skip:(!$IsWindows){
$testFile = 'c:\test.txt'

⬇️ Suggested change

$testFile = 'c:\test.txt'
$testFile = '$testdrive\test.txt'

In powershell-adapter/Tests/powershellgroup.resource.tests.ps1https://github.com/PowerShell/DSC/pull/363#discussion_r1561276840:

($resources | ? {$_.Type -eq 'PSDesiredStateConfiguration/File'}).Count | Should -Be 1

}

It 'Get works on Binary "File" resource' -Skip:(!$IsWindows){

$testFile = 'c:\test.txt'

'test' | Set-Content -Path $testFile -Force

$r = '{"DestinationPath":"' + $testFile.replace('\','\') + '"}' | dsc resource get -r 'PSDesiredStateConfiguration/File'

$LASTEXITCODE | Should -Be 0

$res = $r | ConvertFrom-Json

$res.actualState.result.properties.DestinationPath | Should -Be "$testFile"

}

It 'Get works on traditional "Script" resource' -Skip:(!$IsWindows){

$testFile = 'c:\test.txt'

⬇️ Suggested change

$testFile = 'c:\test.txt'
$testFile = '$testdrive\test.txt'

In powershell-adapter/Tests/winps_resource.dsc.yamlhttps://github.com/PowerShell/DSC/pull/363#discussion_r1561278956:

$schema: https://raw.githubusercontent.com/PowerShell/DSC/main/schemas/2023/10/config/document.json resources:

name: Get info from classic DSC resources

type: Microsoft.Windows/WindowsPowerShell

type: Microsoft.DSC/WindowsPowerShell

Since WindowsPowerShell is part of Windows, we want the typename of this resource to be Microsoft.Windows/WindowsPowerShell whereas PowerShell 7 being cross platform would be Microsoft.DSC/PowerShell

In powershell-adapter/windowspowershell.dsc.resource.jsonhttps://github.com/PowerShell/DSC/pull/363#discussion_r1561279745:

@@ -1,8 +1,12 @@ {

"manifestVersion": "1.0",

"type": "Microsoft.Windows/WindowsPowerShell",

"$schema": "https://raw.githubusercontent.com/PowerShell/DSC/main/schemas/2023/08/bundled/resource/manifest.json",

"type": "Microsoft.DSC/WindowsPowerShell",

As noted above

⬇️ Suggested change

"type": "Microsoft.DSC/WindowsPowerShell",
"type": "Microsoft.Windows/WindowsPowerShell",

In powershell-adapter/~/.config/powershell/Microsoft.PowerShell_profile.ps1https://github.com/PowerShell/DSC/pull/363#discussion_r1561281287:

@@ -0,0 +1 @@ +$env:psmodulepath = $env:psmodulepath.trimend(';')

Why is this file here? seems accidentally included?

— Reply to this email directly, view it on GitHubhttps://github.com/PowerShell/DSC/pull/363#pullrequestreview-1994621602, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABHQMOZI3GXLN74P4IYZK2LY42YYJAVCNFSM6AAAAABEQ3DVPCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSOJUGYZDCNRQGI. You are receiving this because you were mentioned.Message ID: @.***>

mgreenegit commented 2 months ago

Addressed all comments

PowerShell / DSC

PowerShell adapter PR #363