introduced a test_and_capture method for backends

[betesh]

Sometimes you need to capture STDERR, or to look back and see what happened if a command failed.

[robd]

@betesh I've also looked at this and had similar thoughts that it could be useful to be able to get the command stdout, stderr and success? from the capture or test methods.

But I think it would be preferable to simply return the command object itself rather than extracting the full_stdout, full_stderr and success? into the CapturedResult object.

Also, I'm not really in favour having a new test_and_capture method on the abstract backend. I would prefer to expose create_command_and_execute (maybe rename to execute_command). Then to get the behaviour you want, you could call:

on '1.example.com'
  result = execute_command("whatever", raise_on_non_zero_exit: false,  verbosity: Logger::DEBUG)
  if result.full_stderr.strip == 'whatever'
    # Do something
  end
end

raise_on_non_zero_exit: false, verbosity: Logger::DEBUG seems a bit combersome, so maybe we could introduce a new test option which combines these:

on '1.example.com'
  if execute_command("whatever", test: true).full_stderr.strip == 'whatever'
    # Do something
  end
end

What do you think?

[betesh]

@robd The Command class wasn't written with direct user access in mind. It has almost no documentation, and exposes too many methods related to the execution of the command, whereas CapturedResult only has knowledge of the results of the command.

I think taking your approach would require that we first improve documentation there, and make all methods private except what absolutely must be public, and may still necessitate separating the result and the execution into separate classes.

Your approach would also sacrifice the convenience of the "strip" option, making the interface inconsistent with that of execute, test and capture.

If you feel strongly that creating a new pollutes the interface too much, I would take your suggestion of test: true, modify it to capture: true and merge it with test:

  def test(*args)
    options = { strip: true }.merge(args.extract_options!).merge(
      raise_on_non_zero_exit: false, verbosity: Logger::DEBUG
    )
    cmd = create_command_and_execute(args, options)
    if options[:capture]
      stdout, stderr = cmd.full_stdout, cmd.full_stderr
      if options[:strip]
        stdout = stdout.strip
        stderr = stderr.strip
      end
      CapturedResult.new(cmd.success?, stdout, stderr)
    else
      cmd.success?
     end
  end

The only shortcoming I see in this example is that test doesn't always return the same type, so that they can't just use if test("my_common", options) if they don't know whether options[:capture] is true--they might think test returned a boolean, when in fact it returned a CapturedResult. Does Ruby have any solution to that (i.e. the equivalent to C++'s explicit operator bool() const;)?

[leehambley]

Hi Guys, please let me stop you, I can't see myself wanting to merge this, if a test is so complex that you need to capture the output, why not simply repeat the command with a capture in the else block, so you can show a sane message?

The argument about needing standard error is a fine one, and I actually can't tell you off the top of my head if I expect capture() to grab one, or both of the standard output streams, but that would be the easier place to fix this. It sounds like a debugging method, and I wouldn't like to make it a part of the public API.

[robd]

@betesh Just to follow up here, I agree that the Command object has a wide public API, I like the idea of splitting the CommandResult from the execution related parts of the Command. This CommandResult could be returned from the create_command_and_execute method.

I think that the Command could be immutable, with just the responsibilities relating to actually running the command (ie the command, args and options + associated methods) .

The CommandResult could have the attributes to support the methods you have on CapturedCommand (@exit_status, @full_stdout and @full_stderr) plus also @started_at, @started. CommandResult would then have a cut down API which would be simpler to use. It could also have a @command attribute, in case the user needed to get hold of the original command which led to the command result.

I think splitting the Command and CommandResult would also help with an issue I tried to fix in Airbrussh: mattbrictson/airbrussh#53. At the moment it is hard to distinguish between executions of a command across servers, and calling the same command multiple times. I think ideally, each Command should be created once per user execution and then passed to each backend, rather than each backend creating it's own Command. Each backend could then return it's own CommandResult.

@leehambley What do you think about splitting the Command and CommandResult?

[betesh]

if a test is so complex that you need to capture the output, why not simply repeat the command with a capture in the else block, so you can show a sane message?

1. Not all commands are idempotent.
2. Not all commands are fast.

@leehambley What do you think about splitting the Command and CommandResult?

Can't speak for @leehambley but I +1 this

[leehambley]

1. Not all commands are idempotent.
2. Not all commands are fast.

Maybe slow and dangerous commands should have their logs simply redirected to a file, and incase the test() fails you can look at the files with capture() or similar? That seems saner especially given the argument that it's still for debugging.

[leehambley]

At the heart of all the things I do with SSHKit and Capistrano is questioning if there's more harm done by saying "this is an edge case which you can solve with a little workaround"… or that we gradually widen the API until it's so complex, our convenience wrapper, with pretty logging that we built around Net::SSH has become a monster with an API as-wide, but not-quite-the-same-as Net::SSH, and we've "improved" away all the things that made SSHKit fun and convenient to use.

[leehambley]

@leehambley What do you think about splitting the Command and CommandResult?

Totally in favour of that, I can imagine how you struggled in Airbrussh - but I don't think adding another test_with_capture_but_only_stderr_not_stdout() is really worthwhile.

[betesh]

test_with_capture_but_only_stderr_not_stdout is getting a little exaggerated. If you're not interested in the STDOUT of a CommandResult, just don't look at it.

So going forward, how should I approach a PR on this? By splitting Command and CommandResult and making create_command_and_execute a public method that returns the latter? Does that mean that the CommandResult should only be created once the command execution is finished? Is there anything else it should expose besides success, stderr and stdout (runtime?, exit_status?, uuid?)

[leehambley]

I think that splitting Command, and CommandResult makes sense, nothing more.

[betesh]

@leehambley The direction you've given me is vague. It does not leave me with confidence that if I redo this PR, trying to follow your instructions this time around, it will be merged.

Clearly I have not persuaded you on the main goal of this PR, which is to give me a clean way to run a command once, and have success?, STDOUT, and STDERR public available without any monkey-patching.