Enhancing module

[twerthi]

Added the ability to have all Access variables present, but allow for null or empty.

---

This change is 

[johlju]

Review status: 0 of 1 files reviewed at latest revision, all discussions resolved.

---

DscResources/MSFT_xSmbShare/MSFT_xSmbShare.psm1, line 189 at r1 (raw file):

                Write-Verbose "Parameter ChangeAccess is null or empty, removing from collection."
                # Remove the parameter
                $psboundparameters.Remove("ChangeAccess")

Curious why we need to do this. If the parameter is not passed to Set-TargetResource then is it not already not part of $PSBoundParameters?

---

Comments from Reviewable

[stale]

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.

[twerthi]

Holy cow, I did not see that anyone had looked at this! My apologies.

If the parameters were empty or null, the setting was failing completely. If they weren't part of the parameters being passed in, then it safely applied what was being requested.

[johlju]

Maybe we should use ValidateNotNullOrEmpty instead on the parameters? If we should not allow empty values, that should be changed on both the Set- and Test-TargetResource.

Would that work for the intended behavior?

[johlju]

Also, is there a scenario where any of the parameters need to be assigned an empty value? So we don't do a breaking change.

[twerthi]

The ValidateNotNullOrEmpty would prevent anyone passing in null or empty. The scenario that I am using is

				xSmbShare "Share-$($Folder.Share.Name)"
				{
					Name = $Folder.Share.Name
					Ensure = "Present"
					FullAccess = $Folder.Share.FullAccess
					ReadAccess = $Folder.Share.ReadAccess
					ChangeAccess = $Folder.Share.ChangeAccess
					Path = $Folder.PhysicalPath
				}

In my data document, I may or may not have anything specified for ChangeAccess, for example, so that would be a null. I wanted the data document to be flexible enough so that you don't have to specify all of the different levels, only the ones you wanted to change. Does that make sense?

[johlju]

Ah, I see what you are trying to do. I don't think this is the right approach. If a property should not be checked to be in a consistent state it should not be part of the configuration. This way if ChangedAccess on the target node would be anything other than an empty value the configuration would never become in a desired state.
The configuration should be built so that only the properties that should be in desired state is present.

[twerthi]

I think I understand, but I'm going to paraphrase to make sure we're on the same page. Given the scenario where User A has ChangeAccess and we set ChangeAccess to null, the configuration won't do anything with ChangeAccess and User A will retain the permission. Your point is that in this case, User A ChangeAccess should be removed, is that correct?

[johlju]

I think User A's change access should be removed yes, because that is the desired state we want.

Let say we add a share 'TestShare', and we say the ChangeAccess should be $null (none). Then we want the desired state to be that no one should have change access.
If user A would be added manually to the share for change access. Then our configuration should remove user A, for the configuration to be in desired state.
If we want to keep user A that was added manually, we should not add the property ChangeAccess to the configuration. If the property is not present in the configuration the resource should not determine that properties state.

[twerthi]

Gotcha. You are correct, my solution does not currently handle that. The removing of the parameters is still necessary because the SMS Share cmdlets will fail otherwise. However, I need to change my solution so that users that have existing permissions that are not included in the specified desired state are added to the NoAccess collection, which will then remove their permissions.

[twerthi]

That is incorrect, adding them to the NoAccess list only sets them to an explicit Deny versus removing them from the list. I found that the original author checked to see if collection was null, if it wasn't, they removed all users with that access level and replaced it with the collection. I have moved the removal out of the if statement and have it removing and then adding if the collection has anything in it. This should ensure the correct desired state.

[johlju]

Review status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @twerthi)

---

a discussion (no related file):
There are currently no unit tests for this resource. Could you please add unit tests, for at least the functionality that this change affect?

The unit tests should be added to \Tests\Unit, and the unit test template is here https://github.com/PowerShell/DscResources/blob/master/Tests.Template/unit_template.ps1

---

Comments from Reviewable

[twerthi]

I've not done these before, so it might take a little while as I work it into my schedule. Thank you for providing the template that should help speed things up :)

[twerthi]

@johlju, I'm wondering if I can get a little guidance as I've not done this before. The template calls a function Initialize-TestEnvironment, which isn't included in the script, may I assume it's a known method that creates a test VM of some sort? If so, is there documentation that describes what this VM looks like in terms of drives so that I can perform the setup of a share to test with?

[johlju]

@twerthi absolutely, happy too! 😃 Let me know if you have any further questions..

Tests are run using AppVeyor - AppVeyor provides build workers (VM's) and if not specified, by using image keyword in the appveyor.yml file in the root of the repository, then the default build worker name is (for now) 'Visual Studio 2015'.
The build worker is a normal Windows Server 2012 R2, with a bunch of tools installed to help building and testing. But see it as a clean Windows Server 2012 R2 for this purpose.

Tests are started by appveyor.yml (AppVeyor control file) and tests is run under the test framework DscResources.Tests which is cloned on run-time (even when you run tests locally with Invoke-Pester). Initialize-TestEnvironment is a helper function in the test environment. So everything in the header and footer of the test templates can safely be ignored, it just to get the prerequisite in place.
The test framework also has some common tests (for all resource modules) that needs to pass.

For example, you can find the test run for your PR if you click 'Details' on this (just below here).

In AppVeyor you could create a free account (using your GitHub account) and can connect your fork to AppVeyor (add your fork as a project under Projects in AppVeyor). That way you can test your working branches, and send in PR's when you see they are passing tests.

There are two types of tests - unit test and integration tests. I will only expect you to write a unit test. But if you want to give an integration tests a go it would be much appreciated.
Unit test does not actually perform any actions, it just mocks the calls so it's possible to assert that the call mas wade. Integration tests actually performs the action, for example actually creates the share. Due to the limitations of AppVeyor some integration tests can not be done, for example it is not possible to restart the build worker during testing.

Example of unit tests for a resource: https://github.com/PowerShell/SqlServerDsc/blob/dev/Tests/Unit/MSFT_SqlServerDatabaseMail.Tests.ps1

Example of integration tests for a resource: https://github.com/PowerShell/SqlServerDsc/blob/dev/Tests/Integration/MSFT_SqlServerDatabaseMail.Integration.Tests.ps1 and the accompanying configuration https://github.com/PowerShell/SqlServerDsc/blob/dev/Tests/Integration/MSFT_SqlServerDatabaseMail.config.ps1

I realized that we don't have that much information around this in DSC Resource Kit. But below is what I could find (created an issue to update the documentation).

More information around AppVeyor: https://github.com/PowerShell/DscResources/blob/master/CONTRIBUTING.md#tests-in-appveyor

Test guidelines: https://github.com/PowerShell/DscResources/blob/master/TestsGuidelines.md

Test framework (just for reference, normally only necessary read for new resource modules): https://github.com/PowerShell/DscResource.Tests

[twerthi]

Ah!! Thank you, this helps tremendously!

[twerthi]

The pieces are finally fitting together in my head. I've only been able to work on this sporadically so it wasn't quite clicking. Spent some time on the 4th and I believe I understand now. Sorry it's taking so long.

[johlju]

@twerthi No worries, take your time. 🙂 If you get stuck, or need pointers, ping me and I try to help the best I can.

[twerthi]

I understand, been trying not to bug you too much since I can see you cover quite a large range of PRs. Think I got all requested changes complete.

[twerthi]

Is this one acceptable?

[stale]

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.

[johlju]

Reviewed 2 of 2 files at r18.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @twerthi)

---

DscResources/MSFT_xSmbShare/MSFT_xSmbShare.psm1, line 402 at r18 (raw file):

Foreach

foreach (lower-case 'f')

---

DscResources/MSFT_xSmbShare/MSFT_xSmbShare.psm1, line 408 at r18 (raw file):

if (($null -ne $differences) -and ($differences.Length -gt 0))

We should not need to enclose the evaluations in parenthesis?

This should do?

if ($null -ne $differences -and $differences.Length -gt 0)

---

DscResources/MSFT_xSmbShare/MSFT_xSmbShare.psm1, line 410 at r18 (raw file):

 -Verbose

We should only output verbose messages when the user want it, when they do for example Start-DscConfiguration -Verbose. So it should not default to print the verbose messages. I think we can remove -Verbose here?

[twerthi]

Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @johlju)

---

DscResources/MSFT_xSmbShare/MSFT_xSmbShare.psm1, line 402 at r18 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Foreach

foreach (lower-case 'f')

Done.

---

DscResources/MSFT_xSmbShare/MSFT_xSmbShare.psm1, line 408 at r18 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

if (($null -ne $differences) -and ($differences.Length -gt 0))

We should not need to enclose the evaluations in parenthesis?

This should do?

if ($null -ne $differences -and $differences.Length -gt 0)

I'm used to C# syntax, is this considered bad practice?

---

DscResources/MSFT_xSmbShare/MSFT_xSmbShare.psm1, line 410 at r18 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

 -Verbose

We should only output verbose messages when the user want it, when they do for example Start-DscConfiguration -Verbose. So it should not default to print the verbose messages. I think we can remove -Verbose here?

In a previous comment, you'd asked that the differences be displayed. I could not find another way to have the data be written to the DSC output from within a function call. If there is another/better way to do this, please let me know and I'll update :)

[johlju]

Reviewed 1 of 1 files at r19.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @twerthi)

---

DscResources/MSFT_xSmbShare/MSFT_xSmbShare.psm1, line 408 at r18 (raw file):

Previously, twerthi (Shawn Sesna) wrote…

I'm used to C# syntax, is this considered bad practice?

It good as-is.

---

DscResources/MSFT_xSmbShare/MSFT_xSmbShare.psm1, line 410 at r18 (raw file):

Previously, twerthi (Shawn Sesna) wrote…

In a previous comment, you'd asked that the differences be displayed. I could not find another way to have the data be written to the DSC output from within a function call. If there is another/better way to do this, please let me know and I'll update :)

Sorry, let me explain better this time. It's correct to write out the differences with Write-Verbose, but the parameter -Verbose should not be used. The parameter -Verbose forces the verbose output regardless if the user wants verbose output or not.
If we remove the parameter -Verbose then the verbose output will be seen when the user asks for verbose output.
With my initial comment I meant that this should be written out, but only when the users asks for the output.

Also so now that it was missing the Message named parameter. SO if you change to this, then it's all good,

$differences | ForEach-Object { Write-Verbose -Message $_ }

[twerthi]

Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @johlju)

---

DscResources/MSFT_xSmbShare/MSFT_xSmbShare.psm1, line 410 at r18 (raw file):

Previously, johlju (Johan Ljunggren) wrote…

Sorry, let me explain better this time. It's correct to write out the differences with Write-Verbose, but the parameter -Verbose should not be used. The parameter -Verbose forces the verbose output regardless if the user wants verbose output or not.
If we remove the parameter -Verbose then the verbose output will be seen when the user asks for verbose output.
With my initial comment I meant that this should be written out, but only when the users asks for the output.

Also so now that it was missing the Message named parameter. SO if you change to this, then it's all good,

$differences | ForEach-Object { Write-Verbose -Message $_ }

Done.

[twerthi]

We all good now?

[twerthi]

Checking in, anything that I'm missing?

[gaelcolas]

Hi @twerthi,
Thanks for pinging us.
I'll see if @johlju as time for a final review or I'll ask him to fill me in and finish that off.

Do @ me tomorrow if it hasn't moved forward.

[johlju]

On it.

[johlju]

Reviewed 1 of 1 files at r20.
Reviewable status: complete! all files reviewed, all discussions resolved

[johlju]

@gaelcolas can you merge this one as I have no permission to merge. :)

[gaelcolas]

@twerthi can you add an entry in the changelog describing your change? In the unreleased section on the reader.md. I'll use it to merge the pr first thing tomorrow.
Thanks

[twerthi]

This project doesn't appear to have a CHANGELOG.md file in the root, do I need to add one or is it located somewhere else?

[johlju]

@twerthi some repos have not split the change log into a seperate file. Then you find the unreleased section under the header changelog in the file README.md.

[gaelcolas]

Hi @twerthi, I've made the update to the changelog and made some cosmetic changes (variable capitalization).
I'll merge this after the tests passes.
Thanks again for the PR!

[gaelcolas]

Reviewed 1 of 2 files at r18, 2 of 2 files at r21.
Reviewable status: complete! all files reviewed, all discussions resolved

[twerthi]

Oh! I had fully planned on making the updates as requested, but got busy at work and am prepping for an event. Thank you very much for taking care of that :)

[gaelcolas]

No worries. Given we're all in different timezone I thought it'd be nice to close it before the monthly release tomorrow.