Various fixes, optimizations and code formatting #13

mr-kobalt · 2019-02-03T19:20:21Z

Hello,

please see commit messages for details. Also, this is my first pull-request in public repository, so I'll appreciate any critique and suggestions about contributing workflow in general and your repo in particular.

jborean93 · 2019-02-07T23:08:32Z

Thanks for taking your time to work on this I see you've put in quite a bit of work. I'm not sure if I would accept this PR as it is because it makes a lot of stylistic changes when it isn't needed. While I understand some of the changes you've made conform more to the standard set in PowerShell these style guides are never set in stone. Always be mindful of the choices the original developer has made when creating a pull request.

There still seems to be a good few improvements like | Out-Null to > $null and the temp path handling change. Feel free to modify or open a new PR with those changes in there. Some of the things I think you should avoid changing in another person's PR are;

Variable casing, I typically follow snake_case compared to camelCase or PascalCase. IMO neither one is right or wrong, the key is to stick with one standard
Quotes from " to ', technically " can be more dangerous when it comes to escaping variables but I see no real difference to change existing quotes to ' when " has worked just fine
Function parameters from inline to param () blocks are definitely more PowerShell like but for internal commands it really isn't that big of a deal and just make unecessary noise in the PR diff
Changing the $null comparisons to -not $var may work but I like to be explicit. Although I did make the mistake of putting $null on the right hand side of the operator

Feel free to disagree with my points here, I did make this script when I first started with PowerShell so it was more aligned with how I worked in other languages.

In saying all this, the beauty of open source is that it allows you to fork the code and make your own changes and do whatever you want with it. There's nothing stopping you from making whatever changes you desire and adding features that are specific for yourself.

mr-kobalt · 2019-02-08T17:37:39Z

Thank you for your feedback, I really appreciate it.

Definitely code style changes should be discussed thoroughly beforehand, so I'll rework this pull request. I still have some questions, though:

Mostly you wrote about code style and that is related to 2a070a8 and, partially, to 40258aa, but what about other commits, which make more functional changes in script's logic? Should I include them into the next pull request?
Is it better in that case for me to rebase commits (delete the unwanted and amend in code style of the desired), or revert unwanted commits to preserve history?

jborean93 · 2019-02-10T23:12:20Z

Mostly you wrote about code style and that is related to 2a070a8 and, partially, to 40258aa, but what about other commits, which make more functional changes in script's logic? Should I include them into the next pull request?

I would like to add the functional changes you've made as they seem like good additions but would like to separate those changes from the stylistic changes. Once we have the functional stuff in then I probably could entertain some of the stylistic changes but that's a better discussion for a separate PR.

Is it better in that case for me to rebase commits (delete the unwanted and amend in code style of the desired), or revert unwanted commits to preserve history?

Really entirely up to you. When I go to merge the PR I would probably just squash and merge unless keeping the history is valuable.

Fixes: - Fix bug in `Reboot-AndResume`: if spaces were contained in `$script_path`, then script wouldn't restart after rebooting. - Fix error throwing in `Reboot-Resume`: it's not an error that user may not want to reboot immediately, so we should exit normally without throwing an error. - Fix comparison against `$null` [1] Features: - Suppress all interactions with user by adding new parameter `-Force` and restarting computer automatically - User now could specify domain by using new `-Domain` parameter Changes: - Refactor tests for whether or not to install .NET. If `Get-ItemProperty` couldn't find specified item, then `$dotnet_version` will be `$null`, and it's property `Release` as well. `$null` casts to `0` in comparisons against integers, so expression `$dotnet_version.Release -lt 379893` always resulted in `$true` - Use `$PSHOME` instead of hardcoded path - Replace `| Out-Null` with `> $null` for consistency - Use `[uri]` type for download URLs for more elegant and robust way for filename extraction from them. - Switch to `Join-Path` for paths concatenation. - Generalize behavior of enabling and disabling AutoLogon feature with a new `Set-AutoLogon` function which expects one of the two parameters: - `-Enable` for enabling auto logon with specified `$sctipt:username`, `$sctipt:domain`, and `$sctipt:password` - `-Disable` for disabling auto logon `Set-AutoLogon` DOES NOT check if credentials ($Username, etc.) were provided to script at all, as it's a caller responsibility. - Refactor `Download-Wmf5Server2008` logic: `Download-Wmf5Server2008` is only called on systems with WFM3 and .NET 4.5.2 already installed, so it's safe to presume that we don't need a fallback to legacy `Shell.Application` unzipping functionality, especially as it is full of pitfalls like asynchronous nature [2]. [1] https://github.com/PowerShell/DscResources/blob/master/StyleGuidelines.md#ensure-null-is-on-left-side-of-comparisons [2] https://social.technet.microsoft.com/Forums/en-US/6988d856-09ae-41c5-aa79-3d78a9e4d03a/powershell-use-shellapplication-to-zip-files?forum=ITCG

mr-kobalt · 2019-02-18T03:25:40Z

ОК, it has taken some time, but finally I made it. Definitely next time I'll discuss stylistic changes beforehand , rather than editing commits and resolving conflicts. Would be a lesson, though.
But I was surprised that git squashed conflicted commits without asking after I resolved them while rebasing interactively. I hope it wouldn't be a problem.

`-Force` parameter was ignored if `-Username` and `-Password` params have been set. Also comparison to `$null` changed to comparison with `$false`, because `[switch]` param cannot be `$null`.

jborean93

Looks great, just a few minor questions/comments I've added inline.

jborean93 · 2019-02-21T10:10:06Z

scripts/Upgrade-PowerShell.ps1

-    Remove-ItemProperty -Path $reg_winlogon_path -Name DefaultPassword -ErrorAction SilentlyContinue
+Function Set-AutoLogon {
+    param (
+        [switch] $Enable,


I would probably just have the 1 switch, if set then enable AutoLogon, if not then remove AutoLogon. That way you don't need the ambiguous if case and Enable/Disable are opposites of each other.

In that case, maybe a better way would be to make two separate functions - Enable-AutoLogon and Disable-AutoLogon?

jborean93 · 2019-02-21T10:10:57Z

scripts/Upgrade-PowerShell.ps1

 Param(
    [string]$version = "5.1",
    [string]$username,
+    [string]$domain,


Can you not specify the domain as part of the $username, like DOMAIN\user or user@DOMAIN.COM?

Actually I don't know and haven't got domain controller at my disposal to check it. Nevertheless in my opinion separate parameter for domain name is more clear and less obscure way for users to login using their domain accounts.

jborean93 · 2019-02-21T10:13:32Z

scripts/Upgrade-PowerShell.ps1


    Write-Log -message "extracting '$zip_file' to '$tmp_dir'"
-    try {
-        Add-Type -AssemblyName System.IO.Compression.FileSystem > $null


Please keep this in here, Server 2008/R2 probably won't include .NET 4.5 which is when System.IO.Compression.ZipFile was added.

You are correct that .NET 4.5 isn't available in clean Server 2008 R2, and you definitely found a bug in my code, but my intentions was to get rid of the CopyHere() method from Shell.Application object as it is unreliable and unpredictable.
We could achieve that by loading assembly in a slightly different way and also telling PowerShell to use latest installed NET Framework - it will work because first step in upgrading PS to version 3.0 and highter is to install .NET 4.5.2. You can find more information in that SO answer: https://stackoverflow.com/a/37815418/10504393

So, after .NET installation we could add DWORD that forces usage of the latest CRL, respawn script and load assembly like this:

[Reflection.Assembly]::LoadWithPartialName("System.IO.Compression.Filesystem")

Also, this could allow us to get rid of Download-Wmf5Server2008 and rewrite script in more consistent way, that all steps logic will be determined in foreach ($action in $actions) loop.

What do you think?

sergeevabc · 2024-02-10T17:16:31Z

Dear @jborean93, are you there? Let's finish improving your script by accepting the suggested changes. Please take a moment to do this.

mr-kobalt force-pushed the master branch from 85450ae to 5490026 Compare February 18, 2019 02:59

mr-kobalt force-pushed the master branch from 5490026 to 6c9e4b3 Compare February 18, 2019 03:11

Fix bug in -Force param processing in Reboot-AndResume function

e036a4d

`-Force` parameter was ignored if `-Username` and `-Password` params have been set. Also comparison to `$null` changed to comparison with `$false`, because `[switch]` param cannot be `$null`.

mr-kobalt force-pushed the master branch from 0e6a3f8 to e036a4d Compare February 18, 2019 16:43

jborean93 reviewed Feb 21, 2019

View reviewed changes

Various fixes, optimizations and code formatting #13

Are you sure you want to change the base?

Various fixes, optimizations and code formatting #13

Uh oh!

Conversation

mr-kobalt commented Feb 3, 2019

Uh oh!

jborean93 commented Feb 7, 2019

Uh oh!

mr-kobalt commented Feb 8, 2019

Uh oh!

jborean93 commented Feb 10, 2019

Uh oh!

mr-kobalt commented Feb 18, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jborean93 left a comment

Choose a reason for hiding this comment

Uh oh!

jborean93 Feb 21, 2019

Choose a reason for hiding this comment

Uh oh!

mr-kobalt Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

jborean93 Feb 21, 2019

Choose a reason for hiding this comment

Uh oh!

mr-kobalt Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

jborean93 Feb 21, 2019

Choose a reason for hiding this comment

Uh oh!

mr-kobalt Mar 5, 2019

Choose a reason for hiding this comment

Uh oh!

sergeevabc commented Feb 10, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mr-kobalt commented Feb 18, 2019 •

edited

Loading