Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor module version/GUID comparison logic #7125

Merged

Conversation

@rjmholt
Copy link
Member

commented Jun 21, 2018

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 Reviewable

@rjmholt rjmholt requested review from BrucePay and daxian-dbw as code owners Jun 21, 2018

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

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

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2018

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

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2018

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

This comment has been minimized.

Copy link
Collaborator

commented Jun 21, 2018

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

@daxian-dbw daxian-dbw self-assigned this Jul 2, 2018

@anmenaga

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2018

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

@anmenaga

This comment has been minimized.

Copy link
Contributor

commented Jul 30, 2018

@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

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2018

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

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

@rjmholt rjmholt referenced this pull request Aug 11, 2018
8 of 11 tasks complete
@stale

This comment has been minimized.

Copy link

commented Sep 9, 2018

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.

@stale stale bot added the Stale label Sep 9, 2018

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Sep 10, 2018

@rjmholt Please resolve the merge conflict.

@stale stale bot removed the Stale label Sep 10, 2018

rjmholt added 2 commits Jun 21, 2018

@rjmholt rjmholt force-pushed the rjmholt:refactor-module-version-check branch to e56b1b4 Sep 28, 2018

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

@rjmholt

This comment has been minimized.

Copy link
Member Author

commented Sep 28, 2018

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

@rjmholt

This comment has been minimized.

Copy link
Member Author

commented Sep 29, 2018

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.

@daxian-dbw
Copy link
Member

left a comment

LGTM except for the comment about the optional parameters.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Oct 12, 2018

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

@daxian-dbw

This comment has been minimized.

Copy link
Member

commented Oct 12, 2018

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

@rjmholt

This comment has been minimized.

Copy link
Member Author

commented Oct 12, 2018

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

@daxian-dbw
Copy link
Member

left a comment

Thanks for revisit some of the optional parameters.

/// Checks if an already loaded module meets the constraints passed to the module cmdlet by the user.
/// </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)

This comment has been minimized.

Copy link
@iSazonov

iSazonov Oct 12, 2018

Collaborator

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

This comment has been minimized.

Copy link
@rjmholt

rjmholt Oct 23, 2018

Author Member

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.

@anmenaga

This comment has been minimized.

Copy link
Contributor

commented Oct 23, 2018

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

@anmenaga

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2018

@adityapatwardhan this looks ready.

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Oct 30, 2018

@rjmholt Please address two comments.

@adityapatwardhan

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

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

@rjmholt

This comment has been minimized.

Copy link
Member Author

commented Oct 30, 2018

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

@adityapatwardhan

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

@rjmholt please have a look at failing CI.

@adityapatwardhan

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

@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

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

Looking

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Oct 31, 2018

Test fix #8152

@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Oct 31, 2018

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

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

8 checks passed

CodeFactor 20 issues fixed. 16 issues found.
Details
PowerShell-CI-linux #4938 succeeded
Details
PowerShell-CI-macos #4939 succeeded
Details
PowerShell-CI-spelling #PR-7125-20181030.01 succeeded
Details
PowerShell-CI-windows #PR-7125-20181030.01 succeeded
Details
WIP Ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
license/cla All CLA requirements met.
Details
@iSazonov

This comment has been minimized.

Copy link
Collaborator

commented Nov 2, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.