PowerShell / PSDscResources

MIT License
129 stars 53 forks source link

Bugfix for Issue 87 including two new integration tests #118

Closed tmeckel closed 4 years ago

tmeckel commented 5 years ago

This pull request contains the fix for Issue #87 which might throw an exception when the local administrator account is removed from the local administrators group. In addtion to the fix two integration tests has been added to verify that the DSC Group Resource now handles the local administrators group and the local administrator correctly.

According to the verbose output of those newly added integration tests the fix works correctly.

VERBOSE: [WIN-19O94QK21A9]: LCM:  [ Start  Set      ]
VERBOSE: [WIN-19O94QK21A9]: LCM:  [ Start  Resource ]  [[Group]Group2]
VERBOSE: [WIN-19O94QK21A9]: LCM:  [ Start  Test     ]  [[Group]Group2]
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group2] Invoking the function
Test-TargetResourceOnFullSKU for the group Administrators.
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group2] A group with the name Administrators exists.
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group2] Resolving Administrator as a local account.
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group2] MembersToInclude is empty. No group member
additions are needed.
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group2] Resolving Administrator as a local account.
VERBOSE: [WIN-19O94QK21A9]: LCM:  [ End    Test     ]  [[Group]Group2]  in 8.6570 seconds.
VERBOSE: [WIN-19O94QK21A9]: LCM:  [ Start  Set      ]  [[Group]Group2]
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group2] Begin executing Set functionality on the group
Administrators.
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group2] Performing the operation "Set" on target "Group:
 Administrators".
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group2] Resolving Administrator as a local account.
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group2] MembersToInclude is empty. No group member
additions are needed.
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group2] Resolving Administrator as a local account.
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group2] Skipping local admin account Administrator for
administrators group Administrators.
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group2] Group Administrators properties updated
successfully.
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group2] End executing Set functionality on the group
Administrators.
VERBOSE: [WIN-19O94QK21A9]: LCM:  [ End    Set      ]  [[Group]Group2]  in 15.2510 seconds.
VERBOSE: [WIN-19O94QK21A9]: LCM:  [ End    Resource ]  [[Group]Group2]
VERBOSE: [WIN-19O94QK21A9]: LCM:  [ End    Set      ]
VERBOSE: [WIN-19O94QK21A9]: LCM:  [ End    Set      ]    in  31.1430 seconds.
VERBOSE: Operation 'Invoke CimMethod' complete.
VERBOSE: Time taken for configuration job to complete is 31.219 seconds
 [+] Should not exclude the local Administrator account from the Administrators group via MembersToExclude 104.27s
VERBOSE: Perform operation 'Invoke CimMethod' with following parameters, ''methodName' =
SendConfigurationApply,'className' = MSFT_DSCLocalConfigurationManager,'namespaceName' =
root/Microsoft/Windows/DesiredStateConfiguration'.
VERBOSE: An LCM method call arrived from computer WIN-19O94QK21A9 with user sid
S-1-5-21-768687028-2804546567-894006156-500.
VERBOSE: [WIN-19O94QK21A9]: LCM:  [ Start  Set      ]
VERBOSE: [WIN-19O94QK21A9]: LCM:  [ Start  Resource ]  [[Group]Group1]
VERBOSE: [WIN-19O94QK21A9]: LCM:  [ Start  Test     ]  [[Group]Group1]
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group1] Invoking the function
Test-TargetResourceOnFullSKU for the group Administrators.
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group1] A group with the name Administrators exists.
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group1] Resolving Administrator as a local account.
VERBOSE: [WIN-19O94QK21A9]: LCM:  [ End    Test     ]  [[Group]Group1]  in 15.0780 seconds.
VERBOSE: [WIN-19O94QK21A9]: LCM:  [ Start  Set      ]  [[Group]Group1]
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group1] Begin executing Set functionality on the group
Administrators.
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group1] Performing the operation "Set" on target "Group:
 Administrators".
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group1] Resolving Administrator as a local account.
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group1] Skipping local admin account Administrator for
administrators group Administrators.
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group1] Group Administrators properties updated
successfully.
VERBOSE: [WIN-19O94QK21A9]:                            [[Group]Group1] End executing Set functionality on the group
Administrators.
VERBOSE: [WIN-19O94QK21A9]: LCM:  [ End    Set      ]  [[Group]Group1]  in 15.0160 seconds.
VERBOSE: [WIN-19O94QK21A9]: LCM:  [ End    Resource ]  [[Group]Group1]
VERBOSE: [WIN-19O94QK21A9]: LCM:  [ End    Set      ]
VERBOSE: [WIN-19O94QK21A9]: LCM:  [ End    Set      ]    in  30.1720 seconds.
VERBOSE: Operation 'Invoke CimMethod' complete.
VERBOSE: Time taken for configuration job to complete is 30.25 seconds
 [+] Should not remove the local Administrator account from the Administrators group using Members assignment 46.2s

This change is Reviewable

codecov-io commented 5 years ago

Codecov Report

Merging #118 into dev will decrease coverage by <1%. The diff coverage is 6%.

Impacted file tree graph

@@         Coverage Diff         @@
##            dev   #118   +/-   ##
===================================
- Coverage    83%    83%   -1%     
===================================
  Files        19     19           
  Lines      2760   2770   +10     
  Branches      4      4           
===================================
  Hits       2305   2305           
- Misses      451    461   +10     
  Partials      4      4
stale[bot] commented 5 years ago

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

PlagueHO commented 5 years ago

Hi @tmeckel - sorry this has taken so long. I'm trying to clean up some of these. Can you check the AppVeyor CI - it looks like it has failed. One of the failures is just a missing blank line at the end of a file: https://ci.appveyor.com/project/PowerShell/psdscresources/builds/19321527?fullLog=true#L294

The other is in the integration tests: https://ci.appveyor.com/project/PowerShell/psdscresources/builds/19321527?fullLog=true#L2656

tmeckel commented 5 years ago

Hi @PlagueHO - nice to see progress here again! Cool! I just checked the issues you mentioned.

tmeckel commented 5 years ago

@PlagueHO Okay, after the last appveyor run three errors with the Script DSC Resource still persist. From my point of they should be fixed with another PR not with this one.

image

PlagueHO commented 5 years ago

In the latest PR that I've submitted to this repo these tests aren't failing: https://github.com/PowerShell/PSDscResources/pull/130. I suspect that the change I made to add .gitattributes to the repo to ensure binaries and text files are being treated correctly may have resolved this issue (all repos should probably include the .gitattributes - something to add to the template I think).

tmeckel commented 5 years ago

Okay... I'll add the gitattributes to my PR let's see if this helps

PlagueHO commented 5 years ago

Fingers crossed (it was really only feeling that it would fix it on my part)>

tmeckel commented 5 years ago

@PlagueHO it didn't work! Too bad.. any ideas beside fixing the failing tests inside this PR?

PlagueHO commented 5 years ago

Hi @tmeckel - once I've got #130 complete you can rebase against that and we'll see if we can figure out what is going on.

tmeckel commented 5 years ago

Hi @PlagueHO , when I look at #130 and what changes you applied to what files the only file that could make any difference in my case are the changes to the AppVeyor configuration. All other changes are IMHO not related to my problem to get the test passed. I'll give this a try :-D

tmeckel commented 5 years ago

@PlagueHO it didn't work either! The only thing I discovered is the fact that the file in which the failing tests are located is having a UTF-8 BOM encoding whereas the other files don't have a BOM. Perhaps it's simply an encoding issue.

tmeckel commented 5 years ago

@PlagueHO how do we proceed here? Shall I try to rebase the PR?

stale[bot] commented 5 years ago

Labeling this pull request (PR) as abandoned since it has gone 14 days or more since the last update. An abandoned PR can be continued by another contributor. The abandoned label will be removed if work on this PR is taken up again.

tmeckel commented 5 years ago

Too bad that the PR had been labeled 'abandoned' by the bot. So again @PlagueHO how do we proceed here. Oh and the issue in xPSDesiredStateConfiguration waits for the backport of this PR :-D https://github.com/PowerShell/xPSDesiredStateConfiguration/issues/467