Avoid unnecessary path resolution for `Get-Acl -LiteralPath` #13107

Shriram0908 · 2020-07-05T10:33:00Z

PR Summary

Fix #11566
Removes unnecessary path resolution when using LiteralPath parameter in Get-Acl cmdlet.
Add pester test for Get-Acl cmdlet

PR Context

This PR will resolve the bug reported in #11566. The PR will also reduce the calls made. Adding pester tests to help easier refactoring and to avoid regression.

PR Checklist

rjmholt · 2020-07-07T17:20:54Z

Converted this PR to draft to match the WIP tag. Please feel free to change it back when ready

rjmholt · 2020-07-07T17:24:03Z

test/powershell/Modules/Microsoft.PowerShell.Security/AclCmdlets.Tests.ps1

@@ -1,10 +1,64 @@
 # Copyright (c) Microsoft Corporation.
 # Licensed under the MIT License.
 Describe "Acl cmdlets are available and operate properly" -Tag CI {
-    It "Get-Acl returns an ACL object" -Pending:(!$IsWindows) {
+    It "Get-Acl returns an ACL DirectorySecurity object" -Pending:(!$IsWindows) {


This might be better as -Skip if the ACL cmdlets are Windows-specific -- otherwise they may be pending for a long time.

Two other thoughts:

I can't remember if there's a mechanism to skip a whole describe, but that might be easier here

General thought: we should require some kind of in-code record of why a test is skipped or pending so that we can later re-evaluate

Pester v5 can skip a whole Describe, but there is a lot of work to refactor tests to make things work in there.

I think the Pester module used here is still not at v5.

Executing all tests in '.\test\powershell\Modules\Microsoft.PowerShell.Security\AclCmdlets.Tests.ps1' with Tags CI', 'Feature Executing script .\test\powershell\Modules\Microsoft.PowerShell.Security\AclCmdlets.Tests.ps1 [-] Error occurred in test script '.\test\powershell\Modules\Microsoft.PowerShell.Security\AclCmdlets.Tests.ps1' 0ms ParameterBindingException: A parameter cannot be found that matches parameter name 'Skip'. at <ScriptBlock>, F:\Source\PowerShell\test\powershell\Modules\Microsoft.PowerShell.Security\AclCmdlets.Tests.ps1: line 3 at <ScriptBlock>, F:\Source\PowerShell\src\powershell-win-core\bin\Debug\net5.0\win7-x64\publish\Modules\Pester\4.10.1\Pester.psm1: line 1111 at Invoke-Pester<End>, F:\Source\PowerShell\src\powershell-win-core\bin\Debug\net5.0\win7-x64\publish\Modules\Pester\4.10.1\Pester.psm1: line 1137 Tests completed in 174ms Tests Passed: 0, Failed: 1, Skipped: 0, Pending: 0, Inconclusive: 0

I installed Pester 5.0.1 version and re-ran the test but the script download a different version of pester and performed the test.

Yeah there's a lot we'd have to update here before we can move to v5 for this repo.

Add pester test for Get-Acl cmdlet

rjmholt · 2020-07-09T17:48:18Z

@Shriram0908 what remains to bring this PR out of WIP status?

Shriram0908 · 2020-07-09T17:57:38Z

@rjmholt, We can bring the PR out of the draft state. I want your opinion on few things. The code that I have changed fixes the issue but I feel that block of code can be refactored a bit. Is it okay to do it in this PR or should I raise a seperate request.

rjmholt · 2020-07-09T18:00:28Z

Is it okay to do it in this PR or should I raise a seperate request.

Best to do separately I think. Keeps the functional changes separate from the refactoring.

The Get-Acl tests seem to be triggering and failing on *nix though -- do they need to be skipped on those platforms?

Shriram0908 · 2020-07-09T18:21:03Z

Best to do separately I think. Keeps the functional changes separate from the refactoring.

Got it. Thanks

The Get-Acl tests seem to be triggering and failing on *nix though -- do they need to be skipped on those platforms?

Yes Get-Acl is only for windows. I will add the -skip. The feature to skip entire describe is not yet added so I tried it to do it differently.

rjmholt · 2020-07-09T18:25:30Z

I tried it to do it differently.

I think elsewhere a solution I've seen is something like:

BeforeAll {
    $PSDefaultParameterValues["It:Skip"] = -not $IsWindows
}

AfterAll {
    $PSDefaultParameterValues.Remove("It:Skip")
}

ghost · 2020-07-18T02:00:16Z

This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days.
Maintainer, please provide feedback and/or mark it as Waiting on Author

ANDROID-ZERO-BYTE

Opt.ACL INTEGRATE winsec inv param exec;shell null loop: biassys renew shell param

rjmholt · 2020-07-20T19:13:38Z

@JamesWTruher this PR could probably use your review

daxian-dbw · 2020-07-30T20:47:24Z

@rjmholt @JamesWTruher A gentle ping :) please review the PR when you have time.

daxian-dbw · 2020-07-30T23:29:08Z

@PoshChan Please remind me in a day

PoshChan · 2020-07-30T23:29:52Z

@daxian-dbw, I do not understand: Please remind me in a day

Commands available in this repo for you:

retry <target> this will attempt to retry only the failed jobs for the target pipeline, restart can be used in place of retry
rebuild <target> this will perform a complete rebuild of the target pipeline, rerun can be used in place of rebuild Supported values for <target> which can be a comma separated list are: static,windows,macos,linux,ssh,all
get failures this will attempt to get the latest failures for all of the target pipelines
remind me in <value> <units> this will create a reminder that will be posted after the specified duration <value> is a number, and <units> can be minutes, hours, or days (singular or plural)

daxian-dbw · 2020-07-30T23:30:31Z

@PoshChan Please remind me in 1 day

PoshChan · 2020-07-31T23:30:39Z

@daxian-dbw, this is the reminder you requested 1 day ago

daxian-dbw · 2020-07-31T23:49:12Z

To the maintainer that will work on the change log for the next 7.1 preview:
I updated the commit message when merging but the merging failed. Then I clicked "Try again", and it succeeded, but with the original PR title as the commit message. I have changed the PR title to a better message, which can be used in the change log.

…ell#13107) * Fix PowerShell#11566 bug Add pester test for Get-Acl cmdlet * Replace -pending parameter with -skip * Fix test failing in Linux and MacOS

ghost · 2020-08-17T22:20:49Z

🎉v7.1.0-preview.6 has been released which incorporates this pull request.:tada:

Handy links:

Release Notes

ghost assigned daxian-dbw Jul 5, 2020

daxian-dbw requested review from adityapatwardhan and rjmholt July 7, 2020 00:02

rjmholt marked this pull request as draft July 7, 2020 17:20

rjmholt reviewed Jul 7, 2020

View reviewed changes

Shriram0908 added 2 commits July 9, 2020 23:00

Fix #11566 bug

4976341

Add pester test for Get-Acl cmdlet

Replace -pending parameter with -skip

3a6df14

Shriram0908 force-pushed the Get-Acl-#11566 branch from 284eef6 to 3a6df14 Compare July 9, 2020 17:31

Shriram0908 marked this pull request as ready for review July 9, 2020 17:57

Shriram0908 changed the title ~~WIP: Fix Get-Acl -LiteralPath "HKLM:Software\Classes\*" behaviour~~ Fix Get-Acl -LiteralPath "HKLM:Software\Classes\*" behaviour Jul 9, 2020

Fix test failing in Linux and MacOS

3aea46f

rjmholt requested review from JamesWTruher, SteveL-MSFT and TravisEz13 July 10, 2020 15:56

ghost added the Review - Needed The PR is being reviewed label Jul 18, 2020

ANDROID-ZERO-BYTE approved these changes Jul 18, 2020

View reviewed changes

ghost removed the Review - Needed The PR is being reviewed label Jul 30, 2020

rjmholt approved these changes Jul 30, 2020

View reviewed changes

daxian-dbw merged commit 9ceee3e into PowerShell:master Jul 31, 2020

daxian-dbw added the CL-General Indicates that a PR should be marked as a general cmdlet change in the Change Log label Jul 31, 2020

daxian-dbw added this to the 7.1.0-preview.7 milestone Jul 31, 2020

daxian-dbw changed the title ~~Fix Get-Acl -LiteralPath "HKLM:Software\Classes\*" behaviour~~ Avoid unnecessary path resolution for Get-Acl -LiteralPath Jul 31, 2020

Shriram0908 deleted the Get-Acl-#11566 branch August 2, 2020 13:49

TravisEz13 modified the milestones: 7.1.0-preview.7, 7.1.0-preview.6 Aug 5, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Avoid unnecessary path resolution for `Get-Acl -LiteralPath` #13107

Avoid unnecessary path resolution for `Get-Acl -LiteralPath` #13107

Shriram0908 commented Jul 5, 2020 •

edited

rjmholt commented Jul 7, 2020

rjmholt Jul 7, 2020

vexx32 Jul 9, 2020

Shriram0908 Jul 10, 2020

vexx32 Jul 10, 2020

rjmholt commented Jul 9, 2020

Shriram0908 commented Jul 9, 2020

rjmholt commented Jul 9, 2020

Shriram0908 commented Jul 9, 2020

rjmholt commented Jul 9, 2020 •

edited

ghost commented Jul 18, 2020

ANDROID-ZERO-BYTE left a comment

rjmholt commented Jul 20, 2020

daxian-dbw commented Jul 30, 2020

daxian-dbw commented Jul 30, 2020

PoshChan commented Jul 30, 2020

daxian-dbw commented Jul 30, 2020

PoshChan commented Jul 31, 2020

daxian-dbw commented Jul 31, 2020

ghost commented Aug 17, 2020

Avoid unnecessary path resolution for Get-Acl -LiteralPath #13107

Avoid unnecessary path resolution for Get-Acl -LiteralPath #13107

Conversation

Shriram0908 commented Jul 5, 2020 • edited

PR Summary

PR Context

PR Checklist

rjmholt commented Jul 7, 2020

rjmholt Jul 7, 2020

Choose a reason for hiding this comment

vexx32 Jul 9, 2020

Choose a reason for hiding this comment

Shriram0908 Jul 10, 2020

Choose a reason for hiding this comment

vexx32 Jul 10, 2020

Choose a reason for hiding this comment

rjmholt commented Jul 9, 2020

Shriram0908 commented Jul 9, 2020

rjmholt commented Jul 9, 2020

Shriram0908 commented Jul 9, 2020

rjmholt commented Jul 9, 2020 • edited

ghost commented Jul 18, 2020

ANDROID-ZERO-BYTE left a comment

Choose a reason for hiding this comment

rjmholt commented Jul 20, 2020

daxian-dbw commented Jul 30, 2020

daxian-dbw commented Jul 30, 2020

PoshChan commented Jul 30, 2020

daxian-dbw commented Jul 30, 2020

PoshChan commented Jul 31, 2020

daxian-dbw commented Jul 31, 2020

ghost commented Aug 17, 2020

Avoid unnecessary path resolution for `Get-Acl -LiteralPath` #13107

Avoid unnecessary path resolution for `Get-Acl -LiteralPath` #13107

Shriram0908 commented Jul 5, 2020 •

edited

rjmholt commented Jul 9, 2020 •

edited