skip console messages if the host is non-interactive #13

lisaong · 2017-01-25T03:12:05Z

Issue #12

First try the -NonInteractive parameter check (which should catch most cases). If no parameters are supplied, check for the 'Default Host' name.

The latter check is needed when running the scripts within the EdgePS hosting environment. Unfortunately, no parameters are supplied in the binary version that we are consuming.

Resolves #12: Console messages should be suppressed in Write-Log for non-interactive consoles

msftclas · 2017-01-25T03:12:09Z

Hi @lisaong, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!

It looks like you're a Microsoft contributor (Lisa Ong). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

HowardWolosky · 2017-01-25T20:46:42Z

StoreBroker/Helpers.ps1

@@ -383,23 +401,27 @@ function Write-Log
            $env:username,
            $Message

-        switch ($Level)
+        # Output to the console if it is interactive
+        if ($global:SBInInteractiveConsole)


It's not appropriate to filter this out here. You should have this check only around the Write-Host. People can modify the execution of a non-interactive PowerShell console to still redirect / pipe Error, Warning Verbose, Debug and Informational output to somewhere else. It's the Write-Host specifically that's causing you issues, and so that's the only one that should be protected by this check.

Could solve this with Write-InteractiveHost, mentioned above

Done. Thanks for the feedback!

HowardWolosky · 2017-01-25T20:47:33Z

StoreBroker/Helpers.ps1

-                $output += "This is non-fatal, and your command will continue.  Your log file will be missing this entry:"
-                $output += $ConsoleMessage
-                Write-Warning ($output -join [Environment]::NewLine)
+                if ($global:SBInInteractiveConsole)


Similarly here, Write-Warning shouldn't be causing you issues. If it is, we should talk further.

Removed. Thanks!

HowardWolosky · 2017-01-25T20:52:01Z

StoreBroker/Helpers.ps1

@@ -71,6 +71,14 @@ function Initialize-HelpersGlobalVariables
    {
        $global:SBNotifyCredential = [PSCredential]$null
    }
+
+    # Initialize and cache whether this console is interactive
+    $global:SBInInteractiveConsole = ![Bool]([Environment]::GetCommandLineArgs() -like '-noni*')


The only time we use global variables is when we're looking to allow for a user configuration. This variable is not intended for users to set their own value for (its value is inferred by the environment), and therefore it shouldn't be global. $script: would be the appropriate scope for this one, and it therefore shouldn't be in this method. Just move this block of code to be below the call to Initialize-HelpersGlobalVariables

HowardWolosky · 2017-01-25T21:05:44Z

StoreBroker/Helpers.ps1

+    if ($global:SBInInteractiveConsole)
+    {
+        # Some hosts like "Default Host" don't take command line arguments, but default to non-interactive
+        $global:SBInInteractiveConsole = (Get-Host).Name -ne 'Default Host'


I'm curious if you can avoid this check if you do a check for [Environment]::UserInteractive and -and that with your other check. I was looking at StackOverflow and that appeared to be the consensus for this issue.

Unfortunately, [Environment]::UserInteractive returned true in the edge-ps environment, so I had to keep the check.

danbelcher-MSFT · 2017-01-26T01:11:12Z

StoreBroker/Helpers.ps1

+        if ($global:SBInInteractiveConsole)
+        {
+            Write-Host "`r$($animationFrames[$($iteration % $($animationFrames.Length))])  Elapsed: $([int]($iteration / $framesPerSecond)) second(s) $Description" -NoNewline -f Yellow
+        }


Seems like we could have a Write-InteractiveHost function that takes care of checking if we are in an interactive host and noops if we aren't. Then you just need to change the function names here.

That's a great suggestion. +1

Done. I ended up generating a proxy function, ripping out the guts (because it's more complicated than our purposes), and then forwarding the call to Write-Host appropriately.

In case you are curious, the proxy function is this complex because it allows you to augment parameters. I pasted the original below.

https://blogs.technet.microsoft.com/heyscriptingguy/2011/03/01/proxy-functions-spice-up-your-powershell-core-cmdlets/

[CmdletBinding(HelpUri='http://go.microsoft.com/fwlink/?LinkID=113426', RemotingCapability='None')] param( [Parameter(Position=0, ValueFromPipeline=$true, ValueFromRemainingArguments=$true)] [System.Object] ${Object}, [switch] ${NoNewline}, [System.Object] ${Separator}, [System.ConsoleColor] ${ForegroundColor}, [System.ConsoleColor] ${BackgroundColor}) begin { try { $outBuffer = $null if ($PSBoundParameters.TryGetValue('OutBuffer', [ref]$outBuffer)) { $PSBoundParameters['OutBuffer'] = 1 } $wrappedCmd = $ExecutionContext.InvokeCommand.GetCommand('Microsoft.PowerShell.Utility\Write-Host', [System.Management.Automation.CommandTypes]::Cmdlet) $scriptCmd = {& $wrappedCmd @PSBoundParameters } $steppablePipeline = $scriptCmd.GetSteppablePipeline($myInvocation.CommandOrigin) $steppablePipeline.Begin($PSCmdlet) } catch { throw } } process { try { $steppablePipeline.Process($_) } catch { throw } } end { try { $steppablePipeline.End() } catch { throw } } <# .ForwardHelpTargetName Microsoft.PowerShell.Utility\Write-Host .ForwardHelpCategory Cmdlet #>

}

lisaong · 2017-01-31T02:37:52Z

StoreBroker/Helpers.ps1

@@ -105,7 +105,6 @@ function Wait-JobWithAnimation
        set of configuration options that Wait-Job does.
 #>
    [CmdletBinding()]
-    [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidUsingWriteHost", "", Justification="This function is intended for human interaction, not for scripting.  Write-Host makes the most sense for visible feedback.")]


re-ran Invoke-ScriptAnalyzer -Path .\ -Recurse, and no new hits for Helpers.ps1 due to the changes.

danbelcher-MSFT · 2017-01-31T19:04:02Z

StoreBroker/Helpers.ps1

+    [CmdletBinding(HelpUri='http://go.microsoft.com/fwlink/?LinkID=113426', RemotingCapability='None')]
+    [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidUsingWriteHost", "", Justification="This provides a wrapper around Write-Host. In general, we'd like to use Write-Information, but it's not supported on PS 4.0 which we need to support.")]
+    param(


nit picky stuff, this should be reformatted to be consistent with the file

closing paren ')' for the param block should be on its own line and aligned under the 'p' of 'param'

variables should be on the same line as their type, separated by a space

variables don't need the curlys, just the $ and their name

each characteristic in the Parameter attribute should be on its own line, see ConvertTo-Array in this file
- just specify the characteristic, no need to add the $true as it is implicit. For example, it's enough to say 'ValueFromPipeline'. See ConvertTo-Array in this file

Some things to still fix on this topic:

Still don't need the brackets for these parameter names. Instead of ${NoNewLine}, just do $NoNewLine.

Please have a new-line between params for consistency with the other functions

make cmdletbinding multi-line like we do with all the other functions with multiple inputs to it.

capitalize CmdletBinding the same way as we do in the other functions.

Not sure what you meant by "capitalize CmdletBinding the same way as we do in the other functions." Do you have an example?

danbelcher-MSFT · 2017-01-31T19:05:58Z

StoreBroker/Helpers.ps1

+            $PSBoundParameters['OutBuffer'] = 1
+        }
+        Write-Host @PSBoundParameters


nit: add a newline after the if-block before the next statement.

HowardWolosky · 2017-01-31T22:22:06Z

StoreBroker/Helpers.ps1

+        non-interactive hosts.
+
+        Boilerplate is generated using these commands:


This "boilerplate" info is interesting, but better suited for the .NOTES section of the comment-based-help since it doesn't help users who are trying to use the method.

HowardWolosky · 2017-01-31T22:45:22Z

StoreBroker/Helpers.ps1

+    [CmdletBinding(HelpUri='http://go.microsoft.com/fwlink/?LinkID=113426', RemotingCapability='None')]
+    [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidUsingWriteHost", "", Justification="This provides a wrapper around Write-Host. In general, we'd like to use Write-Information, but it's not supported on PS 4.0 which we need to support.")]
+    param(


Some things to still fix on this topic:

Still don't need the brackets for these parameter names. Instead of ${NoNewLine}, just do $NoNewLine.

Please have a new-line between params for consistency with the other functions

make cmdletbinding multi-line like we do with all the other functions with multiple inputs to it.

capitalize CmdletBinding the same way as we do in the other functions.

HowardWolosky · 2017-01-31T22:46:27Z

StoreBroker/Helpers.ps1

+
+    # Determine if the host is interactive
+    if ([Environment]::UserInteractive -and ![Bool]([Environment]::GetCommandLineArgs() -like '-noni*') -and (Get-Host).Name -ne 'Default Host')


Recommend splitting this into three lines since this line exceeds 100 chars.

HowardWolosky · 2017-02-01T00:09:46Z

StoreBroker/Helpers.ps1

+
+    [CmdletBinding(HelpUri='http://go.microsoft.com/fwlink/?LinkID=113426', RemotingCapability='None')]
+    [Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidUsingWriteHost", "", Justification="This provides a wrapper around Write-Host. In general, we'd like to use Write-Information, but it's not supported on PS 4.0 which we need to support.")]


You should be able to remove the other two PSAvoidUsingWriteHost SuppressMessageAttributes now...one is on Wait-JobWithAnimation and the other is on Write-Log.

they were already removed, I think you may be looking at an older version.

HowardWolosky · 2017-02-01T00:15:50Z

StoreBroker/Helpers.ps1

+
+    # Determine if the host is interactive
+    if ([Environment]::UserInteractive -and ![Bool]([Environment]::GetCommandLineArgs() -like '-noni*') -and (Get-Host).Name -ne 'Default Host')


In a StackOverflow discussion I saw on this topic, some users were talking about the performance impact of some of these tests. I'm curious if you notice any change in perceived behavior when running the command in an interactive console window...

I haven't noticed any difference so far. It could be because the commands themselves take a while to run (e.g. showing progress). If there is really a perf impact, we can consider making a follow-up change to cache the state.

HowardWolosky · 2017-02-01T00:16:23Z

StoreBroker/Helpers.ps1

@@ -442,21 +440,21 @@ function Write-Log

        switch ($Level)
        {
-            'Error'   { Write-Error $ConsoleMessage }


Thanks for catching these...

HowardWolosky · 2017-02-01T00:18:18Z

It would also be helpful for you could bump the patch part of the version as well as part of your change.

HowardWolosky · 2017-02-01T23:20:09Z

StoreBroker/Helpers.ps1

+            ValueFromPipeline=$true,
+            ValueFromRemainingArguments=$true)]
+


nit: No linefeed between the [Parameter] attribute and the parameter that it's associated with,

HowardWolosky · 2017-02-01T23:20:57Z

StoreBroker/Helpers.ps1

+        [Parameter(
+            Position=0,
+            ValueFromPipeline=$true,


nit: no need to specify $true for the parameter configurations. When you reference them by name, they default to $true.

HowardWolosky

Two minor nits. Once done, please squash your changes into a single commit, and then i'll rebase it into master.

@danbelcher-MSFT

consistent casing for $consoleMessage Addressed feedback from @HowardWolosky-MSFT and @danbelcher-MSFT by wrapping Write-Host calls with Write-InteractiveHost, so that calls to Write-Host are only forwarded if the host is interactive. Style feedback from @danbelcher-MSFT Addressed more style feedback from @HowardWolosky-MSFT, bumped patch version Addressed last remaining feedback from @HowardWolosky-MSFT

msftclas added the cla-not-required label Jan 25, 2017

HowardWolosky requested changes Jan 25, 2017

View reviewed changes

danbelcher-MSFT reviewed Jan 26, 2017

View reviewed changes

lisaong commented Jan 31, 2017

View reviewed changes

danbelcher-MSFT suggested changes Jan 31, 2017

View reviewed changes

HowardWolosky requested changes Feb 1, 2017

View reviewed changes

HowardWolosky reviewed Feb 1, 2017

View reviewed changes

danbelcher-MSFT approved these changes Feb 1, 2017

View reviewed changes

HowardWolosky reviewed Feb 1, 2017

View reviewed changes

HowardWolosky approved these changes Feb 1, 2017

View reviewed changes

HowardWolosky merged commit ef9b01c into microsoft:master Feb 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

skip console messages if the host is non-interactive #13

skip console messages if the host is non-interactive #13

lisaong commented Jan 25, 2017 •

edited by HowardWolosky

Loading

msftclas commented Jan 25, 2017

HowardWolosky Jan 25, 2017

danbelcher-MSFT Jan 26, 2017

lisaong Jan 31, 2017

HowardWolosky Jan 25, 2017

lisaong Jan 31, 2017

HowardWolosky Jan 25, 2017

HowardWolosky Jan 25, 2017

lisaong Jan 31, 2017

danbelcher-MSFT Jan 26, 2017

HowardWolosky Jan 26, 2017

lisaong Jan 31, 2017

lisaong Jan 31, 2017 •

edited

Loading

danbelcher-MSFT Jan 31, 2017

lisaong Jan 31, 2017

HowardWolosky Jan 31, 2017

lisaong Feb 1, 2017

danbelcher-MSFT Jan 31, 2017

HowardWolosky Jan 31, 2017

lisaong Feb 1, 2017

HowardWolosky Jan 31, 2017

HowardWolosky Jan 31, 2017

lisaong Feb 1, 2017

HowardWolosky Feb 1, 2017

lisaong Feb 1, 2017

HowardWolosky Feb 1, 2017

lisaong Feb 1, 2017

HowardWolosky Feb 1, 2017

HowardWolosky commented Feb 1, 2017

HowardWolosky Feb 1, 2017

HowardWolosky Feb 1, 2017

HowardWolosky left a comment

		non-interactive hosts.

		Boilerplate is generated using these commands:


		# Determine if the host is interactive
		if ([Environment]::UserInteractive -and ![Bool]([Environment]::GetCommandLineArgs() -like '-noni*') -and (Get-Host).Name -ne 'Default Host')


		[CmdletBinding(HelpUri='http://go.microsoft.com/fwlink/?LinkID=113426', RemotingCapability='None')]
		[Diagnostics.CodeAnalysis.SuppressMessageAttribute("PSAvoidUsingWriteHost", "", Justification="This provides a wrapper around Write-Host. In general, we'd like to use Write-Information, but it's not supported on PS 4.0 which we need to support.")]

skip console messages if the host is non-interactive #13

skip console messages if the host is non-interactive #13

Conversation

lisaong commented Jan 25, 2017 • edited by HowardWolosky Loading

msftclas commented Jan 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lisaong Jan 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HowardWolosky commented Feb 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HowardWolosky left a comment

Choose a reason for hiding this comment

lisaong commented Jan 25, 2017 •

edited by HowardWolosky

Loading

lisaong Jan 31, 2017 •

edited

Loading