Refactor module version/GUID comparison logic #7125

rjmholt · 2018-06-21T01:16:49Z

PR Summary

TLDR: Factors out module GUID/version checking logic to all use the same code path.

This PR routes all of the module GUID/version checking through ModuleIntrinsics.IsModuleMatchingSpec() (or a sub-method of it), so that all the version checking is consistent and DRY.

Previously the code had a number of 6-line if-conditions and cascading if-elses that all check module versioning in a slightly different way. I've tried to identify the common logic and unify it all.

Breaking Change Summary

Fix #7496
Previous to this PR, having a module loaded with one version, meant that importing a different version of that module would give you the (wrong) loaded version. This change fixes the version check so that the loaded module is only used when it is the correct version. See #7496.

Tests

Tests for this PR were already added in #7499. This PR just removes the -Pending flags from the tests around the breaking change (on the basis that the previous behaviour was a bug).

Fixes #7496.

PR Checklist

This change is

iSazonov · 2018-06-21T06:36:26Z

Also, if our testing coverage on this should be improved, I'm happy to add tests to this PR

To make a quality check of this change we are forced to do a lot of tests locally. So it makes sense to start by reviewing and extending the tests in the repo. I see that we don't actually have the tests for this code. I think we could add the new tests in new separate PR and then continue the PR. The Import-Module.Tests.ps1 file is already large so we could create new Import-Module-Versions.Tests.ps1.

rjmholt · 2018-06-21T13:41:32Z

Ah so there is an Import-Module.Tests.ps1? It’s not under /test/powershell/engine/Module like I expected. Where is it?

Yes agreed we should bulk up the tests in another PR to merge before this one. I’ve been thinking about doing that but now I know there’s a test file I will see what we already have...

iSazonov · 2018-06-21T15:12:56Z

See test\powershell\Modules\Microsoft.PowerShell.Core\ subdirectory.

anmenaga · 2018-07-30T20:37:10Z

@rjmholt It would be good to see a green [Feature] test pass result. Can you please run it again?

anmenaga · 2018-07-30T20:39:14Z

@daxian-dbw @BrucePay We need to get some traction on this; this PR is sitting without a review for more than a month now...

rjmholt · 2018-07-30T20:43:08Z

I need to write tests for this I would say. Will mark as WIP.

stale · 2018-09-09T21:48:58Z

This PR has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed if no further activity occurs within 10 days.
Thank you for your contributions.
Community members are welcome to grab these works.

iSazonov · 2018-09-10T03:48:57Z

@rjmholt Please resolve the merge conflict.

…ough one method

rjmholt · 2018-09-28T22:39:17Z

I have now rebased this to include the 774 tests I wrote

rjmholt · 2018-09-29T01:36:54Z

~~I've removed the breaking change tag since I think another PR has already made the relevant change. This PR changes no tests and passes all existing.~~

This is wrong. That test PR skips those tests. This PR should enable them.

EDIT: Tests are now enabled, and pass.

iSazonov · 2018-10-11T17:50:47Z

Fix for CI is in #7987

daxian-dbw

LGTM except for the comment about the optional parameters.

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

iSazonov · 2018-10-12T04:39:14Z

I'd expect that we add new tests because of "breaking change".

daxian-dbw · 2018-10-12T05:13:03Z

@rjmholt Can you please summarize what the breaking change is in the PR description?

rjmholt · 2018-10-12T16:42:20Z

@iSazonov Tests were added in #7499. I added the tests for the new behaviour then and have removed the -Pending flag in this PR.

…d calls

daxian-dbw

Thanks for revisit some of the optional parameters.

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

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

iSazonov · 2018-10-12T17:53:38Z

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

+        /// </summary>
+        /// <param name="alreadyLoadedModule">The already loaded module that matched the name of the module to load.</param>
+        /// <returns>True if the pre-loaded module matches all GUID and version constraints provided, false otherwise.</returns>
        internal bool IsModuleAlreadyLoaded(PSModuleInfo alreadyLoadedModule)


Seems the name should be IsAlreadyLoadedModuleMatchingConstraints. The module description should be corrected too.

This method has been in PowerShell with this name (and with this behaviour) since at least Win8. I would be happy to change it, so long as it's not going to cause any issues.

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

anmenaga · 2018-10-23T19:00:08Z

@rjmholt Please address remaining couple of minor issues that Ilya pointed out and this will be ready.

anmenaga · 2018-10-29T21:11:35Z

@adityapatwardhan this looks ready.

iSazonov · 2018-10-30T04:07:09Z

@rjmholt Please address two comments.

adityapatwardhan · 2018-10-30T17:02:20Z

@rjmholt Please address comments from @iSazonov. After that, is seems ready to merge.

rjmholt · 2018-10-30T23:14:07Z

@adityapatwardhan, @iSazonov Feedback addressed -- let me know if any further changes are required.

adityapatwardhan · 2018-10-30T23:27:55Z

@rjmholt please have a look at failing CI.

adityapatwardhan · 2018-10-30T23:30:50Z

@SteveL-MSFT The newly added test is failing. It is marked as Feature and Feature tests were not run when the PR got merged.

SteveL-MSFT · 2018-10-31T01:06:31Z

Looking

SteveL-MSFT · 2018-10-31T01:24:23Z

Test fix #8152

iSazonov · 2018-10-31T05:01:09Z

@rjmholt Please look CodeFactor issues and fix if they in your code.

iSazonov · 2018-11-02T03:24:15Z

@adityapatwardhan @TravisEz13 Could you please reword the merged commit? [Breaking change] was skipped in description.

rjmholt requested review from BrucePay and daxian-dbw as code owners June 21, 2018 01:16

rjmholt changed the title ~~Refactor module version/GUID comparison logic~~ WIP: Refactor module version/GUID comparison logic Jun 21, 2018

rjmholt added WG-Cmdlets-Core cmdlets in the Microsoft.PowerShell.Core module Issue-Code Cleanup the issue is for cleaning up the code with no impact on functionality labels Jun 21, 2018

rjmholt changed the title ~~WIP: Refactor module version/GUID comparison logic~~ Refactor module version/GUID comparison logic Jun 21, 2018

daxian-dbw self-assigned this Jul 2, 2018

rjmholt changed the title ~~Refactor module version/GUID comparison logic~~ WIP: Refactor module version/GUID comparison logic Jul 30, 2018

rjmholt mentioned this pull request Aug 10, 2018

Import-Module on path ignores version constraints #7496

Closed

rjmholt added the Breaking-Change breaking change that may affect users label Aug 10, 2018

rjmholt mentioned this pull request Aug 11, 2018

Version checking tests for Import-Module #7499

Merged

11 tasks

stale bot added the Stale label Sep 9, 2018

stale bot removed the Stale label Sep 10, 2018

Robert Holt added 2 commits September 28, 2018 15:27

[Feature] Refactor module version comparison logic so it all goes thr…

c08f151

…ough one method

Fix inverted if conditions

e56b1b4

rjmholt force-pushed the refactor-module-version-check branch from 3467201 to e56b1b4 Compare September 28, 2018 22:28

[Feature] Remove rebase artefacts

a1d4707

rjmholt changed the title ~~WIP: Refactor module version/GUID comparison logic~~ Refactor module version/GUID comparison logic Sep 28, 2018

rjmholt removed the Breaking-Change breaking change that may affect users label Sep 28, 2018

rjmholt added the Breaking-Change breaking change that may affect users label Oct 10, 2018

daxian-dbw approved these changes Oct 12, 2018

View reviewed changes

src/System.Management.Automation/engine/Modules/ModuleIntrinsics.cs Show resolved Hide resolved

[Feature] Remove parameter defaults for local module constraint metho…

c1141d9

…d calls

daxian-dbw approved these changes Oct 12, 2018

View reviewed changes

iSazonov reviewed Oct 12, 2018

View reviewed changes

anmenaga assigned adityapatwardhan Oct 29, 2018

[Feature] Rename IsModuleAlreadyLoaded

bf88fd9

iSazonov approved these changes Oct 31, 2018

View reviewed changes

adityapatwardhan merged commit 5d06fba into PowerShell:master Nov 1, 2018

rjmholt mentioned this pull request Nov 11, 2018

Fix incorrect name check when autoloading required modules #8218

Merged

11 tasks

rjmholt mentioned this pull request Feb 21, 2019

Refactor ImportModule commandlet #8936

Closed

Refactor module version/GUID comparison logic #7125

Refactor module version/GUID comparison logic #7125

Uh oh!

Conversation

rjmholt commented Jun 21, 2018 • edited by daxian-dbw Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Breaking Change Summary

Tests

PR Checklist

Uh oh!

iSazonov commented Jun 21, 2018

Uh oh!

rjmholt commented Jun 21, 2018

Uh oh!

iSazonov commented Jun 21, 2018

Uh oh!

anmenaga commented Jul 30, 2018

Uh oh!

anmenaga commented Jul 30, 2018

Uh oh!

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

Uh oh!

Uh oh!

stale bot commented Sep 9, 2018

Uh oh!

iSazonov commented Sep 10, 2018

Uh oh!

rjmholt commented Sep 28, 2018

Uh oh!

rjmholt commented Sep 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Oct 11, 2018

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

iSazonov commented Oct 12, 2018

Uh oh!

daxian-dbw commented Oct 12, 2018

Uh oh!

rjmholt commented Oct 12, 2018

Uh oh!

daxian-dbw left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rjmholt Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anmenaga commented Oct 23, 2018

Uh oh!

anmenaga commented Oct 29, 2018

Uh oh!

iSazonov commented Oct 30, 2018

Uh oh!

adityapatwardhan commented Oct 30, 2018

Uh oh!

rjmholt commented Oct 30, 2018

Uh oh!

adityapatwardhan commented Oct 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adityapatwardhan commented Oct 30, 2018

Uh oh!

SteveL-MSFT commented Oct 31, 2018

Uh oh!

SteveL-MSFT commented Oct 31, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iSazonov commented Oct 31, 2018

Uh oh!

rjmholt commented Jun 21, 2018 •

edited by daxian-dbw

Loading

rjmholt commented Jul 30, 2018 •

edited

Loading

rjmholt commented Sep 29, 2018 •

edited

Loading

iSazonov Oct 12, 2018 •

edited

Loading

adityapatwardhan commented Oct 30, 2018 •

edited

Loading

SteveL-MSFT commented Oct 31, 2018 •

edited

Loading