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

Sergey Vasin (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.

Andrew (anmenaga) · 2018-08-01T20:47:58Z

            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?

Andrew (anmenaga) · 2018-08-01T20:50:59Z

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

Steve Lee (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.

Sergey Vasin (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?

Steve Lee (SteveL-MSFT) · 2018-08-14T20:14:42Z

Sergey Vasin (@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.

Rob Holt (rjmholt)

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

Thanks for your contribution Sergey Vasin (@sethvs)!

Rob Holt (rjmholt) · 2018-08-15T02:13:55Z

            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

Rob Holt (rjmholt) · 2018-08-15T02:26:36Z

  <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}).

Rob Holt (rjmholt) · 2018-08-15T02:29:23Z

        (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

Rob Holt (rjmholt) · 2018-08-15T02:38:58Z

Steve Lee (@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.

Rob Holt (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?

Sergey Vasin (sethvs) · 2018-08-15T12:05:10Z

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

Steve Lee (SteveL-MSFT) · 2018-08-21T00:20:19Z

Sergey Vasin (@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.

Steve Lee (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

Sergey Vasin (sethvs) · 2018-09-05T16:16:27Z

Close and reopen to restart tests.

Steve Lee (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.

Sergey Vasin (sethvs) requested review from Bruce Payette (BrucePay) and Dongbo Wang (daxian-dbw) as code owners July 24, 2018 11:35

Fix logic and typos.

321537c

Ilya (iSazonov) requested a review from Rob Holt (rjmholt) July 25, 2018 12:04

Ilya (iSazonov) assigned Dongbo Wang (daxian-dbw) Jul 25, 2018

Dongbo Wang (daxian-dbw) added the Area-Cmdlets-Core label Jul 25, 2018

Andrew (anmenaga) reviewed Aug 1, 2018

View reviewed changes

Steve Lee (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

Steve Lee (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

Rob Holt (rjmholt) reviewed Aug 15, 2018

View reviewed changes

Sergey Vasin (sethvs) added 2 commits August 15, 2018 14:01

Fix error message.

9b27ae8

Add test.

77241e1

Rob Holt (rjmholt) mentioned this pull request Aug 17, 2018

Version checking tests for Import-Module #7499

Merged

11 tasks

Andrew (anmenaga) approved these changes Aug 20, 2018

View reviewed changes

Steve Lee (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

Sergey Vasin (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

Sergey Vasin (sethvs) closed this Sep 5, 2018

Sergey Vasin (sethvs) reopened this Sep 5, 2018

Steve Lee (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

Sergey Vasin (sethvs) closed this Sep 6, 2018

Ryan Yates (kilasuit) added WG-Cmdlets general cmdlet issues and removed WG-Cmdlets-Core labels Jul 17, 2026

Conversation

Sergey Vasin (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!

Andrew (anmenaga) commented Aug 1, 2018

Uh oh!

Steve Lee (SteveL-MSFT) commented Aug 8, 2018

Uh oh!

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

Uh oh!

Uh oh!

Steve Lee (SteveL-MSFT) commented Aug 14, 2018

Uh oh!

Rob Holt (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!

Rob Holt (rjmholt) commented Aug 15, 2018

Uh oh!

Rob Holt (rjmholt) commented Aug 15, 2018

Uh oh!

Sergey Vasin (sethvs) commented Aug 15, 2018

Uh oh!

Steve Lee (SteveL-MSFT) commented Aug 21, 2018

Uh oh!

Steve Lee (SteveL-MSFT) commented Aug 21, 2018

Uh oh!

Sergey Vasin (sethvs) commented Sep 5, 2018

Uh oh!

Steve Lee (SteveL-MSFT) commented Sep 5, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Sergey Vasin (sethvs) commented Jul 24, 2018 •

edited

Loading

Sergey Vasin (sethvs) commented Aug 12, 2018 •

edited

Loading