Refactor module version/GUID comparison logic by rjmholt · Pull Request #7125 · PowerShell/PowerShell

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

Ilya (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.

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

Ilya (iSazonov) · 2018-06-21T15:12:56Z

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

Andrew (anmenaga) · 2018-07-30T20:37:10Z

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

Andrew (anmenaga) · 2018-07-30T20:39:14Z

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

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

Ilya (iSazonov) · 2018-09-10T03:48:57Z

Rob Holt (@rjmholt) Please resolve the merge conflict.

…ough one method

Rob Holt (rjmholt) · 2018-09-28T22:39:17Z

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

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

Dongbo Wang (daxian-dbw) · 2018-10-12T05:13:03Z

Rob Holt (@rjmholt) Can you please summarize what the breaking change is in the PR description?

Rob Holt (rjmholt) · 2018-10-12T16:42:20Z

Ilya (@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

Dongbo Wang (daxian-dbw)

Thanks for revisit some of the optional parameters.

Ilya (iSazonov) · 2018-10-12T17:53:38Z

+        /// </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.

Andrew (anmenaga) · 2018-10-23T19:00:08Z

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

Andrew (anmenaga) · 2018-10-29T21:11:35Z

Aditya Patwardhan (@adityapatwardhan) this looks ready.

Ilya (iSazonov) · 2018-10-30T04:07:09Z

Rob Holt (@rjmholt) Please address two comments.

Aditya Patwardhan (adityapatwardhan) · 2018-10-30T17:02:20Z

Rob Holt (@rjmholt) Please address comments from Ilya (@iSazonov). After that, is seems ready to merge.

Rob Holt (rjmholt) · 2018-10-30T23:14:07Z

Aditya Patwardhan (@adityapatwardhan), Ilya (@iSazonov) Feedback addressed -- let me know if any further changes are required.

Aditya Patwardhan (adityapatwardhan) · 2018-10-30T23:27:55Z

Rob Holt (@rjmholt) please have a look at failing CI.

Aditya Patwardhan (adityapatwardhan) · 2018-10-30T23:30:50Z

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

Steve Lee (SteveL-MSFT) · 2018-10-31T01:06:31Z

Looking

Steve Lee (SteveL-MSFT) · 2018-10-31T01:24:23Z

Test fix #8152

Ilya (iSazonov) · 2018-10-31T05:01:09Z

Rob Holt (@rjmholt) Please look CodeFactor issues and fix if they in your code.

Ilya (iSazonov) · 2018-11-02T03:24:15Z

Aditya Patwardhan (@adityapatwardhan) Travis Plunk (@TravisEz13) Could you please reword the merged commit? [Breaking change] was skipped in description.

Rob Holt (rjmholt) requested review from Bruce Payette (BrucePay) and Dongbo Wang (daxian-dbw) as code owners June 21, 2018 01:16

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

Rob Holt (rjmholt) added Area-Cmdlets-Core Issue-Code Cleanup the issue is for cleaning up the code with no impact on functionality labels Jun 21, 2018

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

Dongbo Wang (daxian-dbw) self-assigned this Jul 2, 2018

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

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

Import-Module on path ignores version constraints #7496

Closed

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

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

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

[Feature] Remove rebase artefacts

a1d4707

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

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

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

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

c1141d9

…d calls

Dongbo Wang (daxian-dbw) approved these changes Oct 12, 2018

View reviewed changes

Ilya (iSazonov) reviewed Oct 12, 2018

View reviewed changes

Andrew (anmenaga) assigned Aditya Patwardhan (adityapatwardhan) Oct 29, 2018

[Feature] Rename IsModuleAlreadyLoaded

bf88fd9

Ilya (iSazonov) approved these changes Oct 31, 2018

View reviewed changes

Aditya Patwardhan (adityapatwardhan) merged commit 5d06fba into PowerShell:master Nov 1, 2018

Rob Holt (rjmholt) mentioned this pull request Nov 11, 2018

Fix incorrect name check when autoloading required modules #8218

Merged

11 tasks

Rob Holt (rjmholt) mentioned this pull request Feb 21, 2019

Refactor ImportModule commandlet #8936

Closed

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

Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026

Refactor module version/GUID comparison logic (PowerShell#7125)

375b437

Conversation

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

Ilya (iSazonov) commented Jun 21, 2018

Uh oh!

Rob Holt (rjmholt) commented Jun 21, 2018

Uh oh!

Ilya (iSazonov) commented Jun 21, 2018

Uh oh!

Andrew (anmenaga) commented Jul 30, 2018

Uh oh!

Andrew (anmenaga) commented Jul 30, 2018

Uh oh!

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

Ilya (iSazonov) commented Sep 10, 2018

Uh oh!

Rob Holt (rjmholt) commented Sep 28, 2018

Uh oh!

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

Uh oh!

Uh oh!

Dongbo Wang (daxian-dbw) commented Oct 12, 2018

Uh oh!

Rob Holt (rjmholt) commented Oct 12, 2018

Uh oh!

Dongbo Wang (daxian-dbw) left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Ilya (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!

Rob Holt (rjmholt) Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Andrew (anmenaga) commented Oct 23, 2018

Uh oh!

Andrew (anmenaga) commented Oct 29, 2018

Uh oh!

Ilya (iSazonov) commented Oct 30, 2018

Uh oh!

Aditya Patwardhan (adityapatwardhan) commented Oct 30, 2018

Uh oh!

Rob Holt (rjmholt) commented Oct 30, 2018

Uh oh!

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

Uh oh!

Uh oh!

Aditya Patwardhan (adityapatwardhan) commented Oct 30, 2018

Uh oh!

Steve Lee (SteveL-MSFT) commented Oct 31, 2018

Uh oh!

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

Uh oh!

Uh oh!

Ilya (iSazonov) commented Oct 31, 2018

Uh oh!

Ilya (iSazonov) commented Nov 2, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Rob Holt (rjmholt) commented Jun 21, 2018 •

edited by daxian-dbw

Loading

Rob Holt (rjmholt) commented Jul 30, 2018 •

edited

Loading

Rob Holt (rjmholt) commented Sep 29, 2018 •

edited

Loading

Ilya (iSazonov) Oct 12, 2018 •

edited

Loading

Aditya Patwardhan (adityapatwardhan) commented Oct 30, 2018 •

edited

Loading

Steve Lee (SteveL-MSFT) commented Oct 31, 2018 •

edited

Loading