Group: Creates error on forcing members for builtin Administrators group about psdscresources HOT 16 CLOSED

@tmeckel Please do! Happy to review a PR that resolves this.

from psdscresources.

The local group 'Administrators' has the SID S-1-5-32-544. Could we get the group by SID using (Get-LocalGroup -SID 'S-1-5-32-544').Name and use that in the tests?

from psdscresources.

Not dumb at all! Working together gave a solution faster. I wouldn't even have thought of making the test localized like you did! Great work 👍 🙂

from psdscresources.

@johlju I finalized the changes inside the code. I discovered some more places where it's possible to remove the local admin account from the local Admnistrators group, which might lead to an exception if the changes are committed. For each change I'll add a Pester test. When I'm done I'll create a merge request. Or would you like to do quick review of the changes inside my repo at first?

from psdscresources.

@tmeckel Please send in a PR once you feel you are finished. It is easier for me to review a PR. 🙂

from psdscresources.

@tmeckel If the change is needed in xPSDesiredStateConfiguration then pleaseopen a PR there as well. The tow repositories have there own lifecycle and are not ported either way.

from psdscresources.

@tmeckel It is appreciated if you create an issue, then the community to discuss the issue if needed. The new issue could reference this issue for more details. Then refer the new PR to the newly created issue.

from psdscresources.

I appreciate the detailed description - feel free to open a Pull Request with these changes.

from psdscresources.

I verfied that the code, that I've changed, never has been imported into GitHub. The code I patched is from the PSDesiredStateConfiguration Module Version 1.1 which is shipped with WMF 5.1.
So as far that I can see, no need to change anything here, because the offending CLEAR statement isn't used here in the GitHub code.

The only thing that might be improved is the test, that the local administrator account isn't removed from the local Adminitrators group. Because that would create a exception if the changes would be committed.

Let me know if I should provide a pull request for that, or if you let this change be implemented by other means.

from psdscresources.

The potentially offending $Group.Members.Clear() is called here

So what @tmeckel suggest might need to be done, adding a regression test that clears Administrators group and make sure it can be handled.

@tmeckel If you are still up for this, please send in a PR.

from psdscresources.

Sorry @johlju! I didn't watch that issue closely. Shall I still provide a PR for that issue?

from psdscresources.

@johlju Quick question: the (local) Administrators groups uses I18N names, depending on the installed (primary) language of the Windows Computer. So in English (LCID: 1033) the group is named "Administrators". On a system installed with German (LCDI: 1031) as primary language the group is named "Administratoren".

So how to write the Pester regression test for the issue here?

As far as I can see the Pester Unit tests for MSFT_GroupResource uses the $script:testGroupName parameter. This can't be used here either. So what's, from you point of view, the best thing to do to implement the test?

from psdscresources.

Okay ... I was dumb! :-D Great idea .. I could've come up with that myself.

from psdscresources.

@johlju I just added a pull request containing the fixes for this issue. Sorry for the delay.
Oh and will you backport the fix the xPSDesiredStateConfiguration or shall I provide another PR for this repo also?

from psdscresources.

@johlju it's definitly required. The offending Clear() statement ist present here as well

https://github.com/PowerShell/xPSDesiredStateConfiguration/blob/5a205a582b433f11e3109179aeb041f9999c207c/DSCResources/MSFT_xGroupResource/MSFT_xGroupResource.psm1#L2439

Do I have to open an issue at xPSDesiredStateConfiguration for this. Or can I simply provide a PR?

from psdscresources.

@tmeckel It is appreciated if you create an issue, then the community to discuss the issue if needed. The new issue could reference this issue for more details. Then refer the new PR to the newly created issue.

@johlju I created an issue dsccommunity/xPSDesiredStateConfiguration#467 at xPSDesiredStateConfiguration

from psdscresources.