-
Notifications
You must be signed in to change notification settings - Fork 8.1k
PowerShellGet: Change AllUsers scope install location to SHARED_MODULES path #5633
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
Conversation
b8d02ce to
a01f5e4
Compare
daxian-dbw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Please add [feature] tag to the commit message to trigger a full test run.
21f0dc4 to
4426696
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't really have a way to handle this if you don't have the privilege to write in the directory on non-windows platforms, we can look at extending the test framework for next release.
I recommend a test hook approach, which allows you to provide an alternative location. From a test perspective, the only thing you want to be sure of is when you select a location the files get placed there. It doesn't really matter what that location is, it could be anywhere - A testhook which makes [system.management.automation.platform]::SelectProductNameForDirectory('SHARED_MODULES') point to $TESTDRIVE should be sufficient as long as you have a separate test which returns what the default location of SHARED_MODULES should be.
This provides far more flexibility in the long run
If you can't do that, I would mark this specific test as pending for non-windows
4426696 to
9ea2d3d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use -Pending instead of -Skip. We track all pending tests because they are supposed to work but now at the time. -Skip means they are not applicable to the platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And also, please add -Pending before the open curly bracket, like
It "Should install a module correctly to the required location with default AllUsers scope" -Pending:$IsLinux {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment here.
9ea2d3d to
bc24958
Compare
|
In fact, with As for macOS, it turns out the Shared-Module path is not protected, and that's why it passed on macOS. |
|
@adityapatwardhan Can you take a look at the failure in AppVeyor to see what might be wrong? https://ci.appveyor.com/project/PowerShell/powershell/build/v6.1.0-preview.7209 |
|
@bmanikm You didn't address my comment at #5633 (comment) |
7ff19d1 to
6f9e8c9
Compare
…HARED_MODULES location. Refresh PowerShellGet.Tests.ps1 from PowerShell/PowerShellGet repository. PowerShell/PowerShellGet#175
6f9e8c9 to
76fccd5
Compare
…AllUsers scope. (PowerShell#5633) Changed the install location of AllUsers scope on PWSH to SHARED_MODULES location.
…AllUsers scope. (#5633) Changed the install location of AllUsers scope on PWSH to SHARED_MODULES location.
PR Checklist
Note: Please mark anything not applicable to this PR
NA.[feature]if the change is significant or affectes feature testsPR Summary