Repository navigation
Code Review #30
Description
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
Activity
csandfeld commentedon Nov 27, 2015
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 commentedon Dec 1, 2015
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 commentedon Dec 2, 2015
@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 commentedon Dec 2, 2015
@DexterPOSH Welcome on board! If anything matches the above criteria etc, put a #TODO , a brief comment and send a PR 😄
DexterPOSH commentedon Dec 3, 2015
@iainbrighton I will do that. Also will try to put in how it works based on my understanding, you can later enhance it.
csandfeld commentedon Dec 4, 2015
OK, I will jump in on a "best effort" basis :)
csandfeld commentedon Dec 11, 2015
@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 commentedon Dec 11, 2015
@iainbrighton The Script Analyzer rule 'PSMissingModuleManifestField' complains about a missing missing module version field in the manifest:
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 commentedon Dec 11, 2015
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 commentedon Dec 11, 2015
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 commentedon Feb 16, 2016
@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 commentedon Feb 16, 2016
@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 commentedon Feb 16, 2016
@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:
iainbrighton commentedon Feb 16, 2016
@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?
12 remaining items
csandfeld commentedon Apr 3, 2016
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 commentedon Apr 3, 2016
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 commentedon Apr 3, 2016
iainbrighton commentedon Apr 3, 2016
csandfeld commentedon Apr 3, 2016
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 commentedon Apr 4, 2016
ryanspletzer commentedon Apr 20, 2016
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 commentedon Apr 24, 2016
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 commentedon Jul 13, 2016
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 commentedon Jul 13, 2016
@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/DhcpServerDsc#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 commentedon Sep 16, 2016
@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 commentedon Sep 16, 2016
@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 commentedon Sep 3, 2017
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 commentedon Jul 15, 2018
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 commentedon Jul 17, 2018
@MikeShepard Good spot! I'll get that corrected..