Make sudo.ps1 support multi-line commands#39
Conversation
Don't assume Length=5 (people may call sudo.ps1 or the full path) And, you know, clean it up a bit, because I can't stop myself.
scripts/sudo.ps1
Outdated
| } | ||
|
|
||
| if ([Environment]::OSVersion.Platform -ne "Win32NT") { | ||
| if ($IsLinux -or $IsMacOS) { |
There was a problem hiding this comment.
$IsLinux and $IsMacOS are only available since Powershell 7 / Core 6. The previous approach was more portable.
There was a problem hiding this comment.
The only place where they are not available is Windows PowerShell -- thus, when they are missing, they are false, so the logic works without involving classes or string comparisons.
That is to say: in PowerShell, when a variable doesn't exist, it returns $null and in a boolean context, it's $false
As a bonus, variables can be faked for testing, where that static property cannot. 😉
| if ($null -eq (Get-Command -Type Application -Name "$SUDOEXE" -ErrorAction Ignore)) { | ||
| throw "'$SUDOEXE' cannot be found." | ||
| if (!(Get-Command -Type Application -Name $Env:SUDOEXE -ErrorAction Ignore)) { | ||
| throw "Env:SUDOEXE is set to '$Env:SUDOEXE' but it cannot be found." |
There was a problem hiding this comment.
| throw "Env:SUDOEXE is set to '$Env:SUDOEXE' but it cannot be found." | |
| throw "Env:SUDOEXE is set to '$Env:SUDOEXE' but cannot be found." |
There was a problem hiding this comment.
maybe "that cannot be found." or keep "it"
There was a problem hiding this comment.
Yeah, I think this:
"Env:SUDOEXE is set to 'C:\Windows\Sudo.exe' but it cannot be found"
Is better than:
"Env:SUDOEXE is set to 'C:\Windows\Sudo.exe' but cannot be found"
| if ($null -eq (Get-Command -Type Application -Name "$SUDOEXE" -ErrorAction Ignore)) { | ||
| throw "'$SUDOEXE' cannot be found." | ||
| if ($__SUDO_TEST -ne $true) { | ||
| $Env:SUDOEXE = "sudo.exe" |
There was a problem hiding this comment.
This seems overly complicated. One must set two variables: SUDOEXE and __SUDO_TEST, to actually be able to use an alternate executable for testing? I'm not sure why this was here...
I would prefer to just have a parameter SudoPath that you can set to whatever you want for testing purposes, but that defaults to just sudo.exe and assumes that sudo.exe is in the PATH.
NOTE: I changed SUDOEXE to an environment variable. I feel like if we're using a mock sudo, you need to be able to set the environment stuff once (i.e. in your pipeline, or in your machine scope), instead of needing to set multiple flags for different wrapper scripts.
I'd rather remove these, but if we want to keep them, should __SUDO_TEST also be an environment variable?
| $thisPowerShell = $psProcess.MainModule.FileName | ||
| if ($null -eq $thisPowerShell) { | ||
| throw "Cannot determine path to '$psProcess'" | ||
| $thisPowerShell = (Get-Process -Id $PID).MainModule.FileName |
There was a problem hiding this comment.
I want to point out that I dramatically simplified this, just because there isn't really any way this can go wrong. $PID is a constant in PowerShell and can't changed, so this is always going to return the full path to the executable.
trackd
left a comment
trackd
left a comment
There was a problem hiding this comment.
was testing a bit and found some minor stuff.
| $Statement = [System.Management.Automation.InvocationInfo].GetMember( | ||
| '_scriptPosition', | ||
| [System.Reflection.BindingFlags]'NonPublic,Instance' | ||
| )[0].GetValue($MyInvocation).Text. |
There was a problem hiding this comment.
the trailing . after Text needs to be removed
| if ($PSCmdlet.ParameterSetName -eq "Command") { | ||
| if (@(Get-Command $Command -ErrorAction Ignore)[0].CommandType -eq "Application") { | ||
| # NOTE: this assumes that all the parameters can be just strings | ||
| if ($PSBoundParameters.Contains("Debug")) { |
| } | ||
| else { | ||
| throw "Cannot find '$commandName'" | ||
| if ($PSBoundParameters.Contains("Debug")) { |
4 participants
Also, don't assume Length=5 (people may invoke it as sudo.ps1 or as the full path to the script)
I also cleaned up a lot, sorry.
And added a settings file for vscode to specify the formatting.
I think we should have tests for this to guarantee the different use cases work (and ensure they keep working when people make changes in the future). Is there a build I can add tests to?