-
Notifications
You must be signed in to change notification settings - Fork 149
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
xWebVirtualDirectory: Fails to create directory under application #533
base: main
Are you sure you want to change the base?
xWebVirtualDirectory: Fails to create directory under application #533
Conversation
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.
Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @omiossec)
a discussion (no related file):
Can we add integration tests that tests the scenario(s) the change resolves?
a discussion (no related file):
Please add unit tests for this change.
a discussion (no related file):
We could leave as-it, but instead of duplicating code in Set- and Test-function, we should refactor the code to call the Get-function. We could improve this in another PR. You choice.
CHANGELOG.md, line 6 at r1 (raw file):
Update on how Virtual directory are tested to correct the issue
Can we please add a bit more descriptive text what this change fixes and how it affects the (users) existing configurations?
DSCResources/MSFT_xWebVirtualDirectory/MSFT_xWebVirtualDirectory.psm1, line 52 at r1 (raw file):
Assert-Module if ($null -eq $WebApplication -OR $WebApplication.Length -eq 0)
Good as-is but we could evaluate this using IsNullOrEmpty()
https://github.com/PowerShell/xWebAdministration/blob/c33e56b254b1b50111f13175b3fef8075384ae6e/Tests/Unit/MSFT_xWebAppPoolDefaults.Tests.ps1#L53
DSCResources/MSFT_xWebVirtualDirectory/MSFT_xWebVirtualDirectory.psm1, line 56 at r1 (raw file):
else {
We should have open-brace on a separate row.
DSCResources/MSFT_xWebVirtualDirectory/MSFT_xWebVirtualDirectory.psm1, line 121 at r1 (raw file):
if ($Ensure -eq 'Present') { if ($null -eq $WebApplication -OR $WebApplication.Length -eq 0)
See previous comments in Get-function.
DSCResources/MSFT_xWebVirtualDirectory/MSFT_xWebVirtualDirectory.psm1, line 203 at r1 (raw file):
Assert-Module if ($null -eq $WebApplication -OR $WebApplication.Length -eq 0)
See previous comments in Get-function.
@regedit32 Do you have time to look over this too? You have a better knowledge around these cmdlets than me. 😄 |
@johlju solution looks good, but I will take another look after tests have been added. |
We have renamed the resource, removing 'x', so please rebase this PR. |
Make `Set-TargetResource` switch between `WebApplication` '' and '/' as required by `New-WebVirtualDirectory` and `Remove-WebVirtualDirectory` respectively. Fix dsccommunity#331 Fix dsccommunity#366 Similar to old (2019) pr dsccommunity#533
Make `Set-TargetResource` switch between `WebApplication` '' and '/' as required by `New-WebVirtualDirectory` and `Remove-WebVirtualDirectory` respectively. Fix dsccommunity#331 Fix dsccommunity#366 Similar to old (2019) pr dsccommunity#533
Make `Set-TargetResource` switch between `WebApplication` '' and '/' as required by `New-WebVirtualDirectory` and `Remove-WebVirtualDirectory` respectively. Fix dsccommunity#331 Fix dsccommunity#366 Similar to old (2019) pr dsccommunity#533
As a proposed solution to the #331 issue I propose to change the way the resource checks Virtual Directory.
This Pull Request (PR) fixes the following issues
Task list
Entry should say what was changed and how that affects users (if applicable), and
reference the issue being resolved (if applicable).
and comment-based help.
This change is