Change LinuxAdmin.run to return a CommandResult object. #32

jrafanie · 2013-08-13T19:07:32Z

Don't merge... yet.

This is a work in progress.

jrafanie · 2013-08-13T19:09:36Z

There's still some tests and code that use return_exitstatus and return_output that I need to remove and update but I changed some of them to see how things would change.

coveralls · 2013-08-13T19:10:51Z

Coverage decreased (-0%) when pulling 4961ff0 on jrafanie:run_returns_command_result_object into d35ed16 on ManageIQ:master.

jrafanie · 2013-08-13T19:14:09Z

Added @Fryguy's email:

1.9.3-p429 :001 > LinuxAdmin.run("true")
 => #<CommandResult:0x007fd2349feeb8 @output="", @error="", @exit_status=0, @exception=nil>

1.9.3-p429 :002 > LinuxAdmin.run!("true")
 => #<CommandResult:0x007fd2349feeb9 @output="", @error="", @exit_status=0, @exception=nil>

1.9.3-p429 :003 > LinuxAdmin.run("echo Hi")
 => #<CommandResult:0x007fd2349feeb8 @output="Hi\n", @error="", @exit_status=0, @exception=nil>

1.9.3-p429 :004 > LinuxAdmin.run!("echo Hi")
 => #<CommandResult:0x007fd2349feeb9 @output="Hi\n", @error="", @exit_status=0, @exception=nil>



1.9.3-p429 :005 > LinuxAdmin.run("false")
 => #<CommandResult:0x007fd2349feeba @output="", @error="", @exit_status=1, @exception=nil>

1.9.3-p429 :006 > LinuxAdmin.run!("false")
CommandResultError: false: exit code: 1
        from (irb):1:in ``'
        from (irb):1
        from /Users/jfrey/.rvm/rubies/ruby-1.9.3-p429/bin/irb:16:in `<main>'

1.9.3-p429 :007 > LinuxAdmin.run("grep XXX missing.txt")
 => #<CommandResult:0x007fd2349feeba @output="", @error="grep: missing.txt: No such file or directory", @exit_status=2, @exception=nil>

1.9.3-p429 :008 > LinuxAdmin.run!("grep XXX missing.txt")
CommandResultError: grep XXX missing.txt: exit code: 2
        from (irb):1:in ``'
        from (irb):1
        from /Users/jfrey/.rvm/rubies/ruby-1.9.3-p429/bin/irb:16:in `<main>'

1.9.3-p429 :009 > begin; LinuxAdmin.run!("grep XXX missing.txt"); rescue CommandResultError => err; err.result ; end
 => #<CommandResult:0x007fd2349feeba @output="", @error="grep: missing.txt: No such file or directory", @exit_status=2, @exception=nil>



1.9.3-p429 :010 > LinuxAdmin.run("XXX")
 => #<CommandResult:0x007fd2349feebb @output="", @error="", @exit_status=nil, @exception=#<Errno::ENOENT: No such file or directory - XXX>>

1.9.3-p429 :011 > LinuxAdmin.run!("XXX")
Errno::ENOENT: No such file or directory - XXX
        from (irb):1:in ``'
        from (irb):1
        from /Users/jfrey/.rvm/rubies/ruby-1.9.3-p429/bin/irb:16:in `<main>'

Fryguy · 2013-08-13T19:17:43Z

lib/linux_admin/command_result.rb

After having merged #30, should this now go in exceptions.rb, or should that file go away again?

@Fryguy @brandondunne I don't mind if CommandError is replaced with CommandResultError in exceptions.rb or I change the name of this CommandResultError to CommandError and remove the existing one from #30. Thoughts?

Fryguy · 2013-08-13T19:23:34Z

Branch doesn't merge because of #30. It's going to affect some of the specs that raise_error as well as the stuff with exceptions.rb

jrafanie · 2013-08-13T22:46:08Z

@Fryguy @brandondunne I'm not done, but I got the shared examples working for run and run!, plus all of the comments here.

I still need to clean up the remaining return_exitstatus and return_output options in specs and code.

I'll split up the commits to help reviewing tomorrow.

This code works with @Fryguy's email examples.

coveralls · 2013-08-13T22:47:51Z

Coverage increased (+0%) when pulling 4fc5d54 on jrafanie:run_returns_command_result_object into d76dc65 on ManageIQ:master.

Fryguy · 2013-08-13T22:50:51Z

lib/linux_admin/common.rb

Are you ignoring exceptions here? If so, you don't need the => exception part

Oh nvm, I see you are putting into the CommandResult below. Maybe a comment here to explain. Something like # Saving the exception into an variable so it can be passed to the CommandResult.

Fryguy · 2013-08-13T22:55:06Z

Looks good. I think the only thing missing is that nearly all of the run calls need to be changed to run! as that is how they were originally written.

coveralls · 2013-08-14T20:31:46Z

Coverage increased (+0%) when pulling 8015e83 on jrafanie:run_returns_command_result_object into d76dc65 on ManageIQ:master.

coveralls · 2013-08-14T20:39:32Z

Coverage increased (+0%) when pulling 7e1341d on jrafanie:run_returns_command_result_object into d76dc65 on ManageIQ:master.

coveralls · 2013-08-14T20:51:45Z

Coverage increased (+0%) when pulling 4a3504c on jrafanie:run_returns_command_result_object into d76dc65 on ManageIQ:master.

Fryguy · 2013-08-14T21:46:50Z

lib/linux_admin/common.rb

Looks like this is no longer needed here.

coveralls · 2013-08-15T18:14:36Z

Coverage remained the same when pulling 761c60d on jrafanie:run_returns_command_result_object into 607b634 on ManageIQ:master.

coveralls · 2013-08-15T18:24:40Z

Coverage increased (+0%) when pulling 2c7fef8 on jrafanie:run_returns_command_result_object into 607b634 on ManageIQ:master.

jrafanie · 2013-08-15T18:35:44Z

@Fryguy @brandondunne @movitto

Pushed all of my changes to rework the run method. We should cut a new tag before merging this change.

Here is @Fryguy's console examples updated with the latest changes (always raise on bad commands):

1.9.3-p392 :001 > LinuxAdmin.run("true")
 => #<CommandResult:0x007f840610f108 @output="", @error="", @exit_status=0>

1.9.3-p392 :002 > LinuxAdmin.run!("true")
 => #<CommandResult:0x007f84050d0738 @output="", @error="", @exit_status=0>

1.9.3-p392 :003 > LinuxAdmin.run("echo Hi")
 => #<CommandResult:0x007f840383eeb8 @output="Hi\n", @error="", @exit_status=0>

1.9.3-p392 :004 > LinuxAdmin.run!("echo Hi")
 => #<CommandResult:0x007f84038a98f8 @output="Hi\n", @error="", @exit_status=0>



1.9.3-p392 :007 > LinuxAdmin.run("false")
 => #<CommandResult:0x007f84038f6d10 @output="", @error="", @exit_status=1>

1.9.3-p392 :008 > LinuxAdmin.run!("false")
CommandResultError: false exit code: 1
  from linux_admin/lib/linux_admin/common.rb:37:in 'run!'
  from (irb):8

1.9.3-p392 :009 > LinuxAdmin.run("grep XXX missing.txt")
 => #<CommandResult:0x007f840507acc0 @output="", @error="grep: missing.txt: No such file or directory\n", @exit_status=2>

1.9.3-p392 :010 > LinuxAdmin.run!("grep XXX missing.txt")
CommandResultError: grep XXX missing.txt exit code: 2
  from linux_admin/lib/linux_admin/common.rb:37:in 'run!'
  from (irb):10

1.9.3-p392 :011 > begin; LinuxAdmin.run!("grep XXX missing.txt"); rescue CommandResultError => err; err.result ; end
 => #<CommandResult:0x007f84051406a0 @output="", @error="grep: missing.txt: No such file or directory\n", @exit_status=2>



1.9.3-p392 :016 > LinuxAdmin.run("XXX")
Errno::ENOENT: No such file or directory - XXX
  from linux_admin/lib/linux_admin/common.rb:77:in 'spawn'
  from linux_admin/lib/linux_admin/common.rb:77:in 'launch'

1.9.3-p392 :017 > LinuxAdmin.run!("XXX")
Errno::ENOENT: No such file or directory - XXX
  from linux_admin/lib/linux_admin/common.rb:77:in 'spawn'
  from linux_admin/lib/linux_admin/common.rb:77:in 'launch'

Fryguy · 2013-08-15T18:37:01Z

spec/common_spec.rb

If the false method execution doesn't blow up with CommandResultError, then there are no assertions, which will resulting in a passing spec. Maybe we should add a guard for this with something like:

begin subject.send(run_method, "false") rescue CommandResultError => err expect(err.result).to be_kind_of CommandResult else raise "Expected failure" end

The other option is to the use expect {}.to raise_error with a block to handle extra assertions about the error. https://www.relishapp.com/rspec/rspec-expectations/v/2-6/docs/built-in-matchers/raise-error-matcher#set-expectations-on-error-object-passed-to-block I'm not sure our version of rspec supports that though, because I tried this the other day and it would never go into the block.

@Fryguy Good call.

Fryguy · 2013-08-15T18:42:00Z

@jrafanie This is great! Love how this all turned out. I made a small point about 1 spec, but I think this is ready to go.

LinuxAdmin.run(cmd).output - returns the standard output. LinuxAdmin.run(cmd).exit_status - returns the exit code. LinuxAdmin.run(cmd).error - returns the standard errror. LinuxAdmin.run(cmd) for a bad command will raise the original exception, such as: Errno::ENOENT: No such file or directory. LinuxAdmin.run! was added to behave similarly to the old run's functionality in that it raises an exception on failures: 1) CommandResultError is raised on a non-zero exit status. 2) The failed system call exception(such as Errno::ENOENT) is raised on a bad command.

run was changed to run! except where we need to check the exit_status, see below. 1) To execute a command for the side effect only (raises on non-zero exit and bad command). Before: run(cmd, :params => params) After: run!(cmd, :params => params) 2) To execute a command and check the exit_status (raises on bad command only). Before: run(cmd, :return_exitstatus => true) == 0 After: run(cmd).exit_status == 0 3) To execute a command and capture the output (raises on non-zero exit and bad command). Before: output = run(cmd, :return_output => true) After: output = run!(cmd).output 4) To execute a command and capture the standard error where the command exits 0. Before: Was not possible previously. After: error = run!(cmd).error 5) To execute a command and capture the standard error without checking the command exit status. Before: Was not possible previously. After: error = run(cmd).error

jrafanie · 2013-08-15T19:23:50Z

@Fryguy Repushed with requested changes.

coveralls · 2013-08-15T19:25:52Z

Coverage remained the same when pulling 95f49d5 on jrafanie:run_returns_command_result_object into 607b634 on ManageIQ:master.

Fryguy · 2013-08-15T19:50:28Z

Looks great. This is ready for merge, but I think you wanted to cut a patch version of the gem before cutting this as a minor release? Let me know when you are ready for merge.

It is fixed on master, see: rspec/rspec-expectations@b0df827

jrafanie · 2013-08-15T20:24:27Z

@Fryguy @brandondunne @movitto Finally. We're ready to merge after tagging.

Be sure to check out the last commit, a workaround for a bug in 2.14.x rspec-expectations: jrafanie@e6c94fb

coveralls · 2013-08-15T20:25:32Z

Coverage increased (+0%) when pulling e6c94fb on jrafanie:run_returns_command_result_object into 607b634 on ManageIQ:master.

Change LinuxAdmin.run to return a CommandResult object.

Fryguy reviewed Aug 13, 2013
View reviewed changes

Fryguy reviewed Aug 14, 2013
View reviewed changes

lib/linux_admin/common.rb Outdated

Copy link

Member

Fryguy Aug 14, 2013

Choose a reason for hiding this comment

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

Looks like this is no longer needed here.

Fryguy reviewed Aug 15, 2013
View reviewed changes

jrafanie added 3 commits August 15, 2013 15:18

Fix some specs spawning when they shouldn't be.

81819b0

rspec-expectations 2.14.x discards do/end blocks passed to raise_error.

e6c94fb

It is fixed on master, see: rspec/rspec-expectations@b0df827

Fryguy added a commit that referenced this pull request Aug 15, 2013

Merge pull request #32 from jrafanie/run_returns_command_result_object

428621f

Change LinuxAdmin.run to return a CommandResult object.

Fryguy merged commit 428621f into ManageIQ:master Aug 15, 2013

jrafanie deleted the run_returns_command_result_object branch August 15, 2013 21:17

Change LinuxAdmin.run to return a CommandResult object. #32

Change LinuxAdmin.run to return a CommandResult object. #32

Uh oh!

Conversation

jrafanie commented Aug 13, 2013

Uh oh!

jrafanie commented Aug 13, 2013

Uh oh!

coveralls commented Aug 13, 2013

Uh oh!

jrafanie commented Aug 13, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fryguy commented Aug 13, 2013

Uh oh!

jrafanie commented Aug 13, 2013

Uh oh!

coveralls commented Aug 13, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Fryguy commented Aug 13, 2013

Uh oh!

coveralls commented Aug 14, 2013

Uh oh!

coveralls commented Aug 14, 2013

Uh oh!

coveralls commented Aug 14, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Aug 15, 2013

Uh oh!

coveralls commented Aug 15, 2013

Uh oh!

jrafanie commented Aug 15, 2013

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!

Fryguy commented Aug 15, 2013

Uh oh!

jrafanie commented Aug 15, 2013

Uh oh!

coveralls commented Aug 15, 2013

Uh oh!

Fryguy commented Aug 15, 2013

Uh oh!

jrafanie commented Aug 15, 2013

Uh oh!

coveralls commented Aug 15, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants