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

Improve interaction handler api #242

Merged
merged 7 commits into from
May 6, 2015

Conversation

robd
Copy link
Contributor

@robd robd commented May 4, 2015

This PR contains 4 improvements to the :interaction_handler option API from trying to use this with Capistrano in the real world:

  1. Put the MappingInteractionHandler class in the SSHKit module
  2. Support specifying an interaction_handler as a hash
# Before:
execute(:some_command, interaction_handler: SSHKit::MappingInteractionHandler.new("Some output\n" => "Input\n"))
# After:
execute(:some_command, interaction_handler: { "Some output\n" => "Input\n"})
  1. No longer append a newline to the user input. This removes an inconsistency between the server side and user input side of the mapping:
# Before - newline needed on server output only because newline magically appended to input:
execute(:some_command, interaction_handler: { "Some output\n" => "Input"})
# After - no magic newline behaviour; all newlines should be specified:
execute(:some_command, interaction_handler: { "Some output\n" => "Input\n"})
  1. Remove theoutput constructor parameter and attribute from MappingInteractionHandler. This was only added for unit testing purposes, but meant that it didn't work with airbrussh. It also meant that MappingInteractionHandler would hold on to a reference to SSHKit.config.output which seems a bit dodgy to me in the case that SSHKit.config.output is updated after the MappingInteractionHandler is created.

robd added 4 commits May 4, 2015 16:11
This removes a confusing parameter from initialize.
Also fixes a problem with Airbrussh, which is forced to deep copy the command in order to preserve the stdout/stderr.
Wrap hashes in an SSHKit::MappingInteractionHandler to reduce duplicate calls to SSHKit::MappingInteractionHandler.new in client code
This makes the newline handling consistent in the mapping hash. Newlines must always be spcified by the user. It was previously confusing to have to specify the newlines on the server response matching side but not on the user input side.
@leehambley
Copy link
Member

Would a regex work here?

execute(:some_command, interaction_handler: { /Some output\n+/ => "Input\n"})

Calling @stdout.clear broke airbrussh, and was hard to debug. This commit restores the previous (better) behaviour of simply assigning a new blank string when clearing.
@robd
Copy link
Contributor Author

robd commented May 5, 2015

@leehambley I have thought about adding support for this. At the moment, I simply treat the hash keys as hash keys so it won't work (I don't think, because hash uses eql? for key equality), but obviously I could add support for this.

Another option would be to support a lambda:

execute(:some_command, interaction_handler: lambda { |output_line|
  case output_line
  when /Some output\n+/
    "Input\n"
  end
)

I definitely think that some way to match a regex would be helpful. This { /Some output\n+/ => "Input\n"} feels a bit non idiomatic, but is much nicer for one liners.

@robd
Copy link
Contributor Author

robd commented May 5, 2015

I also added a fix (e93f819) for a change of behaviour I accidentally introduced to do with mutating strings which broke airbrussh in a non obvious way and forced @mattbrictson to implement a pretty nasty workaround.

@leehambley
Copy link
Member

Oopse sorry @mattbrictson :) I missed the link with airbrussh when reviewing this :(

@robd
Copy link
Contributor Author

robd commented May 5, 2015

@leehambley I'm adding regex support now. Will update this PR soon.

@robd robd force-pushed the improve-interaction-handler-api branch 4 times, most recently from 84f90c5 to a9591fd Compare May 5, 2015 15:21
@robd
Copy link
Contributor Author

robd commented May 5, 2015

@leehambley OK, I now compare the hash keys using case equality to support regexes. I also support specifying a proc. I also changed the behaviour for unmapped commands to output at the debug level instead of warning level.

Both now supported:

execute(:some_command, interaction_handler: { /Some output\n+/ => "Input\n"})

# OR

execute(:some_command, interaction_handler: lambda { |output_line|
  case output_line
  when /Some output\n+/
    "Input\n"
  end
)

One thing to note is that since the on_stdout/on_stderr are called once per stdout/stderr line, I think that \n+ in your regex won't ever get a chance to match multiple newlines.

@robd robd force-pushed the improve-interaction-handler-api branch from a9591fd to 2051b8e Compare May 5, 2015 15:45
@robd robd force-pushed the improve-interaction-handler-api branch from 2051b8e to ea71dd5 Compare May 5, 2015 15:46
@mattbrictson
Copy link
Member

@robd Thanks! I tested this branch and it fixes the issue that required me to add the deep_copy workaround to airbrussh.

@robd
Copy link
Contributor Author

robd commented May 5, 2015

@mattbrictson OK great - thanks vm for checking

To ensure SSHKit.reset_configuration! is called. Also removed some unnecessary stubbing.
@leehambley
Copy link
Member

At first glance (can't review too thoroughly, or try your branch out) but this all looks sane, with such a flexible API making the docs clean and clear will be essential.

@leehambley
Copy link
Member

Happy to merge, if you are ?

@robd
Copy link
Contributor Author

robd commented May 6, 2015

@leehambley Yes, I'm happy with this - thanks. I hope the Readme covers all usage patterns, but if anything crops up I will raise as a separate PR.

leehambley added a commit that referenced this pull request May 6, 2015
@leehambley leehambley merged commit 5e07f9d into capistrano:master May 6, 2015
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Dec 14, 2015
## 1.8.1

  * Change license to MIT, thanks to all the patient contributors who gave
    their permissions.

## 1.8.0

  * add SSHKit::Backend::ConnectionPool#close_connections
    [PR #285](capistrano/sshkit#285)
    @akm
  * Clean up rubocop lint warnings
    [PR #275](capistrano/sshkit#275)
    @cshaffer
    * Prepend unused parameter names with an underscore
    * Prefer “safe assignment in condition”
    * Disambiguate regexp literals with parens
    * Prefer `sprintf` over `String#%`
    * No longer shadow `caller_line` variable in `DeprecationLogger`
    * Rescue `StandardError` instead of `Exception`
    * Remove useless `private` access modifier in `TestAbstract`
    * Disambiguate block operator with parens
    * Disambiguate between grouped expression and method params
    * Remove assertion in `TestHost#test_assert_hosts_compare_equal` that compares something with itself
  * Export environment variables and execute command in a subshell.
    [PR #273](capistrano/sshkit#273)
    @kuon
  * Introduce `log_command_start`, `log_command_data`, `log_command_exit` methods on `Formatter`
    [PR #257](capistrano/sshkit#257)
    @robd
    * Deprecate `@stdout` and `@stderr` accessors on `Command`
  * Add support for deprecation logging options.
    [README](README.md#deprecation-warnings),
    [PR #258](capistrano/sshkit#258)
    @robd
  * Quote environment variable values.
    [PR #250](capistrano/sshkit#250)
    @Sinjo - Chris Sinjakli
  * Simplified formatter hierarchy.
    [PR #248](capistrano/sshkit#248)
    @robd
    * `SimpleText` formatter now extends `Pretty`, rather than duplicating.
  * Hide ANSI color escape sequences when outputting to a file.
    [README](README.md#output-colors),
    [Issue #245](capistrano/sshkit#245),
    [PR #246](capistrano/sshkit#246)
    @robd
    * Now only color the output if it is associated with a tty,
      or the `SSHKIT_COLOR` environment variable is set.
  * Removed broken support for assigning an `IO` to the `output` config option.
    [Issue #243](capistrano/sshkit#243),
    [PR #244](capistrano/sshkit#244)
    @robd
    * Use `SSHKit.config.output = SSHKit::Formatter::SimpleText.new($stdin)` instead
  * Added support for `:interaction_handler` option on commands.
    [PR #234](capistrano/sshkit#234),
    [PR #242](capistrano/sshkit#242)
    @robd
  * Removed partially supported `TRACE` log level.
    [2aa7890](capistrano/sshkit@2aa7890)
    @robd
  * Add support for the `:strip` option to the `capture` method and strip by default on the `Local` backend.
    [PR #239](capistrano/sshkit#239),
    [PR #249](capistrano/sshkit#249)
    @robd
    * The `Local` backend now strips by default to be consistent with the `Netssh` one.
    * This reverses change [7d15a9a](capistrano/sshkit@7d15a9a) to the `Local` capture API to remove stripping by default.
    * If you require the raw, unstripped output, pass the `strip: false` option: `capture(:ls, strip: false)`
  * Simplified backend hierarchy.
    [PR #235](capistrano/sshkit#235),
    [PR #237](capistrano/sshkit#237)
    @robd
    * Moved duplicate implementations of `make`, `rake`, `test`, `capture`, `background` on to `Abstract` backend.
    * Backend implementations now only need to implement `execute_command`, `upload!` and `download!`
    * Removed `Printer` from backend hierarchy for `Local` and `Netssh` backends (they now just extend `Abstract`)
    * Removed unused `Net::SSH:LogLevelShim`
  * Removed dependency on the `colorize` gem. SSHKit now implements its own ANSI color logic, with no external dependencies. Note that SSHKit now only supports the `:bold` or plain modes. Other modes will be gracefully ignored. [#263](capistrano/sshkit#263)
  * New API for setting the formatter: `use_format`. This differs from `format=` in that it accepts options or arguments that will be passed to the formatter's constructor. The `format=` syntax will be deprecated in a future release. [#295](capistrano/sshkit#295)
  * SSHKit now immediately raises a `NameError` if you try to set a formatter that does not exist. [#295](capistrano/sshkit#295)
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Dec 14, 2015
## 1.8.1

  * Change license to MIT, thanks to all the patient contributors who gave
    their permissions.

## 1.8.0

  * add SSHKit::Backend::ConnectionPool#close_connections
    [PR #285](capistrano/sshkit#285)
    @akm
  * Clean up rubocop lint warnings
    [PR #275](capistrano/sshkit#275)
    @cshaffer
    * Prepend unused parameter names with an underscore
    * Prefer “safe assignment in condition”
    * Disambiguate regexp literals with parens
    * Prefer `sprintf` over `String#%`
    * No longer shadow `caller_line` variable in `DeprecationLogger`
    * Rescue `StandardError` instead of `Exception`
    * Remove useless `private` access modifier in `TestAbstract`
    * Disambiguate block operator with parens
    * Disambiguate between grouped expression and method params
    * Remove assertion in `TestHost#test_assert_hosts_compare_equal` that compares something with itself
  * Export environment variables and execute command in a subshell.
    [PR #273](capistrano/sshkit#273)
    @kuon
  * Introduce `log_command_start`, `log_command_data`, `log_command_exit` methods on `Formatter`
    [PR #257](capistrano/sshkit#257)
    @robd
    * Deprecate `@stdout` and `@stderr` accessors on `Command`
  * Add support for deprecation logging options.
    [README](README.md#deprecation-warnings),
    [PR #258](capistrano/sshkit#258)
    @robd
  * Quote environment variable values.
    [PR #250](capistrano/sshkit#250)
    @Sinjo - Chris Sinjakli
  * Simplified formatter hierarchy.
    [PR #248](capistrano/sshkit#248)
    @robd
    * `SimpleText` formatter now extends `Pretty`, rather than duplicating.
  * Hide ANSI color escape sequences when outputting to a file.
    [README](README.md#output-colors),
    [Issue #245](capistrano/sshkit#245),
    [PR #246](capistrano/sshkit#246)
    @robd
    * Now only color the output if it is associated with a tty,
      or the `SSHKIT_COLOR` environment variable is set.
  * Removed broken support for assigning an `IO` to the `output` config option.
    [Issue #243](capistrano/sshkit#243),
    [PR #244](capistrano/sshkit#244)
    @robd
    * Use `SSHKit.config.output = SSHKit::Formatter::SimpleText.new($stdin)` instead
  * Added support for `:interaction_handler` option on commands.
    [PR #234](capistrano/sshkit#234),
    [PR #242](capistrano/sshkit#242)
    @robd
  * Removed partially supported `TRACE` log level.
    [2aa7890](capistrano/sshkit@2aa7890)
    @robd
  * Add support for the `:strip` option to the `capture` method and strip by default on the `Local` backend.
    [PR #239](capistrano/sshkit#239),
    [PR #249](capistrano/sshkit#249)
    @robd
    * The `Local` backend now strips by default to be consistent with the `Netssh` one.
    * This reverses change [7d15a9a](capistrano/sshkit@7d15a9a) to the `Local` capture API to remove stripping by default.
    * If you require the raw, unstripped output, pass the `strip: false` option: `capture(:ls, strip: false)`
  * Simplified backend hierarchy.
    [PR #235](capistrano/sshkit#235),
    [PR #237](capistrano/sshkit#237)
    @robd
    * Moved duplicate implementations of `make`, `rake`, `test`, `capture`, `background` on to `Abstract` backend.
    * Backend implementations now only need to implement `execute_command`, `upload!` and `download!`
    * Removed `Printer` from backend hierarchy for `Local` and `Netssh` backends (they now just extend `Abstract`)
    * Removed unused `Net::SSH:LogLevelShim`
  * Removed dependency on the `colorize` gem. SSHKit now implements its own ANSI color logic, with no external dependencies. Note that SSHKit now only supports the `:bold` or plain modes. Other modes will be gracefully ignored. [#263](capistrano/sshkit#263)
  * New API for setting the formatter: `use_format`. This differs from `format=` in that it accepts options or arguments that will be passed to the formatter's constructor. The `format=` syntax will be deprecated in a future release. [#295](capistrano/sshkit#295)
  * SSHKit now immediately raises a `NameError` if you try to set a formatter that does not exist. [#295](capistrano/sshkit#295)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants