Closed danielboth closed 6 years ago
johlju
commented
6 years ago @danielboth Can you help resolve the encoding of the schema.mof file, should be UTF-8 (without BOM)? https://ci.appveyor.com/project/PowerShell/xremotedesktopsessionhost/build/1.3.91.0#L28
codecov-io
commented
6 years ago Merging #35 into dev will increase coverage by
49.62%. The diff coverage is97.02%.
@@ Coverage Diff @@
## dev #35 +/- ##
===========================================
+ Coverage 15.96% 65.59% +49.62%
===========================================
Files 5 5
Lines 119 186 +67
Branches 4 4
===========================================
+ Hits 19 122 +103
+ Misses 96 60 -36
Partials 4 4| Impacted Files | Coverage Δ | |
|---|---|---|
| xRemoteDesktopSessionHostCommon.psm1 | 100% <100%> (ø) |
:arrow_up: |
| ...ration/MSFT_xRDSessionCollectionConfiguration.psm1 | 96.36% <96.96%> (+87.27%) |
:arrow_up: |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 1d7210c...59e5198. Read the comment docs.
danielboth
commented
6 years ago @johlju I've updated the schema.mof and added tests for my changes to Set-TargetResource (and a bit more). At least a green build now :)
johlju
commented
6 years ago @ld0614 can you take this review?
@danielboth Awesome work on the tests!
ld0614
commented
6 years ago It looks good, thanks for the PR. There are a number of PSSA rules which need rectifying https://ci.appveyor.com/project/PowerShell/xremotedesktopsessionhost#L218 and the readme and change log will need updating as well.
Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 99 at r2 (raw file):
# This part of the configuration only applies to Win 2016+ if(([version](Get-CimInstance -ClassName win32_operatingsystem -Property Version).Version).Major -ge 10) {
Should probably use the helper function Get-OsVersion to maintain consistancy
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 198 at r2 (raw file):
if(([version](Get-CimInstance -ClassName win32_operatingsystem -Property Version).Version).Major -ge 10) {
As above
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 221 at r2 (raw file):
if(-not($MaxUserProfileDiskSizeGB -gt 0)) { Throw "To enable UserProfileDisk we need a setting for MaxUserProfileDiskSizeGB that is greater than 0. Current value $MaxUserProfileDiskSizeGB is not valid"
I would rearrange this if statement so that it matches the one above with the else throwing, -le 0 is also clearer than -not (-gt 0)
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 238 at r2 (raw file):
2>&1This should be rewritten as -ErrorAction Continue if that is the desired behaviour or possibly -ErrorAction SilentlyContinue with -ErrorVariable
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 336 at r2 (raw file):
$isInDesiredState = $true if(([version](Get-CimInstance -ClassName win32_operatingsystem -Property Version).Version).Major -lt 10) {
As Above
Comments from Reviewable
danielboth
commented
6 years ago Thanks for the feedback @ld0614, I'll get the code fixed, probably won't happen tonight but will get this done in the next two days.
danielboth
commented
6 years ago Review status: 0 of 3 files reviewed at latest revision, 5 unresolved discussions.
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 238 at r2 (raw file):
> ``` > 2>&1 > ``` This should be rewritten as -ErrorAction Continue if that is the desired behaviour or possibly -ErrorAction SilentlyContinue with -ErrorVariable
I started with this and it seems a bit more challenging than I was thinking before. This due to a bug that I hit when setting up the UserProfileDisk. As you can see in the error message, the error is about two command that cannot be found. I traced this down to the Test-UserVhdPathInUse function that is being called in the module C:\Windows\system32\WindowsPowerShell\v1.0\Modules\RemoteDesktop\Utility.psm1. For some reason that function is not aware of the prefix of the RemoteDesktop commands when running the DSC context. I manually edited the file (after changing the permissions to write for administrators) to include the commands without the -RD prefix, after which the DSC configuration can be applied without any errors.
A second bug I'm hitting is that if you are only changing the size of the UserProfileDisk, for example from 5GB to 6GB, that it will run the command fine, but it will not apply the setting. This is also true outside the DSC context. So when a UserProfileDisk is already configured, the size for the disk is not changed when this command is ran:
Set-RDSessionCollectionConfiguration -CollectionName TestCollection -MaxUserProfileDiskSizeGB 6 -EnableUserProfileDisk -DiskPath c:\tempThe last part I could fix in the resource by disabling the UserProfileDisk configuration first and then enabling it again. But it will keep erroring out on the two functions it cannot find. Any recommendations on how to handle that best?
Comments from Reviewable
danielboth
commented
6 years ago Review status: 0 of 5 files reviewed at latest revision, 5 unresolved discussions, some commit checks failed.
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 99 at r2 (raw file):
Should probably use the helper function Get-OsVersion to maintain consistancy
Done, changed Get-OsVersion to Get-xRemoteDesktopSessionHostOsVersion and used that one instead.
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 198 at r2 (raw file):
As above
Done, as above
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 221 at r2 (raw file):
I would rearrange this if statement so that it matches the one above with the else throwing, -le 0 is also clearer than -not (-gt 0)
Done, switched to -le
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 336 at r2 (raw file):
As Above
Done, as above
Comments from Reviewable
danielboth
commented
6 years ago Review status: all files reviewed at latest revision, 5 unresolved discussions.
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 99 at r2 (raw file):
Done, changed Get-OsVersion to Get-xRemoteDesktopSessionHostOsVersion and used that one instead.
Done.
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 198 at r2 (raw file):
Done, as above
Done.
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 221 at r2 (raw file):
Done, switched to -le
Done.
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 336 at r2 (raw file):
Done, as above
Done.
Comments from Reviewable
ld0614
commented
6 years ago Review status: all files reviewed at latest revision, 2 unresolved discussions.
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 221 at r2 (raw file):
Done.
Apologies if my above comment was unclear, now that you've switched the throw to the else statement the logic of -not(-le) == -gt. -gt is easier to read as its a single comparison rather than needing 2 comparisons.
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 238 at r2 (raw file):
I started with this and it seems a bit more challenging than I was thinking before. This due to a bug that I hit when setting up the UserProfileDisk. As you can see in the error message, the error is about two command that cannot be found. I traced this down to the Test-UserVhdPathInUse function that is being called in the module C:\Windows\system32\WindowsPowerShell\v1.0\Modules\RemoteDesktop\Utility.psm1. For some reason that function is not aware of the prefix of the RemoteDesktop commands when running the DSC context. I manually edited the file (after changing the permissions to write for administrators) to include the commands without the -RD prefix, after which the DSC configuration can be applied without any errors. A second bug I'm hitting is that if you are only changing the size of the UserProfileDisk, for example from 5GB to 6GB, that it will run the command file, but it will not apply the setting. This is also true outside the DSC context. So when a UserProfileDisk is already configured, the size for the disk is not changed when this command is ran: ```PowerShell Set-RDSessionCollectionConfiguration -CollectionName TestCollection -MaxUserProfileDiskSizeGB 6 -EnableUserProfileDisk -DiskPath c:\temp ``` The last part I could fix in the resource by disabling the UserProfileDisk configuration first and then enabling it again. But it will keep erroring out on the two functions it cannot find. Any recommendations on how to handle that best?
I hit similar issues with loading in the module when working on the HA build currently available https://github.com/ld0614/xRemoteDesktopSessionHost which I'll be pushing once I have written tests for it. It appears to be a bug in the module implementation I resolved it by taking the approach that the Azure RDS quickstart modifications did (https://github.com/Azure/azure-quickstart-templates/blob/master/rds-deployment/Configuration.zip). If you look here you can see what I was talking about by using ErrorAction to allow program continuation and then view the output of the error using the ErrorVariable option.
Comments from Reviewable
danielboth
commented
6 years ago Review status: all files reviewed at latest revision, 2 unresolved discussions.
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 221 at r2 (raw file):
Apologies if my above comment was unclear, now that you've switched the throw to the else statement the logic of -not(-le) == -gt. -gt is easier to read as its a single comparison rather than needing 2 comparisons.
Done. Sorry, obviously one is easier to read. Fixed now :).
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 238 at r2 (raw file):
I hit similar issues with loading in the module when working on the HA build currently available https://github.com/ld0614/xRemoteDesktopSessionHost which I'll be pushing once I have written tests for it. It appears to be a bug in the module implementation I resolved it by taking the approach that the Azure RDS quickstart modifications did (https://github.com/Azure/azure-quickstart-templates/blob/master/rds-deployment/Configuration.zip). If you look [here](https://github.com/ld0614/xRemoteDesktopSessionHost/blob/QuickStartMerge/DSCResources/MSFT_xRDServer/MSFT_xRDServer.psm1#L130) you can see what I was talking about by using ErrorAction to allow program continuation and then view the output of the error using the ErrorVariable option.
Ok, I like the way the CommandNotFoundException is checked, so I modified the code to align with what the workaround you showed is doing. However, it does not work with just -ErrorAction SilentlyContinue and -ErrorVariable. Even with both of those, the CommandNotFoundException still shows up in the error stream so I see no other solution than to redirect the error stream. I captured what I did in a small video (see here: https://1drv.ms/v/s!AorMCX2HQHeyg_h-0aB_Aso4e1SMDQ). Here I start with redirecting the error stream, after which everything is handeled nicely, also visible in the verbose stream. If I take the redirection of the error stream of, by commenting it out, the errors show up in the next run right away.
Do you see any reason not to workaround the problem by redirecting the error stream? I would like to learn about the reasoning behind this.
Comments from Reviewable
ld0614
commented
6 years ago Review status: all files reviewed at latest revision, 1 unresolved discussion.
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 238 at r2 (raw file):
Ok, I like the way the CommandNotFoundException is checked, so I modified the code to align with what the workaround you showed is doing. However, it does not work with just -ErrorAction SilentlyContinue and -ErrorVariable. Even with both of those, the CommandNotFoundException still shows up in the error stream so I see no other solution than to redirect the error stream. I captured what I did in a small video (see here: https://1drv.ms/v/s!AorMCX2HQHeyg_h-0aB_Aso4e1SMDQ). Here I start with redirecting the error stream, after which everything is handeled nicely, also visible in the verbose stream. If I take the redirection of the error stream of, by commenting it out, the errors show up in the next run right away. Do you see any reason not to workaround the problem by redirecting the error stream? I would like to learn about the reasoning behind this.
hmm, thanks for the video, I'll see if I can recreate the issue on my side, the reason for me disliking using a stream redirect is that its not especially clear whats going on (kind of like using % or instead of foreach).
What about something like this? I've not tested it yet but in theory I belive that it should handle only command not found exceptions while throwing all others from the orgion. I also think its easier to read while actually following PowerShell pylosofy a bit better, @johlju what are your thoughts?
try
{
$null = Set-RDSessionCollectionConfiguration @enableUserProfileDiskSplat -ErrorAction Stop
}
catch [System.Management.Automation.CommandNotFoundException]
{
Write-Verbose "Set-RDSessionCollectionConfiguration: trapped erroneous CommandNotFoundException errors, that's ok, continuing..."
}Comments from Reviewable
danielboth
commented
6 years ago From a PowerShell perspective you are completely right @ld0614. In this case however, that doesn't work, since Set-RDSessionCollectionConfiguration with -ErrorAction Stop will actually stop executing the required code when you try - catch like this. It does not continue anymore. This means all the code after the first CommandNotFoundException is not executed, which would break the actual changing of the session collection configuration (I tested this to confirm and indeed, it does not change the config anymore).
I think this is the reason the Azure configuration also uses the workaround this way. Not beautiful for sure, but we would need a bugfix from Microsoft on the RemoteDesktop module before we can do this properly.
Edit, btw, I think in the comment after the redirection of the error stream I'm quite verbose on what is going on in the code and why it is being done, so I hope that makes it clear for future contributors as well.
ld0614
commented
6 years ago Review status: all files reviewed at latest revision, 1 unresolved discussion.
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 238 at r2 (raw file):
hmm, thanks for the video, I'll see if I can recreate the issue on my side, the reason for me disliking using a stream redirect is that its not especially clear whats going on (kind of like using % or instead of foreach). What about something like this? I've not tested it yet but in theory I belive that it should handle only command not found exceptions while throwing all others from the orgion. I also think its easier to read while actually following PowerShell pylosofy a bit better, @johlju what are your thoughts? ``` try { $null = Set-RDSessionCollectionConfiguration @enableUserProfileDiskSplat -ErrorAction Stop } catch [System.Management.Automation.CommandNotFoundException] { Write-Verbose "Set-RDSessionCollectionConfiguration: trapped erroneous CommandNotFoundException errors, that's ok, continuing..." } ```
😟 I guess we'll have to run with it then, would I be able to ask that you just comment either on the line or above it something like '2>&1 redirects the error stream to output strream' please?
Comments from Reviewable
danielboth
commented
6 years ago Review status: all files reviewed at latest revision, 1 unresolved discussion.
DSCResources/MSFT_xRDSessionCollectionConfiguration/MSFT_xRDSessionCollectionConfiguration.psm1, line 238 at r2 (raw file):
😟 I guess we'll have to run with it then, would I be able to ask that you just comment either on the line or above it something like '2>&1 redirects the error stream to output strream' please?
Comment added on the line above the redirection.
Comments from Reviewable
ld0614
commented
6 years ago Thanks, if no one has any objections I'll merge tomorrow afternoon
Review status: all files reviewed at latest revision, all discussions resolved.
Comments from Reviewable
This version adds the ability to configure User Profile Disks on Windows Server 2016 using the xRemoteDesktopSessionCollectionConfiguration resource. This is built in a way that the new properties are ignored when running on Windows Server 2012 R2.
First of all, I would like to get some feedback on the implementation. If we agree this is the right direction I will finish by updating the readme etc.
This change is