jborean93/ansible-windows

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 of verbose 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 <#.