Code Review

[iainbrighton]

Before publishing to the gallery, we need to perform a code review. This should (at a minimum) cover:

• Running the PSScriptAnalyzer
  • Ignoring the pesky 'PSProvideDefaultParameterValue' rule!
  • A quick run shows no errors, but a few warnings (excluding the above rule)!
• Check that ShouldProcess is implemented where necessary
  • Highlighting where you think -WhatIf and/or -Confirm should if necessary would be valuable
• Check that pipeline input is accepted
  • Pipeline support should be in place for all publicly exposed cmdlets

[csandfeld]

Not sure my skills are adequite, but will be happy to help. Just let me know what part you would like me to check, and I'll do my best.

[iainbrighton]

I'm sure you'll be great! Looking through the code and where you don't understand what/why something is happening will mean I need to add addition comments. If this is going to turn into a community effort, everyone needs to understand what's going on before they can join in!

[DexterPOSH]

@iainbrighton @csandfeld I am going through the Code in order to learn the approach you have used here. I would like to pitch in for this effort.

[iainbrighton]

@DexterPOSH Welcome on board! If anything matches the above criteria etc, put a #TODO , a brief comment and send a PR 😄

[DexterPOSH]

@iainbrighton I will do that. Also will try to put in how it works based on my understanding, you can later enhance it.

[csandfeld]

OK, I will jump in on a "best effort" basis :)

[csandfeld]

@iainbrighton The Script Analyzer rule 'PSUseSingularNouns' deals with function names, not following the the 'singular noun' best practice.

In this project it throws a warning about the *-LabHostDefaults and *-LabVMDefaults functions. I can start changing them to singular nouns, but wanted to run that by you before I go down that route. What's your take on it?

[csandfeld]

@iainbrighton The Script Analyzer rule 'PSMissingModuleManifestField' complains about a missing missing module version field in the manifest:

• .\en-US\CustomMedia.psd1
• .\en-US\CustomResource.psd1
• .\en-US\Example.psd1
• .\Resources.psd1

What's your take on that warning? Can it be ignored (as a module version has no relevance in these files), or should we add a field to make the script analyzer happy?

[iainbrighton]

This sounds like it needs to filed as an issue on the PSScriptAnalyzer GitHub page. The script analyzer is assuming that all .psd1 files are module manifests when they’re just PowerShell “data files”. The most common usage is manifests, but Import-LocalizedData requires their usage too and that’s being flagged. Therefore, I think this is a bug?

[iainbrighton]

I have no problem with renaming them to singular nouns. If we add an alias then we won’t break anything – other than the tests?!

[ryanspletzer]

@iainbrighton I've been reviewing the codebase over at at fork of Dev:

https://github.com/ryanspletzer/Lability

Did some basic cleanup / typo fix commits.

Is there a reason that xHyper-V and xPendingReboot are bundled with the repo vs. being pulled in separately from PowerShell gallery?

[DexterPOSH]

@ryanspletzer
From what I see xPendingReboot is the DSC resource needed for the host configuration done via Start-LabHostConfiguration (bare bone requirement).

And xHyper-V DSC resource is needed to provision the VMs.
These are the bare minimum DSC resources needed for the config, maybe that piece can be automated too.

[iainbrighton]

@ryanspletzer Thanks for the contribution! I'm not sure all the formatting changes are necessary, but the typos are definitely gratefully received. Send the PR and we can discuss.

As for the bundled modules, they are only used internally to configure the host, virtual switches and VMs. They were originally external to the module. They have been moved internally for two main reasons:

1. To avoid conflicts with locally installed DSC resource versions. We don't want to enforce a particular version of the xHyper-V or xPendingReboot modules upon anyone.
2. Bugs fixes and new functionality is easier to add. The fixes/additions are pushed upstream to the public repositories, but it can take months to get changes merged in and made available in the PSGallery.

[iainbrighton]

@ryanspletzer Whilst we're on the subject of documentation.. As you've probably read through most of the documentation, have you noticed any glaring omissions that need to be added?

[ryanspletzer]

@iainbrighton Understood on the formatting, I went a little overboard there, we don't have to include all that. :)

For reference some of that comes from PowerShellPracticeAndStyle which you may be familiar with, specifically Attribute Parameters: Using =$true or Not #52 but since this would only be v4+ probably not necessary.

Understood on the bundled modules now, makes sense.

I took a first pass through the docs to familiarize myself with the overall project, just correcting some typo / grammatical stuff as I saw it, but I'll take another pass at it shortly and see if there's anything else that should be added / touched on.

I do plan on actually running the project based on your PSHSummit-Man-vs-Testlab example to see what kinks I run into so this will probably help inform which further docs might be needed.

[iainbrighton]

@ryanspletzer If you use those examples, you will now need to change VirtualEngineLab references to Lability but you've probably already worked that out! There have also been updates to DSC resources that fix a few things and you can now attach the EDGE server to multiple virtual switches. If you need a hand with updating the config when you come to look at it, just let me know.

Yep - I'm aware of the style guidlines and disagree with some of it. Whilst I'm not a fan of the =$true, more explicit is always better. I lieu of not having published/contribution guidelines for this repository, I'll live 😃

[ryanspletzer]

@iainbrighton Yep I worked through that rename once for the Man vs Test Lab stuff, gonna try it again here soon from my fork of that.

There are pieces of the style guidelines I disagree with, too. (Max 116 chars per line?! Very tough to stick to, I've tried... And for what true purpose...)

But beyond that the other thing I rolled through the files and looked at was trailing whitespace. An editor like VS Code has a setting that you can turn on to auto-trim out trailing whitespace, but the ISE most likely doesn't...

[ryanspletzer]

Alrighty, I've been reading through the code, couple tiny fixes in #70 pull request (I started fresh and re-forked, left out all those crazy initial reformatting stuff I did, not necessary), I'm sure more to come.

I'm using an updated fork of PSHSummit-Man-vs-Testlab with the Lability renamed stuff to test through it a bit.

One issue I ran into:

VERBOSE: [7:19:09 PM] Invoking command 'Set-VMTargetResource'.
DEBUG: Command parameter: 'ProcessorCount' = '2'.
DEBUG: Command parameter: 'SwitchName' = 'System.String[]'.
DEBUG: Command parameter: 'MinimumMemory' = '536870912'.
DEBUG: Command parameter: 'RestartIfNeeded' = 'True'.
DEBUG: Command parameter: 'Generation' = '2'.
DEBUG: Command parameter: 'VhdPath' = 'D:\TestLab\VM Disks\DC1.vhdx'.
DEBUG: Command parameter: 'SecureBoot' = 'True'.
DEBUG: Command parameter: 'StartupMemory' = '1610612736'.
DEBUG: Command parameter: 'MaximumMemory' = '1099511627776'.
DEBUG: Command parameter: 'Name' = 'DC1'.
New-VM : Sequence contains more than one element
At C:\Program Files\WindowsPowerShell\Modules\Lability\DSCResources\xHyper-V\DSCResources\MSFT_xVMHyperV\MSFT_xVMHyperV.psm1:350 char:21
+             $null = New-VM @parameters
+                     ~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : InvalidOperation: (:) [New-VM], VirtualizationException
    + FullyQualifiedErrorId : InvalidOperation,Microsoft.HyperV.PowerShell.Commands.NewVM

The xHyper-V resource included with Lability is complaining when it calls New-VM, as New-VM just expects a System.String for SwitchName but it's being passed a System.String[].

The ConfigurationData parameter for Start-LabConfiguration pulls from a file in the PSHSummit-Man-vs-Testlab fork here: TLGVirtualEngineLab.psd1

I didn't really change that file besides the VirtualEngineLab -> Lability parameter renames. The DC1 node that it's choking on doesn't seem to have multiple switches configured (unless I'm reading it wrong), so I'm not quite sure yet where in the chain the System.String[] is getting passed to New-VM... or how best to fix it.

Any ideas on if I'm doing something wrong or if something here needs fixing?

[iainbrighton]

@ryanspletzer Geez, you were mighty unlucky here. What you're seeing is a result of merging the latest xVMHyper-V DSC resources in v0.9.7 and then picking up this dsccommunity/HyperVDsc#48 bug.

In the short-term you can pull in this branch (or mirror the changes in DSCResources/xHyper-V/DSCResources/MSFT_xVMSwitch/MSFT_xVMSwitch.psm1 locally in your repo) https://github.com/iainbrighton/Lability/tree/dev. Once the PR has been agreed, I will then merge the changes in.

In this branch, I did spend some time updating the example.ps1 and example.psd1 files to take account of new functionality available in DSC resources and renamed them to TestLabGuide. The updated \Examples\TestLabGuide.ps1 and \Examples\TestLabGuide.psd1 should see you good (excluding the above bug!).

[ryanspletzer]

@iainbrighton I synced up my fork with #72 and gave it another go with the Man vs. Test Lab example and it worked! :)

I'll work off TestLabGuide examples that come with Lability from here on out.

I'll continue reviewing the code and docs and let you know if I run across anything. Thanks!

[iainbrighton]

@ryanspletzer Great to hear! Let me know if you have any further problems/questions 😃

[csandfeld]

@iainbrighton do you have a general idea of what still needs doing when it comes to reviewing the code?

[iainbrighton]

From memory there's the ShouldProcess support to add to the Set- and Remove- cmdlets. I've done some of them. Other than that - whatever else is flagged by the Script Analyzer.

[csandfeld]

I can take a stab at adding ShouldProcess to those cmdlets if I can find time for it. Any thoughts on what you would prefer ConfirmImpact set to?

I see that 'SupportsShouldProcess' is added to CmdletBinding() on some of them already, but for those I checked, it did not seem to be implementet in the code (no $PSCmdlet.ShouldProcess() found). Is this on pupose?

[iainbrighton]

Thanks for volunteering :)

I've left the default ConfirmImpact at medium for the time being.

As for the .ShouldProcess() you'll need to test as all cmdlets in any called sub functions could prompt. If you look at the ones that have it implemented already, you should see some -Confim:$false on some nested functions/cmdlets to leave just the relevant prompts..

Does this make sense?

[csandfeld]

Yep, I think I get what you are saying :)

[csandfeld]

Unrelated to the above, but have been wondering for some time now. Some code is placed in the 'Src' folder, while other code is placed in the 'Lib' folder. What criteria dictates where code is placed?

[csandfeld]

I have started adding ShouldProcess support to Remove-LabConfiguration and the called sub-cmdlets. When done I would like you to review the changes before I move on.

[iainbrighton]

No hard and fast rule. Generally, Lib contains internal/generic functions and Src are the public functions.

[iainbrighton]

No problem =)

[csandfeld]

Been working a bit on ShouldProcess for Remove-LabConfiguration and sub Cmdets today. Not all done yet (need to set -Confirm:$false here and there), but when you get a couple of minutes @iainbrighton, could you look through 64325ec and see if I am on the right path here?

[iainbrighton]

No problem. I'm busy this week but will hopefully get a chance of an evening. Thanks again :)

[ryanspletzer]

More of a style thing -- I noticed in certain places [string] is used where in other places the full [System.String] is used. Other examples are [System.Management.Automation.SwitchParameter] which is used largely instead of [switch].

Is the preference in this module to use the fully qualified type names wherever possible?

[iainbrighton]

Hi @ryanspletzer - I do tend to fully qualify everything, as we can never be too explicit and it's a C# habit (just like the semicolons). That's not to say that I wouldn't accept a PR that isn't fully qualified or missing semi-colons!

[ryanspletzer]

Can I ask another lame question? Line 130 in TestLabGuide.ps1, what is that commented out #Router = '10.0.0.2'; all about?

Even xDhcpServer doesn't have this Router parameter documented well, and I let them know about it.

[iainbrighton]

@ryanspletzer Erm - I'm not entirely sure! It could be because the default gateway didn't work in my lab environment or (judging by dsccommunity/xDhcpServer#26) it might be because it didn't work. However, if it was the latter, I'm surprised I either didn't log an issue or fix it.. Not particularly helpful, I know 😞

[brycem]

@iainbrighton - Unless I uncomment that router parameter, I was having xDhcpServer config fail (OS 2016 Server Standard) - Sorry, didn't capture the failure from the failed run.

[iainbrighton]

@brycem Yeah - a xDhcpServer DSC resource update broke that. It was fixed, but seems that it's crept back into the latter xDhcpServer releases..

[DevOpsBoondoggles]

Not sure you still need help but I spent some time on a testlab set of scripting before finding your one which is WAAAY better.
Am not great but happy to help

[MikeShepard]

I just noticed that Get-WindowsImageByIndex and Get-WindowsImageByName are switched in the ps1 files (i.e. ByIndex is in ByName.ps1 and vice versa).

Not a big deal, but it did confuse me for a bit.

[iainbrighton]

@MikeShepard Good spot! I'll get that corrected..