Fix importing module remote using version filters and added tests #4900

SteveL-MSFT · 2017-09-22T18:30:25Z

added tests for remote import-module
fixed issue with remotely importing module where it was checking the version filter incorrectly against the proxy module

the filters are applied when importing the module remotely
after proxy module is generated it always has a module version of 1.0, so the filters will always fail when importing the proxy locally

Fix #4161

PaulHigin · 2017-09-22T21:29:19Z

src/System.Management.Automation/engine/Modules/ImportModuleCommand.cs

@@ -989,6 +989,10 @@ private PSModuleInfo ImportModule_LocallyViaName(ImportModuleOptions importModul
                try
                {
                    this.ArgumentList = new object[] { psSession };
+                    // since we already applied these filters remotely, we don't want to apply it locally where the generated module is always 1.0


I am a little worried about changing these object properties here permanently. It looks like this method is called within a loop of many kinds of different modules, including CIM and remote modules. I think we should change and restore in the try/finally to be safe.

This is pretty confusing and I wonder if we can add a clearer comment? Something like: "The correct module version has been imported from the remote session and created locally. The locally created module always has a version of 1.0 regardless of the actual module version imported from the remote session, and version checking is no longer needed and will not work while importing this created local module".

I suppose the right thing to do is to ensure the generated module has the same version info as the module imported from the remote session. But that is a much bigger change and can be handled in a separate issue.

I'll update the comment with your suggestion.

I initially looked to see about updating the proxy module version, but there were a couple of problems. Not only was it not trivial to update the module version, based on my reading of the code, because you can export-pssession multiple modules, you actually don't have a single module version to use so from that perspective 1.0 makes sense. Of course, we could special case single modules to have the same version number, but a bunch of plumbing code would be needed.

As far as I could tell, it seemed safe to change it, but I can store it and change it back.

PaulHigin · 2017-09-22T21:37:13Z

src/System.Management.Automation/engine/Modules/ImportModuleCommand.cs

@@ -989,6 +989,10 @@ private PSModuleInfo ImportModule_LocallyViaName(ImportModuleOptions importModul
                try
                {
                    this.ArgumentList = new object[] { psSession };


I don't understand why the PSSession is added/reverted from the ArgumentList here. The remote session has already been imported (via the Export-PSSession Cmdlet) and is no longer needed. It looks like this can be removed.

Actually this does need to be here. It is used to fill the PSSession property on the module info.

PaulHigin · 2017-09-22T21:40:37Z

test/powershell/Modules/Microsoft.PowerShell.Core/RemoteImportModule.Tests.ps1

+        @{parameter = "Prefix"             ; value = "Hello"},
+        @{parameter = "Name"               ; value = "foo","bar"},
+        @{parameter = "FullyQualifiedName" ; value = @{ModuleName='foo';RequiredVersion='0.0'},@{ModuleName='bar';RequiredVersion='1.1'}},
+        @{parameter = "Assembly"           ; script = { [System.AppDomain]::CurrentDomain.GetAssemblies() | Select-Object -First 2 }}


Can't the Assembly array be pre-computed and passed in as a "value" parameter? Then you wouldn't need the special case for "script" parameter.

My thinking is that if we add new test cases here, we can reuse script even though it's only used once here. Also, I don't like having variables defined at the top of the script in a BeforeAll statement if it's only used once for a specific It, prefer to have it closer to the code where it's being used.

SteveL-MSFT · 2017-09-23T00:08:47Z

@PaulHigin feedback addressed

PaulHigin · 2017-09-25T16:25:35Z

src/System.Management.Automation/engine/Modules/ImportModuleCommand.cs

+                    BaseMinimumVersion = null;
+                    BaseMaximumVersion = null;
+                    BaseRequiredVersion = null;
+                    try


You can just use the existing try/catch (for this.ArgumentList). I don't think a new one is needed.

Yes, didn't notice that. Will fix.

PaulHigin

LGTM

daxian-dbw · 2017-09-25T21:24:46Z

@SteveL-MSFT Can you run git submodule update? The PR has changs to Pester files.

SteveL-MSFT · 2017-09-25T21:44:18Z

@daxian-dbw Fixed.

daxian-dbw · 2017-09-25T23:23:15Z

@SteveL-MSFT there are 2 test failures in AppVeyor run. Maybe they are not related to your changes but can you please take a look?

daxian-dbw · 2017-09-25T23:24:34Z

Oh, and it seems the line 3200 and 3201 are rendered as red incorrectly :)

Describing Validate Invoke-WebRequest and Invoke-RestMethod -InFile
   Context InFile parameter negative tests

SteveL-MSFT · 2017-09-25T23:59:47Z

@daxian-dbw webcmdlets are failing on two tests and it's affecting two of my PRs, trying a rebase to see if changes in master fixed something

added tests for remote import-module fixed issue with remotely importing module where it was checking the version filter incorrectly against the proxy module - the filters are applied when importing the module remotely - after proxy module is generated it always has a module version of 1.0, so the filters will always fail when importing the proxy locally

address PR feedback from Paul

daxian-dbw · 2017-09-26T04:35:22Z

@SteveL-MSFT I don't think those 2 failing tests are related to changes in this PR. I will merge it.

SteveL-MSFT assigned daxian-dbw Sep 22, 2017

SteveL-MSFT requested a review from PaulHigin September 22, 2017 18:30

SteveL-MSFT requested review from BrucePay, daxian-dbw, lzybkr and vors as code owners September 22, 2017 18:30

msftclas added the cla-not-required label Sep 22, 2017

PaulHigin suggested changes Sep 22, 2017

View reviewed changes

vors removed their request for review September 25, 2017 00:39

PaulHigin reviewed Sep 25, 2017

View reviewed changes

SteveL-MSFT force-pushed the import-module-remote branch from 40c9039 to f6c32e4 Compare September 25, 2017 17:14

PaulHigin approved these changes Sep 25, 2017

View reviewed changes

SteveL-MSFT force-pushed the import-module-remote branch from f6c32e4 to ccb7a9e Compare September 25, 2017 21:43

SteveL-MSFT added 3 commits September 25, 2017 17:01

address PR feedback to store and revert the changed parameter values

4f28982

[feature]

c6c2f96

address PR feedback from Paul

SteveL-MSFT force-pushed the import-module-remote branch from ccb7a9e to c6c2f96 Compare September 26, 2017 00:01

daxian-dbw merged commit efbdea5 into PowerShell:master Sep 26, 2017

SteveL-MSFT deleted the import-module-remote branch September 26, 2017 04:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix importing module remote using version filters and added tests #4900

Fix importing module remote using version filters and added tests #4900

Fix importing module remote using version filters and added tests #4900

Fix importing module remote using version filters and added tests #4900

Conversation

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

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