Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix command exists check on Windows with full paths #1850

Merged
merged 2 commits into from
Jun 6, 2017

Conversation

username-is-already-taken2
Copy link
Contributor

@username-is-already-taken2 username-is-already-taken2 commented May 25, 2017

Amended the command.rb file as it wasn't passing the path correctly.

Needed to force the return of exit code numbers (rather than $ture $false) to maintain compatiblity with the existing script

chefconf 2017

Fixes #1839

Signed-off-by: username-is-already-taken2 <gary.bright@niu-solutions.com>
Copy link
Contributor

@arlimus arlimus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yay thank you @username-is-already-taken2 for hacking this at ChefConf 😄

Copy link
Contributor

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome improvement @username-is-already-taken2

@arlimus
Copy link
Contributor

arlimus commented May 26, 2017

one more verification for regressions...

Copy link
Contributor

@chris-rock chris-rock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While Test-Path looks great at first, the following is not turning green:

cmd = 'C:\Windows\System32\cscript.exe'
describe powershell("If(Test-Path \"#{cmd}\"){exit 0}Else{exit 1}") do
  its('exit_status') { should eq 0 }
end

cmd2 = 'cscript.exe'
describe powershell("If(Test-Path \"#{cmd2}\"){exit 0}Else{exit 1}") do
  its('exit_status') { should eq 0 }
end

We need to find a solution for all commands without absolute paths

Copy link
Contributor

@arlimus arlimus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as detected by Chris :) i think a simple condition should solve this

@arlimus arlimus dismissed their stale review May 27, 2017 23:36

Duplicate of Chris' review

Signed-off-by: username-is-already-taken2 <gary.bright@niu-solutions.com>
@username-is-already-taken2
Copy link
Contributor Author

I thought I might have to get funky with an if statement, in the end I swapped out the cmdlet

Here is what I used to validate it.

describe command('C:\Windows\System32\cscript.exe') do
  it { should exist }
  its('exit_status') { should eq 0 }
end

describe command('C:/Windows/System32/cscript.exe') do
  it { should exist }
  its('exit_status') { should eq 0 }
end

describe command('cscript.exe') do
  it { should exist }
  its('exit_status') { should eq 0 }
end

describe command('cscript2.exe') do
  it { should_not exist }
end

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have revised the cmdlet used and updated the pr

@adamleff adamleff changed the title Amended command.rb in order resolve issue 1839 Fix command exists check on Windows with full paths May 31, 2017
Copy link
Contributor

@adamleff adamleff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this locally and it works great! Thanks again, @username-is-already-taken2

@adamleff adamleff added the Type: Bug Feature not working as expected label Jun 6, 2017
Copy link
Contributor

@arlimus arlimus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and verified, awesome update @username-is-already-taken2 !! Kudos :)

@arlimus arlimus dismissed chris-rock’s stale review June 6, 2017 12:46

verified it, works now

@arlimus arlimus merged commit 5fd558f into inspec:master Jun 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Feature not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants