-
Notifications
You must be signed in to change notification settings - Fork 7.6k
Enhance Remove-Item to work with OneDrive (Second) #15260
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
Enhance Remove-Item to work with OneDrive (Second) #15260
Conversation
a1d15d6
to
1bc1de8
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
/azp run PowerShell-CI-Windows-daily |
Azure Pipelines successfully started running 1 pipeline(s). |
@rjmholt See how to run the CI that tests the issue here: #15260 (comment) |
This pull request has been automatically marked as Review Needed because it has been there has not been any activity for 7 days. |
/azp run PowerShell-CI-Windows-daily |
Azure Pipelines successfully started running 1 pipeline(s). |
src/System.Management.Automation/namespaces/FileSystemProvider.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/namespaces/FileSystemProvider.cs
Outdated
Show resolved
Hide resolved
src/System.Management.Automation/namespaces/FileSystemProvider.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Robert Holt <rjmholt@gmail.com>
Co-authored-by: Robert Holt <rjmholt@gmail.com>
Co-authored-by: Robert Holt <rjmholt@gmail.com>
Ok this seems to be passing CI so I'm going to merge, but we'll just need to keep an eye out for CI failures later on |
Verified this passed CI after merge: |
We've picked up a test failure during release testing that we think is caused by this change. The failing tests are these: PowerShell/test/powershell/Modules/Microsoft.PowerShell.Management/FileSystem.Tests.ps1 Lines 960 to 999 in dd5cf86
And the error message is this:
Exception:
The error is thrown here: PowerShell/src/System.Management.Automation/namespaces/FileSystemProvider.cs Lines 8248 to 8254 in dd5cf86
It appears that using
It also seems that the long path tests to catch this did not in our daily builds and we need to investigate why.
|
I wonder CIs was passed successfully. |
🎉 Handy links: |
|
||
Test-Path $testdirectory | Should -BeFalse | ||
} | ||
It "Should be able to recursively delete a directory with a trailing backslash" { |
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.
Thanks for adding this regression test!
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.
Really we have very small code coverage. I hope WGs start work and we add more tests to exclude regressions.
I discussed this with @daxian-dbw, @adityapatwardhan and @anmenaga in a maintainer review and rather than trying to fix this as is, we think the previous bug is easier to live with than the new bug and that we need to revert it. We can accept a full solution later. |
🎉 Handy links: |
PR Summary
Better to review commit by commit:
About second commit.
Partial regression in #14902. Partial because there are other code paths (a path comes from public with trailing backslash) which can raise an issue in the place.
Docs say that FindFirstFileEx does not accept a path with a trailing backslash (\).
The fix is always removing a trailing backslash from path string before calling FindFirstFileEx.
Before the fix only enumeration (Get-ChildItem) worked on OneDrive.
Now Remove-Item correctly removes folders and files on OneDrive too.
PR Context
Replace #14860 and #14902 (reverted in #15253)
Related #9246
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
or[ WIP ]
to the beginning of the title (theWIP
bot will keep its status check atPending
while the prefix is present) and remove the prefix when the PR is ready.(which runs in a different PS Host).