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

Formatters::Pretty: display command being run #304

Merged
merged 2 commits into from
Dec 18, 2015
Merged

Formatters::Pretty: display command being run #304

merged 2 commits into from
Dec 18, 2015

Conversation

steved
Copy link
Contributor

@steved steved commented Dec 9, 2015

It seemed odd to see the /usr/bin/env prefix when that wasn't the command being run.

@robd
Copy link
Contributor

robd commented Dec 10, 2015

I just wanted to make sure I understand exactly what the problem here is. Eye balling the code, it looks like this bug is that commands which don't use the command mapper (ie ones with spaces in), are incorrectly logged as if the command mapper had been applied. For commands which do use the command mapper, then the log line is correct. Have I understood this right?

One thing I wanted to check, for a command with a lot of options applied, would this change make the log line very long (ie commands which are wrapped with umask, within, user etc would now print out all of the wrapping details). From the looks of the code, the intention is not to print all this stuff out for commands using the command mapper. I haven't tried this so I'm not sure if I misunderstood something.

@steved
Copy link
Contributor Author

steved commented Dec 10, 2015

Ah, I see what you're saying. Would this be ok? I know it isn't DRY, but the methods are so close anyway:

diff --git a/lib/sshkit/command.rb b/lib/sshkit/command.rb
index d3787d3..cdf9811 100644
--- a/lib/sshkit/command.rb
+++ b/lib/sshkit/command.rb
@@ -206,7 +206,11 @@ module SSHKit
     end

     def to_s
-      [SSHKit.config.command_map[command.to_sym], *Array(args)].join(' ')
+      if should_map?
+        [SSHKit.config.command_map[command.to_sym], *Array(args)].join(' ')
+      else
+        command
+      end
     end

if should_map?
[SSHKit.config.command_map[command.to_sym], *Array(args)].join(' ')
else
command
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs to be command.to_s, because command may be a symbol.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, done

@steved
Copy link
Contributor Author

steved commented Dec 17, 2015

bump

@mattbrictson
Copy link
Member

This looks good, but I haven't had time to think through the consequences. My guess is that Command#to_s is used a lot, either explicitly or within string interpolation. Have you tried doing a cap deploy against this branch?

@steved
Copy link
Contributor Author

steved commented Dec 18, 2015

Yep, looks alright to me. Unfortunately, I'm new to v3 so I'm not 100% sure what the implications are either.

@mattbrictson
Copy link
Member

OK, I've now tested it as well. Seems good to me! 👍

@steved
Copy link
Contributor Author

steved commented Dec 18, 2015

@leehambley

leehambley added a commit that referenced this pull request Dec 18, 2015
Formatters::Pretty: display command being run
@leehambley leehambley merged commit 025392a into capistrano:master Dec 18, 2015
@steved
Copy link
Contributor Author

steved commented Dec 18, 2015

Thanks! Do you mind releasing a new version?

@leehambley
Copy link
Member

Next week proobably
On 18 Dec 2015 7:30 p.m., "Steven Davidovitz" [email protected]
wrote:

Thanks! Do you mind releasing a new version?


Reply to this email directly or view it on GitHub
#304 (comment).

jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Oct 17, 2016
## [1.11.3][] (2016-09-16)

  * Fix known_hosts caching to match on the entire hostlist
    [PR #364](capistrano/sshkit#364) @byroot

## [1.11.2][] (2016-07-29)

### Bug fixes

  * Fixed a crash occurring when `Host@keys` was set to a non-Enumerable.
    @xavierholt [PR #360](capistrano/sshkit#360)

## [1.11.1][] (2016-06-17)

### Bug fixes

  * Fixed a regression in 1.11.0 that would cause
    `ArgumentError: invalid option(s): known_hosts` in some older versions of
    net-ssh. @byroot [#357](capistrano/sshkit#357)

## [1.11.0][] (2016-06-14)

### Bug fixes

  * Fixed colorized output alignment in Logger::Pretty. @xavierholt
    [PR #349](capistrano/sshkit#349)
  * Fixed a bug that prevented nested `with` calls
    [#43](capistrano/sshkit#43)

### Other changes

  * Known hosts lookup optimization is now enabled by default. @byroot

## 1.10.0 (2016-04-22)

  * You can now opt-in to caching of SSH's known_hosts file for a speed boost
    when deploying to a large fleet of servers. Refer to the
    [README](https://github.com/capistrano/sshkit/tree/v1.10.0#known-hosts-caching) for
    details. We plan to turn this on by default in a future version of SSHKit.
    [PR #330](capistrano/sshkit#330) @byroot
  * SSHKit now explicitly closes its pooled SSH connections when Ruby exits;
    this fixes `zlib(finalizer): the stream was freed prematurely` warnings
    [PR #343](capistrano/sshkit#343) @mattbrictson
  * Allow command map entries (`SSHKit::CommandMap#[]`) to be Procs
    [PR #310](capistrano/sshkit#310)
    @mikz

## 1.9.0

**Refer to the 1.9.0.rc1 release notes for a full list of new features, fixes,
and potentially breaking changes since SSHKit 1.8.1.** There are no changes
since 1.9.0.rc1.

## 1.9.0.rc1

### Potentially breaking changes

  * The SSHKit DSL is no longer automatically included when you `require` it.
    **This means you  must now explicitly `include SSHKit::DSL`.**
    See [PR #219](capistrano/sshkit#219) for details.
    @beatrichartz
  * `SSHKit::Backend::Printer#test` now always returns true
    [PR #312](capistrano/sshkit#312) @mikz

### New features

  * `SSHKit::Formatter::Abstract` now accepts an optional Hash of options
    [PR #308](capistrano/sshkit#308) @mattbrictson
  * Add `SSHKit::Backend.current` so that Capistrano plugin authors can refactor
    helper methods and still have easy access to the currently-executing Backend
    without having to use global variables.
  * Add `SSHKit.config.default_runner` options that allows to override default command runner.
    This option also accepts a name of the custom runner class.
  * The ConnectionPool has been rewritten in this release to be more efficient
    and have a cleaner internal API. You can still completely disable the pool
    by setting `SSHKit::Backend::Netssh.pool.idle_timeout = 0`.
    @mattbrictson @byroot [PR #328](capistrano/sshkit#328)

### Bug fixes

  * make sure working directory for commands is properly cleared after `within` blocks
    [PR #307](capistrano/sshkit#307)
    @steved
  * display more accurate string for commands with spaces being output in `Formatter::Pretty`
    [PR #304](capistrano/sshkit#304)
    @steved
    [PR #319](capistrano/sshkit#319) @mattbrictson
  * Fix a race condition experienced in JRuby that could cause multi-server
    deploys to fail. [PR #322](capistrano/sshkit#322)
    @mattbrictson
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.

4 participants