The script Upgrade-PowerShell.ps1 - remarks
it-praktyk opened this issue · 1 comments
I read your script and I would like to propose some improvements.
-
Currently, credentials are provided as strings - the best practice is to use [System.Management.Automation.PSCredential] type - detailed explanation How to Add Credential Parameters to PowerShell Functions. Changing the list of parameters is a broking change so the newer version should be published as 2.0 with an error message that parameters username and password are not valid anymore.
-
The parameter
verbose
is declared as a switch type you don't have to assign the$false
value to it. It has$false
value by default. Additionally, usage ofverbose
should be checked by$verbose.IsPresent
-
References in the file header
.LICENSEURI https://github.com/jborean93/ansible-windows-scripts/blob/master/LICENSE
.PROJECTURI https://github.com/jborean93/ansible-windows-scripts
are not actual - you probably changed the GitHub project name. Yes, GitHub redirects to the current URL but ... ;-)
If you will pre-agree with my remarks I can prepare a pull request to address them.
Happy for 2 and 3, I was originally tossing up with making the -password
parameter a secure string but in the end I didn't do it for 1 reason. This reason was because the password is stored as plain text in the registry and I didn't want to give people the illusion that the string stays secure. As a middle ground I'm happy to have a new parameter added called -credential
that takes in a PSCredential
object and that overrides -username
and -password
or raise an error if both are specified. For users who want to use the credential type they are able to but it also preserves backwards compatibility for others who just download the script from Github.
If you feel up to changing the documentation for the script you may also want to look at #2. I haven't gotten round to changing it but it just means all docs need to be commented line by line with #
instead of <#
.