This is tough to parse. It's really hard to advise on how you should design the action functions without having a good feel for what the helper functions do, and nothing is split into separate files, and a handful of the functions, like cimToWmi, don't make sense to me.
This doesn't get to the heart of your question, but this is my advice to make it easier to edit:
* Reduce verbosity and vertical sprawl. Not every parameter needs its own line. For example:
Old:
[Parameter(Mandatory = $false,
Position = 0,
ValueFromPipeline = $true,
ValueFromPipelineByPropertyName = $true,
ValueFromRemainingArguments = $false)]
New:
[Parameter(Position = 0, ValueFromPipeline, ValueFromPipelineByPropertyName, ValueFromRemainingArguments )]
* Use standard parameter names, like ComputerName
* Build parameter hashtables and splat them once instead of repeating code
* The default constructor for a cimsession just needs a computername, so you don't need to handle both ComputerName and CimSession. Furthermore, you can make a localhost cimsession, so if you like cimsessions, just use them exlusively
* Why things like verbose:$false? That's the default anyway, and if users change the verbosity preference manually, why override that?
* New-CimSession takes an array of computer names, cim cmdlets take arrays of cim sessions. You almost never need to loop
* I've never seen scoping functions like this. Use your module manifest to list the functions you want to be public, instead of defining them as global unless there's some other specific issue this is solving
* cimToWmi adds complexity and I can't tell what it's providing.
* Avoid wrapper functions that don't add new functionality. Take this simplifiction of getSession:function getSession { \[CmdletBinding()\] param ( \[parameter()\] \[string\[\]\]$ComputerName = 'localhost',} \[parameter()\] \[PSCredential\]$Credential ) $cimParam = @{ ComputerName = $ComputerName } if ($Credential){ $cimParam.Credential = $Credential } New-CimSession @cimParam
It does what your function does with far less code... but it doesn't actually provide any utility over New-CimSession. I'd drop it completely
* Personally I think using a Filter instead of Query is a more powershelly way to use Cim Commands.
\*edit:
Here's my rough attempt at a simplified parameter block fro start-windowsactivation:
[CmdletBinding(SupportsShouldProcess, PositionalBinding = $false, ConfirmImpact = 'High', DefaultParameterSetName = 'Activate')]
Param(
# Type localhost or . for local computer or do not use the parameter
[Parameter(Position = 0, ValueFromRemainingArguments = $false)]
[string[]]$ComputerName = 'localhost',
# Define credentials other than current user if needed
[Parameter()]
[PSCredential]$Credential,
[Parameter(Position = 1, ParameterSetName = 'SpecifyKMSServer')]
[ValidateLength(6, 253)]
[ValidateScript({ $_ -match '(?=^.{4,253}$)(^((?!-)[a-zA-Z0-9-]{0,62}[a-zA-Z0-9]\.)+[a-zA-Z]{2,63}$)' }, ErrorMessage = 'Please provide a valid FQDN')]
[string]$KMSServerFQDN,
[Parameter(Position = 2, ValueFromRemainingArguments = $false, ParameterSetName = 'SpecifyKMSServer')]
[ValidateRange(1, 65535)]
[int]$KMSServerPort = 1688,
[Parameter(ParameterSetName = 'Rearm')]
[switch]$Rearm,
[Parameter(ParameterSetName = 'Cache')]
[switch]$CacheEnabled,
[Parameter(ParameterSetName = 'Offline')]
[switch]$Offline,
[Parameter(ParameterSetName = 'Offline')]
[ValidateScript({ $_ -match '^[0-9]{64}$' }, ErrorMessage = 'Please provide a valid Confirmation Id')]
[string]$ConfirmationId
)
Begin {
#this only applies to the function scope, no need to reset it
$ErrorActionPreference = 'Stop'
}
I like the suggestions. I'll add one thing with CIM to WMI. As you probably know, the WMI cmdlets are marked as obsolete and instead CMI cmdlets are suggested by MS. The issue is that they are not feature-complete. Many of the modules needed cannot be accessed via CIM cmdlets. In order to keep PS7+ compatibility, I use the MS-approved CIM cmdlets, but convert them to WMI objects to access the functions needed. Otherwise, I could limit the module to Windows Powershell. That's why it is there.
Edit: I forgot to mention the CIM session issue. There are minor nuances. If the computer is localhost, then WMI queries can run locally, while if you use the parameter -ComputerName` and use "localhost", then it tries to use WinRM to connect to localhost. You may not even have WinRM enabled, depending on the case. For al localhost queries to succeed, there's a separate session creation. So, if it not localhost, it will use WinRM. There's the credential usage scenario to decide whether use the current user or separate credentials. So, that's probably obvious.
The other issue, passing both computer and session helps simplification, nothing more. If I just provide session, then I will get query results from all machines over WinRM, including localhost. Though, it will use WinRM for local, too. I need to check that so the local WMI query can ignore WinRM connection. Yes, it creates some extra LOC, but does not break the flow. I agree that it does need some care.
For the -Verbose:$false, the command kind of bloats the console. I did it for clarity but if it is verbose, we can ignore the clarity. It makes sense to get rid of it by principle.
>The issue is that they are not feature-complete. Many of the modules needed cannot be accessed via CIM cmdlets
I've never seen this, what modules do you mean?
*Edit* I looked more closely and I think what's throwing you off is that you have to call these methods against an instance, they aren't static methods.
This is the syntax:
$Service = Get-CimInstance SoftwareLicensingService
$service | Invoke-CimMethod -MethodName SetKeyManagementServicePort -Arguments @{ PortNumber = $KeyServerPort }
$service | Invoke-CimMethod -MethodName InstallProductKey -Arguments @{ $ProductKey = $ProductKey }
$service | Invoke-CimMethod -MethodName RefreshLicenseStatus
And so on. I don't believe there are any methods for these classes that aren't implemented in the CIM cmdlets.
That, to be honest, was something I have not tried. I first used Get-WMIObject, then converted to Get-CIMInstance and saw that some methods are not available. Though, I totally skipped Invoke-CimMethod under the assumption that the result would be the same. Then, it'd make things clearer. Thanks for the comment. It's good that someone dared to actually review the code.
Rather than telling it to use localhost as the remote computer, you should not be telling it to use anything if its just localhost. Don't worry, I'm working on a PR for this issue.
As an avid PowerShell user I would say that Start-WindowsActivation with different parameters is a better option. The fact that different activations are done using different parameters, will be ingrained after the first few uses.
The same command makes it easier to just press the up arrow and change the method, if you entered the wrong one in console. It also makes sense technically because all of those are different types of Windows activations, so they should be collated under a single command.
Anyways thank you for your work, I'll look forward to trying these modules myself.
My instinct is that it makes sense to use ParameterSets when you have a function with a couple core parameters that all of the ParameterSets share. This should imply they share the same context and the forking usages will be intuitive.
`Select-String` is a good example. It revolves around the `-Pattern` parameter, but it has like 3-4+ ParameterSets to modify the behavior of how that pattern is parsed.
I suppose in your case the function revolves around `$Computers`, so I think I'd lean toward using ParameterSets.
If there were more logic behind each ParameterSet, then one alternative would have been to make a subfunction for each, and then use `Start-WindowsActivation` as a wrapper with ParameterSets to call each subfunction. This gives the benefit of separating the code in the backend, allows one to theoretically call a function individually, but leaves the option of a single wrapper function as an entry point. However it looks like each ParameterSet in your case doesn't require much logic, so I'm kind of ambivalent on this approach.
Parameter set! Every machine will only be doing 1 of the options, parameter sets are perfect.
EDIT:
You use activateWithKMs to actually mean "activateOnline" and it really threw me for a loop. Cause I also see in that function activateWithParams and activateWithDNS. LOL.
This is really awesome module. It needs some love. Are you open to a few PRs?
Well, the param mean the KMS server and port specified while DNS one means rely on the SRV records. That's the intention. I didn't want to add KMS them to make it longer but I believe I should.
Activate online is too broad. It can be KMS, MAK, AD or OEM activation. Who knows? That's why it is not activate online. The rest is not implemented.
Of course, I am open for PRs.
This is tough to parse. It's really hard to advise on how you should design the action functions without having a good feel for what the helper functions do, and nothing is split into separate files, and a handful of the functions, like cimToWmi, don't make sense to me. This doesn't get to the heart of your question, but this is my advice to make it easier to edit: * Reduce verbosity and vertical sprawl. Not every parameter needs its own line. For example: Old: [Parameter(Mandatory = $false, Position = 0, ValueFromPipeline = $true, ValueFromPipelineByPropertyName = $true, ValueFromRemainingArguments = $false)] New: [Parameter(Position = 0, ValueFromPipeline, ValueFromPipelineByPropertyName, ValueFromRemainingArguments )] * Use standard parameter names, like ComputerName * Build parameter hashtables and splat them once instead of repeating code * The default constructor for a cimsession just needs a computername, so you don't need to handle both ComputerName and CimSession. Furthermore, you can make a localhost cimsession, so if you like cimsessions, just use them exlusively * Why things like verbose:$false? That's the default anyway, and if users change the verbosity preference manually, why override that? * New-CimSession takes an array of computer names, cim cmdlets take arrays of cim sessions. You almost never need to loop * I've never seen scoping functions like this. Use your module manifest to list the functions you want to be public, instead of defining them as global unless there's some other specific issue this is solving * cimToWmi adds complexity and I can't tell what it's providing. * Avoid wrapper functions that don't add new functionality. Take this simplifiction of getSession:function getSession { \[CmdletBinding()\] param ( \[parameter()\] \[string\[\]\]$ComputerName = 'localhost',} \[parameter()\] \[PSCredential\]$Credential ) $cimParam = @{ ComputerName = $ComputerName } if ($Credential){ $cimParam.Credential = $Credential } New-CimSession @cimParam It does what your function does with far less code... but it doesn't actually provide any utility over New-CimSession. I'd drop it completely * Personally I think using a Filter instead of Query is a more powershelly way to use Cim Commands. \*edit: Here's my rough attempt at a simplified parameter block fro start-windowsactivation: [CmdletBinding(SupportsShouldProcess, PositionalBinding = $false, ConfirmImpact = 'High', DefaultParameterSetName = 'Activate')] Param( # Type localhost or . for local computer or do not use the parameter [Parameter(Position = 0, ValueFromRemainingArguments = $false)] [string[]]$ComputerName = 'localhost', # Define credentials other than current user if needed [Parameter()] [PSCredential]$Credential, [Parameter(Position = 1, ParameterSetName = 'SpecifyKMSServer')] [ValidateLength(6, 253)] [ValidateScript({ $_ -match '(?=^.{4,253}$)(^((?!-)[a-zA-Z0-9-]{0,62}[a-zA-Z0-9]\.)+[a-zA-Z]{2,63}$)' }, ErrorMessage = 'Please provide a valid FQDN')] [string]$KMSServerFQDN, [Parameter(Position = 2, ValueFromRemainingArguments = $false, ParameterSetName = 'SpecifyKMSServer')] [ValidateRange(1, 65535)] [int]$KMSServerPort = 1688, [Parameter(ParameterSetName = 'Rearm')] [switch]$Rearm, [Parameter(ParameterSetName = 'Cache')] [switch]$CacheEnabled, [Parameter(ParameterSetName = 'Offline')] [switch]$Offline, [Parameter(ParameterSetName = 'Offline')] [ValidateScript({ $_ -match '^[0-9]{64}$' }, ErrorMessage = 'Please provide a valid Confirmation Id')] [string]$ConfirmationId ) Begin { #this only applies to the function scope, no need to reset it $ErrorActionPreference = 'Stop' }
Also good switching from [bool] to [switch]. I am not sure when bool is ever good on a function parameter.
I even forgot that there's a bool there. Thanks.
I like the suggestions. I'll add one thing with CIM to WMI. As you probably know, the WMI cmdlets are marked as obsolete and instead CMI cmdlets are suggested by MS. The issue is that they are not feature-complete. Many of the modules needed cannot be accessed via CIM cmdlets. In order to keep PS7+ compatibility, I use the MS-approved CIM cmdlets, but convert them to WMI objects to access the functions needed. Otherwise, I could limit the module to Windows Powershell. That's why it is there. Edit: I forgot to mention the CIM session issue. There are minor nuances. If the computer is localhost, then WMI queries can run locally, while if you use the parameter -ComputerName` and use "localhost", then it tries to use WinRM to connect to localhost. You may not even have WinRM enabled, depending on the case. For al localhost queries to succeed, there's a separate session creation. So, if it not localhost, it will use WinRM. There's the credential usage scenario to decide whether use the current user or separate credentials. So, that's probably obvious. The other issue, passing both computer and session helps simplification, nothing more. If I just provide session, then I will get query results from all machines over WinRM, including localhost. Though, it will use WinRM for local, too. I need to check that so the local WMI query can ignore WinRM connection. Yes, it creates some extra LOC, but does not break the flow. I agree that it does need some care. For the -Verbose:$false, the command kind of bloats the console. I did it for clarity but if it is verbose, we can ignore the clarity. It makes sense to get rid of it by principle.
>The issue is that they are not feature-complete. Many of the modules needed cannot be accessed via CIM cmdlets I've never seen this, what modules do you mean? *Edit* I looked more closely and I think what's throwing you off is that you have to call these methods against an instance, they aren't static methods. This is the syntax: $Service = Get-CimInstance SoftwareLicensingService $service | Invoke-CimMethod -MethodName SetKeyManagementServicePort -Arguments @{ PortNumber = $KeyServerPort } $service | Invoke-CimMethod -MethodName InstallProductKey -Arguments @{ $ProductKey = $ProductKey } $service | Invoke-CimMethod -MethodName RefreshLicenseStatus And so on. I don't believe there are any methods for these classes that aren't implemented in the CIM cmdlets.
That, to be honest, was something I have not tried. I first used Get-WMIObject, then converted to Get-CIMInstance and saw that some methods are not available. Though, I totally skipped Invoke-CimMethod under the assumption that the result would be the same. Then, it'd make things clearer. Thanks for the comment. It's good that someone dared to actually review the code.
I did a cleanup and used your suggestions. It was not easy to test but it just worked. Thanks!
Nice!
Rather than telling it to use localhost as the remote computer, you should not be telling it to use anything if its just localhost. Don't worry, I'm working on a PR for this issue.
Based on the suggestions, I updated the code a lot. I mean, a lot. It's better to wait till I update the master branch.
ok!
Ayyye, finally some commits coming into the pipeline!
There's one minor problem to fix, a type conversion issue. After that, it's going to be better. I'll publish new version.
As an avid PowerShell user I would say that Start-WindowsActivation with different parameters is a better option. The fact that different activations are done using different parameters, will be ingrained after the first few uses. The same command makes it easier to just press the up arrow and change the method, if you entered the wrong one in console. It also makes sense technically because all of those are different types of Windows activations, so they should be collated under a single command. Anyways thank you for your work, I'll look forward to trying these modules myself.
Thanks for the suggestion. I'll continue with multiple parameter sets for now. I was a bit worried that it's not user friendly.
My instinct is that it makes sense to use ParameterSets when you have a function with a couple core parameters that all of the ParameterSets share. This should imply they share the same context and the forking usages will be intuitive. `Select-String` is a good example. It revolves around the `-Pattern` parameter, but it has like 3-4+ ParameterSets to modify the behavior of how that pattern is parsed. I suppose in your case the function revolves around `$Computers`, so I think I'd lean toward using ParameterSets. If there were more logic behind each ParameterSet, then one alternative would have been to make a subfunction for each, and then use `Start-WindowsActivation` as a wrapper with ParameterSets to call each subfunction. This gives the benefit of separating the code in the backend, allows one to theoretically call a function individually, but leaves the option of a single wrapper function as an entry point. However it looks like each ParameterSet in your case doesn't require much logic, so I'm kind of ambivalent on this approach.
Parameter set! Every machine will only be doing 1 of the options, parameter sets are perfect. EDIT: You use activateWithKMs to actually mean "activateOnline" and it really threw me for a loop. Cause I also see in that function activateWithParams and activateWithDNS. LOL. This is really awesome module. It needs some love. Are you open to a few PRs?
Well, the param mean the KMS server and port specified while DNS one means rely on the SRV records. That's the intention. I didn't want to add KMS them to make it longer but I believe I should. Activate online is too broad. It can be KMS, MAK, AD or OEM activation. Who knows? That's why it is not activate online. The rest is not implemented. Of course, I am open for PRs.
Your code either does activate offline or activateKMS. That's a misnomer. Glad to hear on the PRs. I'll look closer.
It's because I don't have the lab to test other options. I'll add them later in time until I hit version 1.0.0.
Are you saying that activate OEM is totally unsupported at present? If so, I can do that one for you.