Return exit code for local Windows command #533

james-stocks · 2019-11-08T10:56:26Z

Signed-off-by: James Stocks jstocks@chef.io

Description

The exit code might not be 0 for a script that does not throw a PowerShell exception. We should get the actual exit code and return it.
Note that PowerShell has a $? which is boolean as well as a numeric last exit code. The variable name train is using exitstatus is ambiguous on which it represents, maybe.

Related Issue

Partly addresses #288 but that issue also requires the stderr to be provided.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New content (non-breaking change)
[X ] Breaking change (a content change which would break existing functionality or processes)

If anyone is currently depending on the inaccurate 0 and 1 exit codes then they will get different behaviour when train starts returning the real exit code.

Checklist:

[X ] I have read the CONTRIBUTING document.

Partly addresses inspec#288 but that issue also requires the stderr to be provided. Signed-off-by: James Stocks <jstocks@chef.io>

chef-expeditor · 2019-11-08T10:56:35Z

Hello james-stocks! Thanks for the pull request!

Here is what will happen next:

Your PR will be reviewed by the maintainers.
Possible Outcomes
a. If everything looks good, one of them will approve it, and your PR will be merged.
b. The maintainer may request follow-on work (e.g. code fix, linting, etc). We would encourage you to address this work in 2-3 business days to keep the conversation going and to get your contribution in sooner.
c. Cases exist where a PR is neither aligned to Chef InSpec's product roadmap, or something the team can own or maintain long-term. In these cases, the maintainer will provide justification and close out the PR.

Thank you for contributing!

skpaterson

Thanks @james-stocks 👍

james-stocks · 2019-11-08T11:03:55Z

lib/train/transports/local.rb

@@ -199,10 +199,12 @@ def start_pipe_server(pipe_name)
              $scriptBlock = $ExecutionContext.InvokeCommand.NewScriptBlock($command)
              try {
                $stdout = & $scriptBlock | Out-String
-                $result = @{ 'stdout' = $stdout ; 'stderr' = ''; 'exitstatus' = 0 }
+                $exit_code = $LastExitCode
+                $result = @{ 'stdout' = $stdout ; 'stderr' = ''; 'exitstatus' = $exit_code }
              } catch {


I only need the exit code but at some point we should fix #288 fully and get stderr for passing scripts and stdout for failing scripts.
This whole section should maybe change because the catch block only applies to PowerShell scripts that throw. The catch block isn't entered for PowerShell scripts that "fail" by calling exit n

For the change that's given here, this seems fine - you're not making the lack of STDOUT/STDERR worse.

This looks reasonable as long as you are looking for the last exit code from the most recent Win32 process, and not the last PowerShell command was successful - use $? for that

I was taking the previous code's use of 0 to assume that it meant exit code and not exit status (in spite of the variable name)

miah

This change looks fine, but could we also get a test?

miah

This change looks fine, but could we also get a test?

clintoncwolfe

I am not a powershell expert - I've requested someone with more PowerShell than I to review to look it over.

I'd like a test too - see test/windows/local_test.rb

clintoncwolfe · 2019-11-14T17:51:30Z

lib/train/transports/local.rb

@@ -199,10 +199,12 @@ def start_pipe_server(pipe_name)
              $scriptBlock = $ExecutionContext.InvokeCommand.NewScriptBlock($command)
              try {
                $stdout = & $scriptBlock | Out-String
-                $result = @{ 'stdout' = $stdout ; 'stderr' = ''; 'exitstatus' = 0 }
+                $exit_code = $LastExitCode
+                $result = @{ 'stdout' = $stdout ; 'stderr' = ''; 'exitstatus' = $exit_code }
              } catch {


For the change that's given here, this seems fine - you're not making the lack of STDOUT/STDERR worse.

james-stocks · 2019-11-15T11:26:58Z

@miah @clintoncwolfe I added a test that mimics my own need (running a PS1 script that may or may not work).
There seems to be a bug where train has a problem with directly exiting a local Windows command.

e.g. if I attempt this test:

  it "run command with exit code" do
    cmd = conn.run_command('Write-Output "L8r allig8r"; Exit 42')
    _(cmd.stdout).must_equal "L8r allig8r\r\n"
    _(cmd.stderr).must_equal ""
    _(cmd.exit_status).must_equal 42
  end

it causes this error:

  2) Error:
windows local command#test_0003_run command with exit code:
EOFError: end of file reached
    C:/Users/jstocks/code/train/lib/train/transports/local.rb:154:in `readline'
    C:/Users/jstocks/code/train/lib/train/transports/local.rb:154:in `run_command'
    C:/Users/jstocks/code/train/lib/train/transports/local.rb:76:in `run_command_via_connection'
    C:/Users/jstocks/code/train/lib/train/plugins/base_connection.rb:128:in `run_command'
    test/windows/local_test.rb:40:in `block (2 levels) in <main>'

28 runs, 45 assertions, 2 failures, 1 errors, 0 skips
Traceback (most recent call last):
        3: from C:/Users/jstocks/code/train/lib/train/transports/local.rb:225:in `block in start_pipe_server'
        2: from C:/Users/jstocks/code/bundles/train/ruby/2.6.0/gems/win32-process-0.8.3/lib/win32/process.rb:797:in `kill'
        1: from C:/Users/jstocks/code/bundles/train/ruby/2.6.0/gems/win32-process-0.8.3/lib/win32/process.rb:797:in `each'
C:/Users/jstocks/code/bundles/train/ruby/2.6.0/gems/win32-process-0.8.3/lib/win32/process.rb:850:in `block in kill': Input/output error - TerminateProcess (Errno::EIO)
rake aborted!

I can put this failing test on a different branch and file an issue for it?

zenspider

Looks good now that we have a couple tests. Thank you.

If you have resources you referenced while you were doing this work, it'd be great if you cited them in a comment somewhere close to the impl. We suck at powershell.

james-stocks · 2019-11-15T11:29:57Z

lib/train/transports/local.rb

+                $exit_code = $LastExitCode
+                if ($exit_code -eq $null)
+                {
+                  $exit_code = 0


This treats an unset $LastExitCode as 0.
This is fiddling the result, but it preserves the previous behaviour where we set the exit_code to 0 if nothing went wrong.
I could not do this, but it might break workflow if any current users are testing that 0 is returned for their command.

This runs test fixture scripts. There seems to be an outstanding issue where the command being run cannot directly exit non-zero because it results in a `Input/output error - TerminateProcess (Errno::EIO)` exception. Signed-off-by: James Stocks <jstocks@chef.io>

Previously local Windows commands hardcoded the exit code as 0. After changing this to get the real exit code; there are cases where LASTEXITCODE is not set and is `$null`. `$null` is probably the correct value to reflect in this case; but to maintain compatibility with previous versions of train that returned 0, this commit has train return 0 when LASTEXITCODE is `$null` Signed-off-by: James Stocks <jstocks@chef.io>

james-stocks · 2019-11-15T13:33:44Z

@zenspider good call - I added 1 more unit test and a code comment to define the desired behaviour.
I can't easily find a good single reference article that explains PowerShell exit statuses. PowerShell has both a boolean $? exit status variable for the success status of the last command and a numeric $LASTEXITCODE variable with the exit code value of the last time exit was called*; and they behave differently (but not totally independently).

*note potential confusing behaviour here - $LASTEXITCODE is $null for a new PowerShell session and it's value represents the last script that bothered to call exit, not the last script that executed.

To represent this properly, Windows and Linux might not be able to share a uniform CommandResult structure.

Make sure the behaviour of Windows exit status is well defined. Signed-off-by: James Stocks <jstocks@chef.io>

codeclimate · 2019-11-15T13:36:23Z

Code Climate has analyzed commit 79aa959 and detected 4 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Style	4

View more on Code Climate.

james-stocks · 2019-11-15T13:39:43Z

lib/train/transports/local.rb

@@ -146,6 +146,12 @@ def initialize(powershell_cmd = "powershell")
          raise PipeError if @pipe.nil?
        end

+        # @param  cmd The command to execute
+        # @return Local::ComandResult with stdout, stderr and exitstatus
+        #         Note that exitstatus ($?) in PowerShell is boolean, but we use a numeric exit code.


Code Climate says these comment lines are too long; but the Rakefile rubocop task doesn't mind them.
I'd rather have them this length for human readability. Do I need to shorten them?

Codeclimate is not required (yet).

Return exit code for local Windows command

7769824

Partly addresses inspec#288 but that issue also requires the stderr to be provided. Signed-off-by: James Stocks <jstocks@chef.io>

james-stocks requested a review from skpaterson November 8, 2019 10:56

skpaterson approved these changes Nov 8, 2019

View reviewed changes

james-stocks commented Nov 8, 2019

View reviewed changes

miah suggested changes Nov 14, 2019

View reviewed changes

miah requested a review from a team November 14, 2019 17:35

ghost requested review from clintoncwolfe and miah and removed request for a team November 14, 2019 17:35

miah suggested changes Nov 14, 2019

View reviewed changes

clintoncwolfe requested changes Nov 14, 2019

View reviewed changes

james-stocks requested a review from a team as a code owner November 15, 2019 11:20

ghost requested review from clintoncwolfe and zenspider November 15, 2019 11:20

zenspider approved these changes Nov 15, 2019

View reviewed changes

zenspider requested a review from miah November 15, 2019 11:27

james-stocks commented Nov 15, 2019

View reviewed changes

James Stocks added 2 commits November 15, 2019 11:38

james-stocks force-pushed the js/windows_local_exit_code branch from d767739 to a2b30f7 Compare November 15, 2019 11:38

Add code comment and additional unit test

79aa959

Make sure the behaviour of Windows exit status is well defined. Signed-off-by: James Stocks <jstocks@chef.io>

james-stocks force-pushed the js/windows_local_exit_code branch from a9b2955 to 79aa959 Compare November 15, 2019 13:35

james-stocks commented Nov 15, 2019

View reviewed changes

clintoncwolfe approved these changes Nov 18, 2019

View reviewed changes

miah approved these changes Nov 26, 2019

View reviewed changes

zenspider merged commit 4bde05e into inspec:master Dec 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return exit code for local Windows command #533

Return exit code for local Windows command #533

james-stocks commented Nov 8, 2019

chef-expeditor bot commented Nov 8, 2019

skpaterson left a comment

james-stocks Nov 8, 2019

clintoncwolfe Nov 14, 2019

stuartpreston Nov 14, 2019

james-stocks Nov 15, 2019

miah left a comment

miah left a comment

clintoncwolfe left a comment

clintoncwolfe Nov 14, 2019

james-stocks commented Nov 15, 2019

zenspider left a comment

james-stocks Nov 15, 2019

james-stocks commented Nov 15, 2019 •

edited

Loading

codeclimate bot commented Nov 15, 2019

james-stocks Nov 15, 2019

miah Nov 15, 2019

Return exit code for local Windows command #533

Return exit code for local Windows command #533

Conversation

james-stocks commented Nov 8, 2019

Description

Related Issue

Types of changes

Checklist:

chef-expeditor bot commented Nov 8, 2019

skpaterson left a 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

miah left a comment

Choose a reason for hiding this comment

miah left a comment

Choose a reason for hiding this comment

clintoncwolfe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-stocks commented Nov 15, 2019

zenspider left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-stocks commented Nov 15, 2019 • edited Loading

codeclimate bot commented Nov 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

james-stocks commented Nov 15, 2019 •

edited

Loading