Throw error message when MaxumimVersion is less than Version when using Import-Module cmdlet with -FullyQualifiedName parameter. by sethvs · Pull Request #7347 · PowerShell/PowerShell

sethvs · 2018-07-24T11:35:18Z

PR Summary

Also fixing following error message to include 'MaximumVersion' in the list of 'valid members':

  <data name="InvalidModuleSpecificationMember" xml:space="preserve">
    <value>The hashtable describing a module contains one or more members that are not valid.  The valid members are ({0}). Remove the members that are not valid ({1}), then try again.</value>
  </data>

Concerning if the change is breaking, I would refer it to Unlikely Grey Area.

PR Checklist

…ng Import-Module cmdlet with -FullyQualifiedName parameter.

anmenaga · 2018-08-01T20:47:58Z

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

            if (badKeys.Length != 0)
            {
-                message = StringUtil.Format(Modules.InvalidModuleSpecificationMember, "ModuleName, ModuleVersion, RequiredVersion, GUID", badKeys);
+                message = StringUtil.Format(Modules.InvalidModuleSpecificationMember, "ModuleName, ModuleVersion, MaximumVersion, RequiredVersion, GUID", badKeys);


Please add to PR Summary:

Also fixing following error message to include 'MaximumVersion' in the list of 'valid members': <data name="InvalidModuleSpecificationMember" xml:space="preserve"> <value>The hashtable describing a module contains one or more members that are not valid. The valid members are ({0}). Remove the members that are not valid ({1}), then try again.</value> </data>

Ideally this should use $"{nameof(...)}" for the bad members

nameof(badKeys) returns 'badKeys', but we need its actual value.
Or am I missing something?

anmenaga · 2018-08-01T20:50:59Z

@daxian-dbw @SteveL-MSFT please evaluate if this technically breaking (however unlikely) change has to go to PS Committee for review.

SteveL-MSFT · 2018-08-08T22:16:13Z

@PowerShell/powershell-committee agrees this is a breaking change. For example using this in a pipeline would terminate the pipeline now whereas a $null being returned would not execute the rest of the pipeline. We agree that this should have been an error and would accept returning a non-terminating error and not a terminating one.

sethvs · 2018-08-12T11:10:27Z

Can you guide me a little bit?
How can I use WriteError in there?

PowerShell/src/System.Management.Automation/engine/Modules/ModuleSpecification.cs

Lines 167 to 172 in 321537c

    
           if (moduleSpecification.Version != null && moduleSpecification.MaximumVersion != null && 
        
               moduleSpecification.Version > ModuleCmdletBase.GetMaximumVersion(moduleSpecification.MaximumVersion)) 
        
           { 
        
               message = StringUtil.Format(Modules.ModuleSpecificationMemberIsLessThanOther, "MaximumVersion", "ModuleVersion"); 
        
               return new ArgumentException(message); 
        
           }

Or is there another way to return a non-terminating error?

SteveL-MSFT · 2018-08-14T20:14:42Z

@sethvs I took a look at the code. Since the validation is in the ModuleSpecification class that should continue to return an exception that gets thrown. You'll probably need to update GetModuleCommand.cs to handle this specific case.

rjmholt

Apologies, I should have reviewed this sooner. Looks like a good change! Just left a couple of minor comments.

Thanks for your contribution @sethvs!

rjmholt · 2018-08-15T02:13:55Z

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

            if (badKeys.Length != 0)
            {
-                message = StringUtil.Format(Modules.InvalidModuleSpecificationMember, "ModuleName, ModuleVersion, RequiredVersion, GUID", badKeys);
+                message = StringUtil.Format(Modules.InvalidModuleSpecificationMember, "ModuleName, ModuleVersion, MaximumVersion, RequiredVersion, GUID", badKeys);


Ideally this should use $"{nameof(...)}" for the bad members

rjmholt · 2018-08-15T02:26:36Z

src/System.Management.Automation/resources/Modules.resx

  <data name="InvalidModuleSpecificationMember" xml:space="preserve">
    <value>The hashtable describing a module contains one or more members that are not valid.  The valid members are ({0}). Remove the members that are not valid ({1}), then try again.</value>
  </data>
+  <data name="ModuleSpecificationMemberIsLessThanOther" xml:space="preserve">


The error message should ideally be slightly more specific. Something like:

The hashtable describing a module cannot have its ModuleVersion ({0}) greater than its MaximumVersion ({1}).

rjmholt · 2018-08-15T02:29:23Z

test/powershell/Modules/Microsoft.PowerShell.Core/Import-Module.Tests.ps1

        (Get-Module TestModule).Version | Should -BeIn "1.1"
    }
+
+    It "should throw if 'MaximumVersion' is less than 'Version' when using -FullyQualifiedName parameter" {


In addition to this test, we should add a more specific one using the [ModuleSpecification] constructor.

You should find those tests here (I know that's not an obvious place at all).

Note to self: once #7499 is merged, this test will be included in that.

Let me know if you need any help on creating that test btw, I'm always happy to

rjmholt · 2018-08-15T02:38:58Z

@SteveL-MSFT is it possible for a parameter binding exception to be non-terminating? The error when called with Get-Module will occur when the hashtable is cast to a ModuleSpecification; execution will never reach Get-Module.

rjmholt · 2018-08-15T02:41:57Z

Also for reference, here is Import-Module's version check:

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

Lines 1686 to 1690 in c94fc31

    
           if (BaseMaximumVersion != null && BaseMinimumVersion != null && BaseMaximumVersion < BaseMinimumVersion) 
        
           { 
        
               string message = StringUtil.Format(Modules.MinimumVersionAndMaximumVersionInvalidRange, BaseMinimumVersion, BaseMaximumVersion); 
        
               throw new PSArgumentOutOfRangeException(message); 
        
           }

I think that's a terminating error, no?

sethvs · 2018-08-15T12:05:10Z

Is this OK with VSTS: PowerShell-CI-macos?

SteveL-MSFT · 2018-08-21T00:20:19Z

@sethvs the VSTS runs are currently not blocking for merging, we're still working on improving them. They currently run in parallel to allow us to validate it's going to work for us.

SteveL-MSFT · 2018-08-21T00:21:21Z

In light of recent discussions about returning nothing vs an error. I'd like @PowerShell/powershell-committee to re-review this

sethvs · 2018-09-05T16:16:27Z

Close and reopen to restart tests.

SteveL-MSFT · 2018-09-05T22:58:04Z

@PowerShell/powershell-committee re-reviewed this. The conclusion is that well-formed filters should return nothing if the result set is nothing and not return an error. In this case, the filter is well-formed, although invalid. Since this is also a breaking change, our recommendation is to not take this change as it's inconsistent with the desired filtering behavior and introduces a breaking change.

Throw error message when MaxumimVersion is less than Version when usi…

4d0da14

…ng Import-Module cmdlet with -FullyQualifiedName parameter.

sethvs requested review from BrucePay and daxian-dbw as code owners July 24, 2018 11:35

Fix logic and typos.

321537c

iSazonov requested a review from rjmholt July 25, 2018 12:04

iSazonov assigned daxian-dbw Jul 25, 2018

daxian-dbw added the WG-Cmdlets-Core cmdlets in the Microsoft.PowerShell.Core module label Jul 25, 2018

anmenaga reviewed Aug 1, 2018

View reviewed changes

SteveL-MSFT added Review - Committee The PR/Issue needs a review from the PowerShell Committee Breaking-Change breaking change that may affect users labels Aug 2, 2018

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Aug 8, 2018

rjmholt reviewed Aug 15, 2018

View reviewed changes

sethvs added 2 commits August 15, 2018 14:01

Fix error message.

9b27ae8

Add test.

77241e1

rjmholt mentioned this pull request Aug 17, 2018

Version checking tests for Import-Module #7499

Merged

11 tasks

anmenaga approved these changes Aug 20, 2018

View reviewed changes

SteveL-MSFT added Review - Committee The PR/Issue needs a review from the PowerShell Committee and removed Committee-Reviewed PS-Committee has reviewed this and made a decision labels Aug 21, 2018

sethvs added 2 commits August 30, 2018 10:55

Split nonrelated fixes into separate PRs.

fdfe12e

Merge branch 'master' into importModule

3f4d778

Fix error introduced while resolving conflict.

1a784a4

sethvs closed this Sep 5, 2018

sethvs reopened this Sep 5, 2018

SteveL-MSFT added Committee-Reviewed PS-Committee has reviewed this and made a decision and removed Review - Committee The PR/Issue needs a review from the PowerShell Committee labels Sep 5, 2018

sethvs closed this Sep 6, 2018

Conversation

sethvs commented Jul 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anmenaga commented Aug 1, 2018

Uh oh!

SteveL-MSFT commented Aug 8, 2018

Uh oh!

sethvs commented Aug 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SteveL-MSFT commented Aug 14, 2018

Uh oh!

rjmholt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmholt commented Aug 15, 2018

Uh oh!

rjmholt commented Aug 15, 2018

Uh oh!

sethvs commented Aug 15, 2018

Uh oh!

SteveL-MSFT commented Aug 21, 2018

Uh oh!

SteveL-MSFT commented Aug 21, 2018

Uh oh!

sethvs commented Sep 5, 2018

Uh oh!

SteveL-MSFT commented Sep 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

sethvs commented Jul 24, 2018 •

edited

Loading

sethvs commented Aug 12, 2018 •

edited

Loading