SmbShare: Adding new resource #211
Conversation
|
Working on integration tests next, and saw now that I missed some documentation in the resource folder, and the README.md. @PlagueHO testing a draft PR to see if it helps the review or discussions from the community. |
|
Oh right. Doh! I guess it should be have the needs review then! |
|
@PlagueHO The examples fails to compile in my fork because it sees multiple versions on ComputerManagementDsc. Is it something in my change that is causing that? See test run in my forks working branch: |
|
@johlju - I don't know if this is the cause of the issue, but you do need to rebase I think as you need to pick up the more recent changes. I couldn't see anything in your code that looked like it would cause this problem! That is strange. Does it work locally? |
|
P.S. as soon as #195 is through I'm going to refactor this module to get rid of the harness - which will make it simpler and easier to debug issues like this. |
|
Yep, I will rebase as soon as I get the last of the integration tests fixed. After that I take it out of draft and AppVeyor will run for the PR. We debug after that why there are two modules present on the node, if the problem is still there. |
|
Updated to reflect new knowledge. I have a slight problem with this resource, and it is the default behavior of the cmdlet SmbShare 'Integration_Test'
{
Name = 'MyShare'
Path = 'c:\Temp'
FullAccess = @()
ChangeAccess = @()
ReadAccess = @()
NoAccess = @()
}The share would be created with the group 'Everyone' having read access to the share. This is because the cmdlet It would not be possible to remove the 'Everyone' access permission either, because we are only allowed to have on instance of this resource. I see some options to solve this.
|
Ok this is weird. The spec does say that default is none. If i pass in None, I get a user account None. The sid is an actual user though and it's not using a built-in sid. This led me into trying S-1-0-0, which does work! I would use that. |
|
Actually it seems that if removing the automatically added 'Everyone' from read access, then 'Everyone' is instead moved to NoAccess property so that Everyone in 'Denied'. It seems that by design an access rule must exist. If adding a user to any other access permission, 'Everyone' is not added. So this is only a problem when specifying all access permission with empty collections, see below. @kungfu71186 thanks for finding the If it find the |
|
if using this configuration, will will ignore and just assume that the user knows the default is that Everyone is added as read access permission. We should note it in the documentation though. |
|
After thinking on this during the day, I think we instead just throw an error if all of the access permission parameters are configured with empty collections. The reason is that it is not possible to modify the permissions manually if using an configuration where all the access permission parameters are empty collections, the manually added users are always removed. Instead it should throw an error saying that either remove the access permission parameters completely or add an account or group to one of the access permission parameters. |
|
Seems reasonable. Honestly, this is a very special edge case that probably shouldn't be used anyway. Otherwise you can just specify absent and your problems are solved and it's more secure. I think leaving the default to everyone read access if nothing is specified is fine as well. But you have options now :) Maybe just leave a note in there that it's intentional to not support it as well for the reason you stated above so if anyone ever makes an issue of it, we can quickly say it's already been thought of. |
|
@kungfu71186 That was my thought as well, if there should be no permission then it would be better to just remove the share. 😄 Yes I appreciate the help, because that was led me to this. 🙇 |
Codecov Report
@@ Coverage Diff @@
## dev #211 +/- ##
====================================
+ Coverage 83% 85% +2%
====================================
Files 10 11 +1
Lines 1074 1242 +168
====================================
+ Hits 897 1064 +167
- Misses 177 178 +1 |
|
@PlagueHO This is ready for review now. I'm debugging to find where and how it gets two instances of the module during testing. |
|
Returning two module folder locations from First one is imported here: Second one is here: The problem might be that the meta tests are not run first as it is in a "non-harness modul". But why is the problem showing now. 🤔 |
|
Awesome stuff @johlju - I'll review this weekend! Sorry about not commenting earlier. Day job :/ |
|
That is really strange. Perhaps it is just better if I do the work to get this off Harness mode? It won't take me long to do - maybe a couple of hours? |
|
@PlagueHO Yeah, you were planning on doing it anyway, so lets do that. But no hurry, I won't be available this weekend to fix any review comments, so take your time. 😄 I just ran the dev version, and it is passing. Maybe it is the unit test template or integration test template that messes this up. 🤔 |
|
No problem, I will help review 😄 |
There was a problem hiding this comment.
Reviewable status: 0 of 15 files reviewed, 1 unresolved discussion
Tests/Integration/MSFT_SmbShare.config.ps1, line 171 at r2 (raw file):
#ContinuouslyAvailable = $true
I missed this! I need to create a virtual drive I think so we can test till property. Forgot to look into it.
https://blog.workinghardinit.work/tag/continuously-available-file-shares/
There was a problem hiding this comment.
Reviewable status: 0 of 15 files reviewed, 3 unresolved discussions
Modules/ComputerManagementDsc/DSCResources/MSFT_SmbShare/MSFT_SmbShare.psm1, line 77 at r2 (raw file):
$returnValue['FolderEnumerationMode'] = $smbShare.FolderEnumerationMode
This returns the wrong type.
VERBOSE: [APPVYR-WIN]: [[SmbShare]Integration_Test] NOTMATCH: Type mismatch for property 'FolderEnumerationMode' Current state type is 'FolderEnumerationMode' and desired type is 'String'.
https://ci.appveyor.com/project/PowerShell/computermanagementdsc/builds/23632778?fullLog=true#L3772
Modules/ComputerManagementDsc/DSCResources/MSFT_SmbShare/MSFT_SmbShare.psm1, line 78 at r2 (raw file):
$returnValue['CachingMode'] = $smbShare.CachingMode
This returns the wrong type.
VERBOSE: [APPVYR-WIN]: [[SmbShare]Integration_Test] NOTMATCH: Type mismatch for property 'CachingMode' Current state type is 'CachingMode' and desired type is 'String'.
https://ci.appveyor.com/project/PowerShell/computermanagementdsc/builds/23632778?fullLog=true#L3776
|
I will look into getting the missed integration tests into this PR this weekend, and also rebase when the refactor PR is merged. |
|
@PlagueHO rebased 😄 |
|
Time to review! 😁 |
There was a problem hiding this comment.
Looking good @johlju - just some minor tweaks and we can merge! Woo!
Reviewed 1 of 2 files at r4, 5 of 12 files at r5.
Reviewable status: 8 of 15 files reviewed, 33 unresolved discussions (waiting on @johlju and @PlagueHO)
CHANGELOG.md, line 5 at r5 (raw file):
## Unreleased - xComputer:
Just noticed this is incorrect - should be Computer - not xComputer - can you correct it?
CHANGELOG.md, line 8 at r5 (raw file):
- Fix for 'directory service is busy' error when joining a domain and renaming a computer when JoinOU is specified - Fixes [Issue #221](https://github.com/PowerShell/ComputerManagementDsc/issues/221). - Added new resource
Do we want to put the name of the resource we are adding on this line? E.g.
- Added new resource SmbShare
- Moved and improved from deprecated module xSmbShare.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 123 at r5 (raw file):
} It 'Should mock call to Get-SmbShare and return membership' {
It doesn't look like we're actually checking that the mock is called until the next assertion. So should this just say "Should return the correct values"?
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 141 at r5 (raw file):
It 'Should call the mock function Get-SmbShare' { $getTargetResourceResult = Get-TargetResource @testParameters
Do we need to recall Get-TargetResource here?
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 146 at r5 (raw file):
It 'Should Call the mock function Get-SmbShareAccess' { $getTargetResourceResult = Get-TargetResource @testParameters
Do we need to recall Get-TargetResource here?
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 183 at r5 (raw file):
$getTargetResourceResult.NoAccess | Should -HaveCount 0 }
Should we also assert that Get-SmbShare was called?
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 223 at r5 (raw file):
Context 'When no access permission is given' { It 'Should call the correct mocks' {
I can't actually see any Assert-MockCalled in this it block - shouldn't there be based on the description?
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 350 at r5 (raw file):
} }
Remove extra blank line.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 356 at r5 (raw file):
Describe 'MSFT_SmbShare\Test-TargetResource' -Tag 'Test' { Context 'When the system is not in the desired state' { Context 'When no members in provided in neither access permission collection' {
This context doesn't seem to make sense? Could it be reworded? "When no member are provided in any of the access permission collections"?
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 683 at r5 (raw file):
} It 'Should call the correct mock' {
mock->mocks - also, it should not throw an exception.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 709 at r5 (raw file):
} It 'Should call the correct mock' {
mock->mocks - also, it should not throw an exception.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 735 at r5 (raw file):
} It 'Should call the correct mock' {
mock->mocks - also, it should not throw an exception.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 767 at r5 (raw file):
} It 'Should call the correct mock' {
mock->mocks - also, it should not throw an exception.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 812 at r5 (raw file):
} It 'Should call the correct mock' {
mock->mocks - also, it should not throw an exception.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 863 at r5 (raw file):
} It 'Should call the correct mock' {
mock->mocks - also, it should not throw an exception.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 890 at r5 (raw file):
} It 'Should call the correct mocks' {
Also, it should not throw an exception.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 921 at r5 (raw file):
} It 'Should call the correct mock' {
mock->mocks - also, it should not throw an exception.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 947 at r5 (raw file):
} It 'Should call the correct mock' {
mock->mocks - also, it should not throw an exception.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 975 at r5 (raw file):
} It 'Should call the correct mock' {
mock->mocks - also, it should not throw an exception.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 1018 at r5 (raw file):
} It 'Should call the correct mock' {
mock->mocks - also, it should not throw an exception.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 1095 at r5 (raw file):
} Context 'When providing no member in either of the access permission collections' {
Could read - When not providing any members in any of the access permission collections"
DSCResources/MSFT_SmbShare/MSFT_SmbShare.psm1, line 304 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Done. This is already supported. The helper function Remove-SmbShareAccessPermission will only remove accounts that should not have permission (it does not remove all permissions, at least that was my intention, and I think it is coded that way). 🤔
Maybe the naming of the functions are less then good. Maybe we should add both theRemove-andAdd-to aSet-instead? If so I could add an issue to track that.
Let's see how it goes. I think it should be OK to leave as is for now - because we're not going to be continuously applying set.
DSCResources/MSFT_SmbShare/MSFT_SmbShare.psm1, line 549 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Done. I think I overdid the comments in this code just to make sure I remembered all the stuff it should do, or did. 🙂
I completely understand and do the same thing 😁
DSCResources/MSFT_SmbShare/MSFT_SmbShare.psm1, line 675 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Done. Added an issue to track this instead, you could label it as 'good first issue' too. 😄 Did this so we can get this one through.
Sounds good!
DSCResources/MSFT_SmbShare/MSFT_SmbShare.psm1, line 273 at r5 (raw file):
} $parametersToRemove | ForEach-Object {
Should have -Process parameter.
There was a problem hiding this comment.
I think I got them all! 😄
Reviewable status: 4 of 17 files reviewed, 33 unresolved discussions (waiting on @PlagueHO)
CHANGELOG.md, line 5 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Just noticed this is incorrect - should be Computer - not xComputer - can you correct it?
Done.
CHANGELOG.md, line 8 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Do we want to put the name of the resource we are adding on this line? E.g.
- Added new resource SmbShare - Moved and improved from deprecated module xSmbShare.
Done.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 123 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
It doesn't look like we're actually checking that the mock is called until the next assertion. So should this just say "Should return the correct values"?
Done.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 141 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Do we need to recall Get-TargetResource here?
Done. If there is a seperate It-block I my opinion the call to the function being tested should always be called in the It-block (instead of using -Scope Context). In this case I think that was left overs from the refactored test, I normally don't do that, so I just moved the the asserts up to the first It-block. No need to have separate It-blocks for asserting the calls to the mocks. 🤔
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 146 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Do we need to recall Get-TargetResource here?
Done.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 183 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Should we also assert that Get-SmbShare was called?
Done. I added it. I think I left it out since the first context block did that test, so I think left it out to speed up the tests. :)
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 223 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
I can't actually see any
Assert-MockCalledin this it block - shouldn't there be based on the description?
Done.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 350 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Remove extra blank line.
Done.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 356 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
This context doesn't seem to make sense? Could it be reworded? "When no member are provided in any of the access permission collections"?
Done. Thanks for this. 😃 I did not understand that either, sometimes it feels like I don't even read back what I wrote 😆
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 683 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
mock->mocks - also, it should not throw an exception.
Done.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 709 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
mock->mocks - also, it should not throw an exception.
Done.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 735 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
mock->mocks - also, it should not throw an exception.
Done.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 767 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
mock->mocks - also, it should not throw an exception.
Done.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 812 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
mock->mocks - also, it should not throw an exception.
Done.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 863 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
mock->mocks - also, it should not throw an exception.
Done.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 890 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Also, it should not throw an exception.
Done.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 921 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
mock->mocks - also, it should not throw an exception.
Done.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 947 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
mock->mocks - also, it should not throw an exception.
Done.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 975 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
mock->mocks - also, it should not throw an exception.
Done.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 1018 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
mock->mocks - also, it should not throw an exception.
Done.
Tests/Unit/MSFT_SmbShare.Tests.ps1, line 1095 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Could read - When not providing any members in any of the access permission collections"
Done. Read much better, thanks! 😄
DSCResources/MSFT_SmbShare/MSFT_SmbShare.psm1, line 273 at r5 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
Should have -Process parameter.
Done. Throughout the repo. Fixed the -FilterScript throughout the repo too.
|
@PlagueHO Hope that was it! Ready for review again. 😃 |
|
Awesome @johlju -will get onto it tonight. |
There was a problem hiding this comment.
Awesome job @johlju - just one FYI and we're good to go.
Reviewed 7 of 12 files at r5, 6 of 6 files at r6.
Reviewable status:complete! all files reviewed, all discussions resolved
DSCResources/MSFT_WindowsEventLog/MSFT_WindowsEventLog.psm1, line 180 at r6 (raw file):
if ($PSBoundParameters.ContainsKey('LogRetentionDays')) { if ($LogMode -eq 'AutoBackup' -and (Get-EventLog -List | Where-Object -FilterScript {$_.Log -like $LogName}))
FYI, I know this is existing code, but maybe we could make a change to improve this code:
Why not assign the (Get-EventLog -List | Where-Object -FilterScript { $_.Log -like $LogName }) to a variable before the if block and then you can reuse it within the if block and won't have to run the same expression again. This would improve performance.
There was a problem hiding this comment.
Reviewable status:
DSCResources/MSFT_WindowsEventLog/MSFT_WindowsEventLog.psm1, line 180 at r6 (raw file):
Previously, PlagueHO (Daniel Scott-Raynsford) wrote…
FYI, I know this is existing code, but maybe we could make a change to improve this code:
Why not assign the
(Get-EventLog -List | Where-Object -FilterScript { $_.Log -like $LogName })to a variable before the if block and then you can reuse it within the if block and won't have to run the same expression again. This would improve performance.
Created issue for this since I only did a style change (already outside the scope of the PR), and the two uses different evaluations, one uses -like and one uses -eq. Fixing that means potentially changing tests, and could change functionality, meaning I need to dig deeper to resolve, that is outside the scope of this PR. 🙂 Let's make that issue a 'good first issue' so someone new in the community can contribute to this.
There was a problem hiding this comment.
Reviewable status:
DSCResources/MSFT_WindowsEventLog/MSFT_WindowsEventLog.psm1, line 180 at r6 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Created issue for this since I only did a style change (already outside the scope of the PR), and the two uses different evaluations, one uses
-likeand one uses-eq. Fixing that means potentially changing tests, and could change functionality, meaning I need to dig deeper to resolve, that is outside the scope of this PR. 🙂 Let's make that issue a 'good first issue' so someone new in the community can contribute to this.
Ah yes, you're right. I think I recall seeing that at the time when I first reviewed this and decided not to address it yet. So good idea about issue.
There was a problem hiding this comment.
Reviewable status:
Pull Request (PR) description
This moves the resource xSmbShare to the module ComputerManagementDsc, and deprecate the xSmbShare module.
This Pull Request (PR) fixes the following issues
n/a
Task list
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
This change is