Skip to content

LinuxCommand#verify cleaned up #530

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 15, 2020
Merged

LinuxCommand#verify cleaned up #530

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
64bf914
Wrap known exception in LinuxCommand#verify  
Nov 2, 2019
f884192
Add more test cases to validate error message
Nov 18, 2019
c029473
Add verify! method & exact match for sudo errors.
Nov 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 17 additions & 20 deletions lib/train/extras/command_wrapper.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,22 +57,23 @@ def verify
return nil if res.exit_status == 0

rawerr = res.stdout + " " + res.stderr

{
"Sorry, try again" => ["Wrong sudo password.", :bad_sudo_password],
"sudo: no tty present and no askpass program specified" =>
["Sudo requires a password, please configure it.", :sudo_password_required],
"sudo: command not found" =>
msg, reason =
case rawerr
when /Sorry, try again/
Copy link
Contributor

Choose a reason for hiding this comment

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

why are these regexps?

Copy link
Author

Choose a reason for hiding this comment

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

There is no specific to match regex, I only thought of would be nice if we do exact match vs similar one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, String#=== (what is used under a case/when is an exact match... All of these strings went from "some string" to /some string/, so they went from exact match to a regexp (partial) match. As such, this is looser than before.

Copy link
Author

Choose a reason for hiding this comment

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

@zenspider I do agree with your concern so added minor updates for constructing rawerr and have updated the exact match of string.

["Wrong sudo password.", :bad_sudo_password]
when /sudo: no tty present and no askpass program specified/
["Sudo requires a password, please configure it.", :sudo_password_required]
when /sudo: command not found/
["Can't find sudo command. Please either install and "\
"configure it on the target or deactivate sudo.", :sudo_command_not_found],
"sudo: sorry, you must have a tty to run sudo" =>
"configure it on the target or deactivate sudo.", :sudo_command_not_found]
when /sudo: sorry, you must have a tty to run sudo/
["Sudo requires a TTY. Please see the README on how to configure "\
"sudo to allow for non-interactive usage.", :sudo_no_tty],
}.each do |sudo, human|
rawerr = human if rawerr.include? sudo
end
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I'm sold on the benefits of switching this to a case yet. Can you explain this?

That said, I'm REALLY not sold on the each here... it could probably have been a hash lookup? Unless this bit answers the question about why there's regexps now... in which I case I guess I'm sold on the case + regexps.

Copy link
Author

@kvivek1115 kvivek1115 Nov 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Hey @zenspider,

Here the only reason for switching to the case is if an exception would occur I would rather say:

I am the one instead of I am one of them

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand that. Assuming my interpretation is correct, then you're doing "I am the one" via raise, not via case vs {...}.each.

"sudo to allow for non-interactive usage.", :sudo_no_tty]
else
rawerr
end

rawerr
raise Train::UserError.new("Sudo failed: #{msg}", reason)
end

# (see CommandWrapperBase::run)
Expand Down Expand Up @@ -167,15 +168,11 @@ def self.load(transport, options)
return nil unless LinuxCommand.active?(options)

res = LinuxCommand.new(transport, options)
verification_res = res.verify
if verification_res
msg, reason = verification_res
raise Train::UserError.new("Sudo failed: #{msg}", reason)
end
res.verify
Copy link
Contributor

Choose a reason for hiding this comment

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

Much better... I like that you're just calling it. Please rename it to verify! to denote that it is a bit more angry now.

Copy link
Author

Choose a reason for hiding this comment

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

@zenspider need your suggestion in order to rename verify to verify!.

As we have base class CommandWrapperBase as a defined methods interface class. and the implementation in LinuxCommand & WindowsCommand classes.

We have the following options to rename verify!:

1. Just rename all the places wherever is used e.i. in CommandWrapperBase, WindowsCommand and LinuxCommand classes.

OR

2. For class LinuxCommand define as an alias method.

alias verify! verify

OR

3. Add verify! method as well in order to make angrier

a) verify should return nil on success and array object when an error occurs

    # (see CommandWrapperBase::verify)
    def verify
      res = @backend.run_command(run("echo"))
      return nil if res.exit_status == 0

      rawerr = res.stdout + " " + res.stderr
        case rawerr
        when /Sorry, try again/
          ["Wrong sudo password.", :bad_sudo_password]
        when /sudo: no tty present and no askpass program specified/
          ["Sudo requires a password, please configure it.", :sudo_password_required]
        when /sudo: command not found/
          ["Can't find sudo command. Please either install and "\
            "configure it on the target or deactivate sudo.", :sudo_command_not_found]
        when /sudo: sorry, you must have a tty to run sudo/
          ["Sudo requires a TTY. Please see the README on how to configure "\
            "sudo to allow for non-interactive usage.", :sudo_no_tty]
        else
          [rawerr, nil]
        end
    end

b) Add verify! and raise an error if occurs.

def verify!
    msg, reason = verify
    return nil unless msg

    raise Train::UserError.new("Sudo failed: #{msg}", reason)
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. Then either 3.b or 4: ignore @zenspider.

Copy link
Author

Choose a reason for hiding this comment

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

Added verify! method and updated the PR please have a look again thanks


res
elsif transport.platform.windows?
res = WindowsCommand.new(transport, options)
res
WindowsCommand.new(transport, options)
Copy link
Contributor

Choose a reason for hiding this comment

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

thank you!

end
end
end
Expand Down
47 changes: 47 additions & 0 deletions test/unit/extras/command_wrapper_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,53 @@
_(lc.run(cmd)).must_equal "echo #{bcmd} | base64 --decode | /bin/bash --login"
end
end

describe "#verify" do
def mock_connect_result(stderr, exit_status)
OpenStruct.new(stdout: "", stderr: stderr, exit_status: exit_status)
end

it "returns nil on success" do
backend.stubs(:run_command).returns(mock_connect_result(nil, 0))
lc = cls.new(backend, { sudo: true })
_(lc.verify).must_be_nil
end

it "error message for bad sudo password" do
backend.stubs(:run_command).returns(mock_connect_result("Sorry, try again", 1))
lc = cls.new(backend, { sudo: true })
err = _ { lc.verify }.must_raise Train::UserError
_(err.message).must_match(/Sudo failed: Wrong sudo password./)
end

it "error message for sudo password required" do
backend.stubs(:run_command).returns(mock_connect_result("sudo: no tty present and no askpass program specified", 1))
lc = cls.new(backend, { sudo: true })
err = _ { lc.verify }.must_raise Train::UserError
_(err.message).must_match(/Sudo requires a password, please configure it./)
end

it "error message for sudo: command not found" do
backend.stubs(:run_command).returns(mock_connect_result("sudo: command not found", 1))
lc = cls.new(backend, { sudo: true })
err = _ { lc.verify }.must_raise Train::UserError
_(err.message).must_match(/Can't find sudo command. Please either install and configure it on the target or deactivate sudo./)
end

it "error message for requires tty" do
backend.stubs(:run_command).returns(mock_connect_result("sudo: sorry, you must have a tty to run sudo", 1))
lc = cls.new(backend, { sudo: true })
err = _ { lc.verify }.must_raise Train::UserError
_(err.message).must_match(/Sudo failed: Sudo requires a TTY. Please see the README/)
end

it "error message for other sudo related errors" do
backend.stubs(:run_command).returns(mock_connect_result("Other sudo related error", 1))
lc = cls.new(backend, { sudo: true })
err = _ { lc.verify }.must_raise Train::UserError
_(err.message).must_match(/Other sudo related error/)
Copy link
Contributor

Choose a reason for hiding this comment

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

great!

end
end
end

describe "windows command" do
Expand Down