Publish-PSResource: Pack and Push Feature #1682

jshigetomi · 2024-07-31T19:33:15Z

PR Summary

Decomposed Publish-PSResource to Pack and Push model with an addition of Compress-PSResource command.

Compress-PSResource requires a path to a PS module/script and a destination path to save the resulting nupkg.

Added -Nupkg parameter to Publish-PSResource that pushes the .nupkg file to a specified repository.

Publish-PSResource retains old functionality

PR Context

This change is to meet an SFI2 requirement to ensure signed packages. This PR enables publishing signed packages through Publish-PSResource by first packing (Compress-PSResource) then signing (User's Choice) then pushing(Publish-PSResource -Nupkg .

PR Checklist

alerickson

Looks really good, just made some small suggestions here and there.

test/PublishPSResourceTests/PublishPSResource.Tests.ps1

test/PublishPSResourceTests/CompressPSResource.Tests.ps1

src/code/CompressPSResource.cs

src/code/PSResourceHelper.cs

alerickson · 2024-08-06T01:38:48Z

src/code/PSResourceHelper.cs

+            }
+            finally
+            {
+                if(_callerCmdlet == CallerCmdlet.CompressPSResource)


I think we always want to delete outputDir when we're done with it

The CompressPSResource path deletes it here. But the PublishPSResource path still needs it for push to preserve the previous functionality. The PushResource method finally block handles that.

alerickson · 2024-08-06T01:48:29Z

src/code/PSResourceHelper.cs

+                {
+                    if(_isNupkgPathSpecified)
+                    {
+                        outputNupkgDir = pathToNupkgToPublish;


Also consider having ouputNupkgDir as a parameter that's passed in to PushResource()

passing outputNupkgDir makes it difficult to maintain old PublishResource Behavior. All the outputNupkgDir logic needs to be done in Publish and not in helper and that means in Compress as well.

src/code/PSResourceHelper.cs

alerickson · 2024-08-06T23:07:50Z

/azp run PowerShell.PSResourceGet

azure-pipelines · 2024-08-06T23:08:00Z

Azure Pipelines successfully started running 1 pipeline(s).

adityapatwardhan · 2024-08-13T17:23:16Z

Closing and re-opening to re-trigger CI

adityapatwardhan · 2024-08-13T17:24:32Z

/azp run PowerShell.PSResourceGet

azure-pipelines · 2024-08-13T17:24:43Z

Azure Pipelines successfully started running 1 pipeline(s).

alerickson · 2024-08-20T16:59:08Z

/azp run PowerShell.PSResourceGet

azure-pipelines · 2024-08-20T16:59:22Z

Azure Pipelines successfully started running 1 pipeline(s).

jshigetomi · 2024-08-20T18:56:44Z

Fixed a test that was strangely passing on local machine but failing on git.

The problem was with

Get-ChildItem $script:repositoryPath2 | Should -Be $expectedPath
changed to
(Get-ChildItem $script:repositoryPath2).FullName | Should -Be $expectedPath

alerickson · 2024-08-20T20:20:21Z

/azp run PowerShell.PSResourceGet

azure-pipelines · 2024-08-20T20:20:32Z

Azure Pipelines successfully started running 1 pipeline(s).

alerickson · 2024-08-20T20:24:11Z

test/PublishPSResourceTests/PublishPSResource.Tests.ps1

@@ -567,6 +567,8 @@ Describe "Test Publish-PSResource" -tags 'CI' {

        $expectedPath = Join-Path -Path $script:repositoryPath  -ChildPath "$scriptName.$scriptVersion.nupkg"
        (Get-ChildItem $script:repositoryPath).FullName | Should -Be $expectedPath
+
+        Remove-Item -Path $scriptPath


This should probably be moved into the "After All" or "After Each" (whichever is appropriate), just to isolate the test functionality itself and have clean up done outside of test

This test doesn't delete the test.ps1 file it creates. It only happen for this test.
Maybe it can be like the first test where it removes it in the test?

It "Publish module with required module not installed on the local machine using -SkipModuleManifestValidate" { # Skip the module manifest validation test, which fails from the missing manifest required module. $testModulePath = Join-Path -Path $TestDrive -ChildPath ModuleWithMissingRequiredModule Publish-PSResource -Path $testModulePath -Repository $testRepository2 -Confirm:$false -SkipDependenciesCheck -SkipModuleManifestValidate $expectedPath = Join-Path -Path $script:repositoryPath2 -ChildPath 'ModuleWithMissingRequiredModule.1.0.0.nupkg' $publishedModuleFound = Test-Path -Path $expectedPath $publishedModuleFound | Should -BeTrue if ($publishedModuleFound) { Remove-Item $expectedPath -Force -ErrorAction SilentlyContinue } }

Oh, sorry I was just looking at the line kind of out of context. Really the entire line should be removed because $scriptPath will be deleted during clean up in the "After Each" block already:

PSResourceGet/test/PublishPSResourceTests/PublishPSResource.Tests.ps1

Line 108 in 0b4d77f

$pkgsToDelete = Join-Path -Path "$script:repositoryPath" -ChildPath "*"

alerickson · 2024-08-20T20:56:56Z

src/code/PublishHelper.cs

+
+        private bool PackNupkg(string outputDir, string outputNupkgDir, string nuspecFile, out ErrorRecord error)
+        {
+            _cmdlet.WriteDebug($"In {_callerCmdlet}::PackNupkg()");


These debug messages should specify "PublishHelper" instead of "_callerCmdlet" to stay consistent with how we're using the debug/verbose messages elsewhere in the repo

done. Should I also change the errors as well like...

_cmdlet.WriteError(new ErrorRecord( new ArgumentException($"The resource repository '{Repository}' is not a registered. Please run 'Register-PSResourceRepository' in order to publish to this repository."), "RepositoryNotFound", ErrorCategory.ObjectNotFound, this._cmdlet)); return;

should this._cmdlet be instead this? that refers to the PublishHelper?

You can just leave it as this without the .cmdlet (eg:

PSResourceGet/src/code/FindHelper.cs

Line 110 in e79f6a2

_cmdletPassedIn.WriteError(new ErrorRecord(

). And looking at that example, could you change _cmdlet to _cmdletPassedIn? Staying consistent with the naming convention just helps us in the future if there's changes we need to make to the entire code base. It's easier to search and replace one term instead of looking up the different permutations

alerickson · 2024-08-21T17:17:30Z

/azp run PowerShell.PSResourceGet

azure-pipelines · 2024-08-21T17:17:43Z

Azure Pipelines successfully started running 1 pipeline(s).

alerickson · 2024-08-21T19:13:00Z

/azp run PowerShell.PSResourceGet

azure-pipelines · 2024-08-21T19:13:10Z

Azure Pipelines successfully started running 1 pipeline(s).

alerickson · 2024-08-21T20:09:51Z

/azp run PowerShell.PSResourceGet

azure-pipelines · 2024-08-21T20:10:01Z

Azure Pipelines successfully started running 1 pipeline(s).

alerickson · 2024-08-21T20:52:12Z

/azp run PowerShell.PSResourceGet

azure-pipelines · 2024-08-21T20:52:24Z

Azure Pipelines successfully started running 1 pipeline(s).

Co-authored-by: alerickson <25858831+alerickson@users.noreply.github.com>

…press

Changed debug messages in PublishHelper

changed this._cmdlet -> _cmdletPassedIn

adityapatwardhan · 2024-08-22T15:52:10Z

/azp run PowerShell.PSResourceGet

azure-pipelines · 2024-08-22T15:52:22Z

Azure Pipelines successfully started running 1 pipeline(s).

adityapatwardhan · 2024-08-22T16:12:18Z

/azp run PowerShell.PSResourceGet

azure-pipelines · 2024-08-22T16:12:28Z

Azure Pipelines successfully started running 1 pipeline(s).

adityapatwardhan · 2024-08-22T19:31:04Z

/azp run PowerShell.PSResourceGet

azure-pipelines · 2024-08-22T19:31:17Z

Azure Pipelines successfully started running 1 pipeline(s).

…urceGet

alerickson · 2024-08-23T18:43:16Z

/azp run PowerShell.PSResourceGet

azure-pipelines · 2024-08-23T18:43:27Z

Azure Pipelines successfully started running 1 pipeline(s).

jshigetomi changed the title ~~Publish-PSResource: Pack and Push eature~~ Publish-PSResource: Pack and Push Feature Jul 31, 2024

jshigetomi marked this pull request as draft July 31, 2024 19:33

jshigetomi marked this pull request as ready for review August 2, 2024 06:44

alerickson requested changes Aug 6, 2024

View reviewed changes

adityapatwardhan closed this Aug 13, 2024

adityapatwardhan reopened this Aug 13, 2024

jshigetomi requested a review from alerickson August 16, 2024 20:35

alerickson reviewed Aug 20, 2024

View reviewed changes

jshigetomi requested a review from alerickson August 21, 2024 14:49

alerickson approved these changes Aug 21, 2024

View reviewed changes

jshigetomi force-pushed the issue-1635-PSResourceGet branch from 225603b to 2bebc4f Compare August 21, 2024 20:42

jshigetomi and others added 17 commits August 22, 2024 10:24

Update src/code/CompressPSResource.cs

eab5a31

Co-authored-by: alerickson <25858831+alerickson@users.noreply.github.com>

alias to cmres

505b4ec

_psResourceHelper -> _publishHelper

f983374

Added nupkg as a ResourceType

3bc1f1b

initial changes

f57f4cc

Added PSResourceHelper class to share methods between Publish and Com…

c8b2d5a

…press

some test cases pass, still need to fix how it handles scripts

1af00dd

some more test cases passed, still a WIP

549f8dc

All test cases passed

7849888

Addressed issues brought up by @alerickson

f8b8e51

Update PublishPSResource.Tests.ps1

2c2060c

Fixed an ambigous test that failed on git but passed on local machine

259da8c

Addressed issues brought up by @alerickson.

6927585

Changed debug messages in PublishHelper

changed _cmdlet -> _cmdletPassedIn

c4dd691

changed this._cmdlet -> _cmdletPassedIn

cleaned up a test

8c7e37d

deleted extra files by mistake

ea3a5f2

adding .suo back

e869ddf

deleted test file

a537e23

jshigetomi force-pushed the issue-1635-PSResourceGet branch from d76c50b to a537e23 Compare August 22, 2024 16:11

Justin Chung added 2 commits August 23, 2024 13:32

Merge remote-tracking branch 'upstream/master' into issue-1635-PSReso…

935c2c7

…urceGet

rebased with find-repo fix

28b3cbd

adityapatwardhan merged commit e9e8ecb into PowerShell:master Aug 23, 2024
12 checks passed

Publish-PSResource: Pack and Push Feature #1682

Publish-PSResource: Pack and Push Feature #1682

Conversation

jshigetomi commented Jul 31, 2024 • edited Loading

PR Summary

PR Context

PR Checklist

alerickson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alerickson commented Aug 6, 2024

azure-pipelines bot commented Aug 6, 2024

adityapatwardhan commented Aug 13, 2024

adityapatwardhan commented Aug 13, 2024

azure-pipelines bot commented Aug 13, 2024

alerickson commented Aug 20, 2024

azure-pipelines bot commented Aug 20, 2024

jshigetomi commented Aug 20, 2024

alerickson commented Aug 20, 2024

azure-pipelines bot commented Aug 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jshigetomi Aug 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alerickson commented Aug 21, 2024

azure-pipelines bot commented Aug 21, 2024

alerickson commented Aug 21, 2024

azure-pipelines bot commented Aug 21, 2024

alerickson commented Aug 21, 2024

azure-pipelines bot commented Aug 21, 2024

alerickson commented Aug 21, 2024

azure-pipelines bot commented Aug 21, 2024

adityapatwardhan commented Aug 22, 2024

azure-pipelines bot commented Aug 22, 2024

adityapatwardhan commented Aug 22, 2024

azure-pipelines bot commented Aug 22, 2024

adityapatwardhan commented Aug 22, 2024

azure-pipelines bot commented Aug 22, 2024

alerickson commented Aug 23, 2024

azure-pipelines bot commented Aug 23, 2024

jshigetomi commented Jul 31, 2024 •

edited

Loading

jshigetomi Aug 21, 2024 •

edited

Loading