Code Review about lability HOT 41 OPEN

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.

from lability.

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!

from lability.

@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.

from lability.

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

from lability.

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

from lability.

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

from lability.

@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?

from lability.

@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?

from lability.

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?

from lability.

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?!

from lability.

@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?

from lability.

@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.

from lability.

@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.

from lability.

@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?

from lability.

@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.

from lability.

@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 😃

from lability.

@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...

from lability.

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?

from lability.

@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!).

from lability.

@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!

from lability.

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

from lability.

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

from lability.

from lability.

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?

from lability.

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?

from lability.

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

from lability.

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?

from lability.

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.

from lability.

from lability.

from lability.

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?

from lability.

from lability.

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?

from lability.

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!

from lability.

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.

from lability.

@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 😞

from lability.

@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.

from lability.

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

from lability.

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

from lability.

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.

from lability.

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

from lability.