Added xSQLServerRole resource #67
Conversation
With the SQL Server Role DSCResource, we can manage the SQL Server Role security.
|
Hi @luigilink, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
|
Check with AppVeyor failed with other ressources not mine |
|
@luigilink, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
…g ... in file example Because of failed test in app veyor: There are PSScriptAnalyzer errors that need to be fixed: SQL-ClusterDB.ps1 (Line 155): File 'SQL-ClusterDB.ps1' uses ConvertTo-SecureString with plaintext. This will expose secure information. Encrypted standard strings should be used instead. SQL-Standalone.ps1 (Line 104): File 'SQL-Standalone.ps1' uses ConvertTo-SecureString with plaintext. This will expose secure information. Encrypted standard strings should be used instead. SQLServerNetwork.ps1 (Line 60): File 'SQLServerNetwork.ps1' uses ConvertTo-SecureString with plaintext. This will expose secure information. Encrypted standard strings should be used instead.
luigilink
changed the title
|
Awesome work! Love to see this resource merged. Nobody has started the review so I thought I start and give you some comments. :) Reviewed 1 of 2 files at r1, 5 of 5 files at r2. README.md, line 170 [r3] (raw file):
I can't see that this parameter LoginCredential is used in the code. Is this necessary for your intended behavior? README.md, line 171 [r3] (raw file):
Could you change this to ServerRole (see other comments for changing this in code)? DSCResources/MSFT_xSQLServerFirewall/MSFT_xSQLServerFirewall.schema.mof, line 15 [r3] (raw file):
Did you add back the blank line to the end of this file? Just want to make sure. DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 26 [r3] (raw file):
I can't see that this parameter LoginCredential is used in the code. Is this necessary for your intended behavior? DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 30 [r3] (raw file):
Could you change this to ServerRole (and change accordingly in the rest of the code as well)? DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 47 [r3] (raw file):
Could you change this to 'sqlRoles' (and the same for the rest of the code)? If possible you can use `sqlRole' instead DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 50 [r3] (raw file):
Could you change this to DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 111 [r3] (raw file):
I can't see that this parameter LoginCredential is used in the code. Is this necessary for your intended behavior? DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 115 [r3] (raw file):
Could you change this to ServerRole (and change accordingly in the rest of the code as well)? DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 137 [r3] (raw file):
Could you change this to 'sqlRoles' (and the same for the rest of the code)? If possible you can use `sqlRole' instead DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 138 [r3] (raw file):
Could you change this to DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 185 [r3] (raw file):
I can't see that this parameter LoginCredential is used in the code. Is this necessary for your intended behavior? DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 189 [r3] (raw file):
Could you change this to ServerRole (and change accordingly in the rest of the code as well)? DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 214 [r3] (raw file):
Could you change this to DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 241 [r3] (raw file):
Could you change this to 'sqlRoles' (and the same for the rest of the code)? If possible you can use `sqlRole' instead DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 255 [r3] (raw file):
Could you move these brackets to seperate rows, and change DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 256 [r3] (raw file):
Same here as the row above. Change DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.schema.mof, line 5 [r3] (raw file):
The description here doesn't seem to be correct? The "If LoginType..." portion. DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.schema.mof, line 6 [r3] (raw file):
I can't see that this parameter LoginCredential is used in the code. Is this necessary for your intended behavior? DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.schema.mof, line 7 [r3] (raw file):
Could you change this to ServerRole (and change accordingly in the rest of the code as well)? DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.schema.mof, line 7 [r3] (raw file):
Could you align the descriptions in the schema with those in the README.md? DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.schema.mof, line 9 [r3] (raw file):
Could you change SQLInstanceName to be key as well? Then a user could add the same login to role(s) several instances on the same server. Examples/SQL-ClusterDB.ps1, line 156 [r3] (raw file):
Why did you add this? Just wondering if there is an issue having it uncommented? Examples/SQL-Standalone.ps1, line 105 [r3] (raw file):
Why did you add this? Just wondering if there is an issue having it uncommented? Examples/SQLServerNetwork.ps1, line 61 [r3] (raw file):
Why did you add this? Just wondering if there is an issue having it uncommented? Comments from Reviewable |
|
Reviewed 1 of 5 files at r2. Examples/SQL-ClusterDB.ps1, line 156 [r3] (raw file):
|
|
Reviewed 1 of 1 files at r3. Examples/SQL-ClusterDB.ps1, line 156 [r3] (raw file):
|
Add Examples Add Readme.md in resource folder Modify README.md
|
Reviewed 1 of 2 files at r1, 5 of 5 files at r2. README.md, line 170 [r3] (raw file):
|
Resolving Merge Conflicts.
|
Review status: 0 of 9 files reviewed at latest revision, 25 unresolved discussions. README.md, line 170 [r3] (raw file):
|
|
Review status: 0 of 10 files reviewed at latest revision, 25 unresolved discussions. Examples/SQL-ClusterDB.ps1, line 156 [r3] (raw file):
|
|
Reviewed 10 of 10 files at r4. Comments from Reviewable |
|
Nice work with the changes! I have a little more comments. :) It seems that you did a "git merge" or "git pull" ( Reviewed 10 of 10 files at r4. DSCResource.Tests, line 1 [r4] (raw file):
Could you please add this to a .gitignore file in your cloned repo. This folder is created when running tests, but should not be commited to the PR. README.md, line 170 [r3] (raw file):
|
|
Reviewed 1 of 1 files at r5. DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 92 [r4] (raw file):
Could you please remove the extra blank line here? DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 166 [r4] (raw file):
Could you please remove the extra blank line here? Comments from Reviewable |
|
Review status: all files reviewed at latest revision, 4 unresolved discussions. DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 38 [r7] (raw file):
|
|
I don't think the PowerShell Team is enforcing that these existing resources need to have a unit test when doing code changes (they do on new resources). Reviewed 2 of 3 files at r8. DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 38 [r7] (raw file):
|
|
My bad, this is a new resource so I think they are gonna enforce that this one has a unit test before merging. Let me know if you need assistance in creating one. :) Review status: all files reviewed at latest revision, 4 unresolved discussions. Comments from Reviewable |
|
Yes, all PRs with fairly substantial changes need to have (at least) unit tests for those changes. |
mbreakey3
added
waiting for author response
|
Unit Test added for this resource, could you give me feedback. |
|
Unit test looks good. @johlju - could you glance over this once more and give your ok? |
|
Yes, I will look at it. I think I saw an issue with the wrapper. Comment on it shortly. |
|
@luigilink great work! All logic looks good to me! Just a few style changes. Below text is not part of the review, just something you might consider next time you make a test. Also, make a big note that I am far from good at this myself. So let me know if I'm wrong! 😄 Consider that with a little more tweaks I think it would have been possible to get more code coverage. Like mocking the EnumMemberNames() method in the (now) Confirm-SqlServerRole. That I think would have allowed much of the code in the Confirm-SqlServerRole out of the wrapper and instead kept in the main code, which would have increased test code coverage (mocking a function excludes it from 'code coverage'). A wrapper should be as lightweight it can be. Reviewed 1 of 4 files at r9. xSQLServerHelper.psm1, line 458 [r9] (raw file):
Ca you make foreach lowercase? xSQLServerHelper.psm1, line 469 [r9] (raw file):
Can you make else in lower case? xSQLServerHelper.psm1, line 499 [r9] (raw file):
Can you make foreach lowercase? xSQLServerHelper.psm1, line 510 [r9] (raw file):
can you make lese lowercase? xSQLServerHelper.psm1, line 538 [r9] (raw file):
Can you make foreach lowercase? xSQLServerHelper.psm1, line 567 [r9] (raw file):
can you make return lowercase? Comments from Reviewable |
|
Reviewed 3 of 4 files at r9, 1 of 1 files at r10. xSQLServerHelper.psm1, line 458 [r9] (raw file):
|
|
@johlju Thanks you for the advice, I'll look on your test unit. |
|
Reviewed 3 of 4 files at r9, 1 of 1 files at r10. Comments from Reviewable |
|
Reviewed 1 of 7 files at r6, 1 of 6 files at r7, 1 of 3 files at r8, 1 of 4 files at r9. DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 25 at r10 (raw file):
Can we change the case for the values in this set? It's kind of hard to read DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 38 at r10 (raw file):
I think you can get rid of this if check - just assign the $sql variable DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 149 at r10 (raw file):
What about testing the rest of the values? DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.schema.mof, line 6 at r10 (raw file):
Are the '\n's here for a reason? As in '...\nPresent {default} \nAbsent \n...' DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.schema.mof, line 7 at r10 (raw file):
Can we change these values to be CamelCase? i.e. : BulkAdmin, DBCreator, DiskAdmin, etc. Or are they all lowercase for a reason? Examples/Resources/xSQLServerRole/1-AddServerRole.ps1, line 12 at r10 (raw file):
change to $SysAdminAccount Examples/Resources/xSQLServerRole/2-RemoveServerRole.ps1, line 4 at r10 (raw file):
'...CONTOSO\SQLUser does not have...' Examples/Resources/xSQLServerRole/2-RemoveServerRole.ps1, line 12 at r10 (raw file):
$SysAdminAccount Tests/Unit/xSQLServerRole.Tests.ps1, line 89 at r10 (raw file):
'Should return the state as present'? instead of 'Should not..' Tests/Unit/xSQLServerRole.Tests.ps1, line 119 at r10 (raw file):
All of these tests are just checking to see what boolean value that Test-Target resource returns - not the actual state - can you change the It statements here to reflect that? Tests/Unit/xSQLServerRole.Tests.ps1, line 214 at r10 (raw file):
Set-TargetResource doesn't return anything - so just check that the correct mocks are called. Change the It statements here to reflect that too, since this function isn't returning a state. Comments from Reviewable |
|
Also, now that there's been another PR merged the conflicts are going to need to be resolved again. Thanks for your patience! |
|
I totally missed all the |
|
Reviewed 6 of 8 files at r11, 2 of 2 files at r12. DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 25 at r10 (raw file):
|
|
Reviewed 6 of 8 files at r11, 2 of 2 files at r12. DSCResources/MSFT_xSQLServerRole/MSFT_xSQLServerRole.psm1, line 25 at r10 (raw file):
|
|
Reviewed 3 of 3 files at r13. Tests/Unit/xSQLServerRole.Tests.ps1, line 136 at r12 (raw file):
|
|
Reviewed 1 of 3 files at r13. Comments from Reviewable |
|
Reviewed 2 of 3 files at r13. Comments from Reviewable |
mbreakey3
removed
the
waiting for author response
With the SQL Server Role DSCResource, we can manage the SQL Server Role
security.
Of course in matter of security, we need to use a sysadmin account to configure these roles
This change is