LinuxCommand#verify cleaned up #530

kvivek1115 · 2019-11-02T17:34:29Z

Description

Remove for loop and add case statements in order to humanize the error message and class.
LinuxCommand#verify return nil on success and raised the error otherwise.
Minor refactoring.
Add unit test cases.

Related Issue

Fixes #509

Types of changes

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

Checklist:

I have read the CONTRIBUTING document.

Signed-off-by: Vivek Singh [email protected]

chef-expeditor · 2019-11-02T17:34:35Z

Hello vsingh-msys! 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!

kvivek1115 · 2019-11-12T17:43:08Z

Hi @zenspider
Could you please have a look and let's know further if any suggestions/feedback?

zenspider · 2019-11-13T22:06:49Z

lib/train/extras/command_wrapper.rb

-        "sudo: command not found" =>
+      msg, reason =
+        case rawerr
+        when /Sorry, try again/


why are these regexps?

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

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.

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

zenspider · 2019-11-13T22:09:10Z

lib/train/extras/command_wrapper.rb

-            "sudo to allow for non-interactive usage.", :sudo_no_tty],
-      }.each do |sudo, human|
-        rawerr = human if rawerr.include? sudo
-      end


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.

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

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.

zenspider · 2019-11-13T22:10:40Z

lib/train/extras/command_wrapper.rb

        res
      elsif transport.platform.windows?
-        res = WindowsCommand.new(transport, options)
-        res
+        WindowsCommand.new(transport, options)


zenspider · 2019-11-13T22:11:39Z

lib/train/extras/command_wrapper.rb

-          msg, reason = verification_res
-          raise Train::UserError.new("Sudo failed: #{msg}", reason)
-        end
+        res.verify


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.

@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

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

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

zenspider · 2019-11-13T22:13:09Z

test/unit/extras/command_wrapper_test.rb

+    it "custom error for bad sudo password" do
+      backend.stubs(:run_command).returns(mock_connect_result("Sorry, try again", 1))
+      lc = cls.new(backend, { sudo: true })
+      _ { lc.verify }.must_raise Train::UserError


must_raise returns the exception... please test the error message.

Might also want test cases for all of the edge cases covered.

I should add... this can all be refactored into an assertion to make these 1-liners.

Okay noted, thankyou so much for the feedback, will cover more test cases and update you accordingly.

clintoncwolfe

I'm OK with the regexp and case. I would like to see a bit more testing but I think that's coming anyway.

zenspider · 2019-11-21T21:42:55Z

lib/train/extras/command_wrapper.rb

-        "sudo: command not found" =>
+      msg, reason =
+        case rawerr
+        when /Sorry, try again/


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.

zenspider · 2019-11-21T21:44:24Z

lib/train/extras/command_wrapper.rb

-          msg, reason = verification_res
-          raise Train::UserError.new("Sudo failed: #{msg}", reason)
-        end
+        res.verify


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

zenspider · 2019-11-21T21:45:58Z

lib/train/extras/command_wrapper.rb

-            "sudo to allow for non-interactive usage.", :sudo_no_tty],
-      }.each do |sudo, human|
-        rawerr = human if rawerr.include? sudo
-      end


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.

- Minor improvisation & refactoring. - Add unit test cases. Signed-off-by: Vivek Singh <[email protected]>

Signed-off-by: Vivek Singh <[email protected]>

codeclimate · 2019-11-25T15:07:18Z

Code Climate has analyzed commit c029473 and detected 2 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Style	2

View more on Code Climate.

kvivek1115 · 2019-12-02T14:37:15Z

@zenspider could you please have a look again?

zenspider · 2019-12-09T21:05:02Z

test/unit/extras/command_wrapper_test.rb

+      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/)


zenspider · 2019-12-09T21:06:01Z

lib/train/extras/command_wrapper.rb

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


For the future: return and return nil have the same effect.

kvivek1115 changed the title ~~Wrap known exception in LinuxCommand#verify~~ LinuxCommand#verify cleaned up Nov 2, 2019

miah requested review from zenspider and clintoncwolfe November 13, 2019 21:54

miah approved these changes Nov 13, 2019

View reviewed changes

zenspider suggested changes Nov 13, 2019

View reviewed changes

clintoncwolfe approved these changes Nov 14, 2019

View reviewed changes

kvivek1115 requested a review from a team as a code owner November 18, 2019 11:38

zenspider suggested changes Nov 21, 2019

View reviewed changes

Vivek Singh added 3 commits November 24, 2019 16:07

Wrap known exception in LinuxCommand#verify

64bf914

- Minor improvisation & refactoring. - Add unit test cases. Signed-off-by: Vivek Singh <[email protected]>

Add more test cases to validate error message

f884192

Signed-off-by: Vivek Singh <[email protected]>

Add verify! method & exact match for sudo errors.

c029473

Signed-off-by: Vivek Singh <[email protected]>

kvivek1115 force-pushed the VSingh/MSYS-1084_clean_up_linux_command_verify branch from c6465b4 to c029473 Compare November 25, 2019 15:06

zenspider self-requested a review December 4, 2019 23:56

zenspider approved these changes Dec 9, 2019

View reviewed changes

zenspider merged commit d94d44d into inspec:master Jan 15, 2020

LinuxCommand#verify cleaned up #530

LinuxCommand#verify cleaned up #530

Uh oh!

Conversation

kvivek1115 commented Nov 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue

Types of changes

Checklist:

Uh oh!

chef-expeditor bot commented Nov 2, 2019

Uh oh!

kvivek1115 commented Nov 12, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kvivek1115 Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintoncwolfe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codeclimate bot commented Nov 25, 2019

Uh oh!

kvivek1115 commented Dec 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kvivek1115 commented Nov 2, 2019 •

edited

Loading

kvivek1115 Nov 14, 2019 •

edited

Loading