`test()` in `within(dir)` should run after cd into dir

[teohm]

Given a test() in a within(),

within('/home/vagrant/app1/current') do
  if test '[ -e tmp/pids/server.pid ]'
     execute :kill, '-INT', 'tmp/pids/server.pid'
  end
end

Does it make sense to generate the following commands?

cd /home/vagrant/app1/current && [ -e tmp/pids/server.pid ]
cd /home/vagrant/app1/current && (/usr/bin/env kill -INT tmp/pids/server.pid)

However, currently it doesn't cd into the given directory when generate test() command:

[ -e tmp/pids/server.pid ]
cd /home/vagrant/app1/current && (/usr/bin/env kill -INT tmp/pids/server.pid)

[leehambley]

Because the test contains a space https://github.com/leehambley/sshkit/blob/master/EXAMPLES.md#run-a-command-without-it-being-command-mapped

[teohm]

Ah, it's documented in EXAMPLES. Yes, I notice that as well after debugging.

But as a user, I feel a bit surprised when my test() is not executed within the given dir.

Anyway, does it make sense if I write test() without space?

within current_path do
  if test '[', '-e', 'tmp/pids/server.pid', ']'
    ...
  end
end

On Tue, Jul 16, 2013 at 5:12 PM, Lee Hambley [email protected]:

Because the test contains a space

https://github.com/leehambley/sshkit/blob/master/EXAMPLES.md#run-a-command-without-it-being-command-mapped

— Reply to this email directly or view it on GitHubhttps://github.com/leehambley/sshkit/issues/23#issuecomment-21030044 .

Huiming

[leehambley]

For test() the use-case is a bit unusual, granted… I didn't come across any cases where it felt too bad, but if you have a better idea than "if it contains a space" i'd love an alternative. As test() and execute() basically end up calling _execute() internally, with different variadic args, it wouldn't be too tricky to add a :use_command_map flag which could be set to true, or false.

In previous versions, I had a hard time to decide if the decision character should be a space, or a newline, as both made sense in different circumstances.

Thoughts?

Lee Hambley

http://lee.hambley.name/ +49 (0) 170 298 5667

On 16 July 2013 11:52, Huiming Teo [email protected] wrote:

Ah, it's documented in EXAMPLES. Yes, I notice that as well after debugging.

But as a user, I feel a bit surprised when my test() is not executed within the given dir.

Anyway, does it make sense if I write test() without space?

within current_path do
if test '[', '-e', 'tmp/pids/server.pid', ']'
...
end
end

On Tue, Jul 16, 2013 at 5:12 PM, Lee Hambley [email protected]:

Because the test contains a space

https://github.com/leehambley/sshkit/blob/master/EXAMPLES.md#run-a-command-without-it-being-command-mapped

— Reply to this email directly or view it on GitHub< https://github.com/leehambley/sshkit/issues/23#issuecomment-21030044> .

Huiming

— Reply to this email directly or view it on GitHubhttps://github.com/leehambley/sshkit/issues/23#issuecomment-21031821 .

[leehambley]

Also, the trailing ] is optional according to test (man 1 test), which is aliased as [, just for style reasons.

Lee Hambley

http://lee.hambley.name/ +49 (0) 170 298 5667

On 16 July 2013 13:18, Lee Hambley [email protected] wrote:

For test() the use-case is a bit unusual, granted… I didn't come across any cases where it felt too bad, but if you have a better idea than "if it contains a space" i'd love an alternative. As test() and execute() basically end up calling _execute() internally, with different variadic args, it wouldn't be too tricky to add a :use_command_map flag which could be set to true, or false.

In previous versions, I had a hard time to decide if the decision character should be a space, or a newline, as both made sense in different circumstances.

Thoughts?

Lee Hambley

http://lee.hambley.name/ +49 (0) 170 298 5667

On 16 July 2013 11:52, Huiming Teo [email protected] wrote:

Ah, it's documented in EXAMPLES. Yes, I notice that as well after debugging.

But as a user, I feel a bit surprised when my test() is not executed within the given dir.

Anyway, does it make sense if I write test() without space?

within current_path do
if test '[', '-e', 'tmp/pids/server.pid', ']'
...
end
end

On Tue, Jul 16, 2013 at 5:12 PM, Lee Hambley [email protected]:

Because the test contains a space

https://github.com/leehambley/sshkit/blob/master/EXAMPLES.md#run-a-command-without-it-being-command-mapped

— Reply to this email directly or view it on GitHub< https://github.com/leehambley/sshkit/issues/23#issuecomment-21030044> .

Huiming

— Reply to this email directly or view it on GitHubhttps://github.com/leehambley/sshkit/issues/23#issuecomment-21031821 .

[teohm]

I'm trying to pick up SSHKit internals :) so please correct me if I misunderstand. There are 2 questions:

1) Using command_map

First, both test() and execute() execute the string returned from Command#to_command.

module Backend
   class Netssh < Printer

      def _execute(*args)
        command(*args).tap do |cmd|
          ...
          ssh.open_channel do |chan|
            ...
            chan.exec cmd.to_command do |ch, success|

Second, command_map is used in Command#to_s. And in Command#to_command, #to_s is called whether or not the command has a space.

In other words, command_map is always used, even when the command has a space. Is this intended?

module SSHKit
  class Command

    def to_command
      return command.to_s unless should_map?
      within do
        umask do
          with do
            user do
              in_background do
                group do
                  to_s
                end
              end
            end
          end
        end
      end
    end

    def to_s
      [SSHKit.config.command_map[command.to_sym], *Array(args)].join(' ')
    end

2) Expectation of within(), with(), as()

My original question is actually not about command_map, it's about within().

When execute a command in a within(dir) block, I would expect the command is executed within the given directory. But now, the command does not run in the given directory, just because it has space(s)? Is this an expected behavior, I don't quite understand the rationale.

within('/home/app1/current') do
   execute "bundle exec rails about"  # expect to run in /home/app1/current dir
end

[leehambley]

1. Using command_map

Sounds like a bug to me, but we need a way to say "don't mess with my command, I know what I'm doing" The use case being, for example

execute <<-EOCOMMAND
... some bash script here...
EOCOMMAND

2. Expectation of within(), with(), as()

These things can only work if the command passed to execute() can be mapped, and deformed (which would break things HORRIBLY if a heredoc, and a 5 line bash script is passed). The "don't mutate my command" option is important, but it's probably edge case enough that we could add a flag to execute() (or _execute()) to say "don't deform" (don't map.) this.

[teohm]

Ah, I begin to understand the use case. I think I can live with the existing API for now :)

Have you thought about making it explicit in the API? Reminds me of the raw() function in Rails view:

test raw("[ -e /path/to/file ]")

execute raw("echo 'dont mutate my command'")

execute raw(<<-EOCOMMAND
... some bash script here ...
EOCOMMAND
)
``

[leehambley]

raw() might make some sense - at least less magic was one of the design goals

Lee Hambley

http://lee.hambley.name/ +49 (0) 170 298 5667

On 17 July 2013 01:11, Huiming Teo [email protected] wrote:

Ah, I begin to understand the use case. I think I can live with the existing API for now :)

Have you thought about making it explicit in the API? Reminds me of the raw() function in Rails view:

test raw("[ -e /path/to/file ]")

execute raw("echo 'dont mutate my command'")

execute raw(<<-EOCOMMAND ... some bash script here ... EOCOMMAND ) ``

— Reply to this email directly or view it on GitHubhttps://github.com/leehambley/sshkit/issues/23#issuecomment-21081086 .

[thedeeno]

+1 for raw().

I've been implementing some tools with sshkit over the past few days and I think the magic args for _execute has been the most surprising and confusing issue so far. It's not obvious that commands don't get augmented with (as, with, within, etc) when they contain spaces. Not obvious is probably putting it nicely actually. I expected to be able enter remote commands exactly like I would enter them locally.

If we need to override the execution context I think .._raw methods would clearly indicate that we're doing something special.

execute_raw "..."

What's the use-case for having Command options mixed in with the text?

[leehambley]

If we need to override the execution context I think .._raw methods would clearly indicate that we're doing something special. execute_raw "..."

I specifically don't want that, since you and I probably have different ideas on what that would do. (I would assume raw performs no changes at all to the command, especially not prepending, and prefixing) I'm also firmly against widening the public API, I could understand an additional option to execute(), and of course, if it's a problem you keep running into, then of course, take care of your own shell escaping, etc, and write a small wrapper that you can inject into SSHKit, when it matures, and you feel that it's ready to be more widely used, throw me a PR with some docs/notes, and I'll be happy to merge it.

What's the use-case for having Command options mixed in with the text?

Sorry, I don't really understand this question, can you rephrase it?

[thedeeno]

I agree with limiting the public API. I'm just in agreement with @teohm in that the current API has some surprising side-effects. Perhaps this is a bug, perhaps I'm missing something, but here's an example of what I'm talking about:

This works as expected:

## Example 1
on hosts do |host|
  with debian_frontend: :noninteractive do
    as :provisor do
      execute "echo", "'hello world'"
    end
  end
end

# actual & expected
#=> Cmd: ( DEBIAN_FRONTEND=noninteractive sudo su provisor -c "/usr/bin/env echo 'hello world'" )

This is surprising:

## Example 2
on hosts do |host|
  with debian_frontend: :noninteractive do
    as :provisor do
      execute "echo 'hello world'"
    end
  end
end

# actual
#=> Cmd: echo 'hello world'
# expected
#=> Cmd: ( DEBIAN_FRONTEND=noninteractive sudo su provisor -c "/usr/bin/env echo 'hello world'" )

This is surprising:

## Example 3
on hosts do |host|
  with debian_frontend: :noninteractive do
    as :provisor do
      execute <<-EOC
echo "hello world"
      EOC
    end
  end
end

# actual
#=> Cmd: echo "hello world"
# expected
#=> Cmd: ( DEBIAN_FRONTEND=noninteractive sudo su provisor -c "/usr/bin/env echo 'hello world'" )

To me, it seems magical that passing a string containing spaces stops 'command augmentation'. At first it was hard to figure out what was going on.

It seems if somone wanted to ignore the higher up as, with directives it should be explicit. Like:

## Example 4 (theoretical)
on hosts do |host|
  with debian_frontend: :noninteractive do
    as :provisor do
      execute_raw "echo 'hello world'"
    end
  end
end

# actual & expected
#=> Cmd: echo "hello world"

To me a method like execute_raw makes sense for the API. It reads that we're explicitly ignoring the 'augmentation context'.

Maybe there's another solution or perhaps I'm missing a problem. This is why I was wondering what the use-case is for the behavior exhibited in example 2 and example 3.

[leehambley]

That's way too much code to parse. I won't add an excute_raw because raw says to me "don't do ANYTHING with this command", which as far as I understand this issue is the exact opposite of what you want it to do (you are saying "I trust myself to do shell escaping").

I can't see a way to make everyone happy, so I have to put my foot down, Capistrano is our main consumer, and for that use case, this decision makes perfect sense (well, relatively speaking).

As I wrote earlier there's no way to do what everyone expects all the time, so I just have to decide for A or B, I could flip a coin, and piss as many people off one way or another, so I guess I won't change this stuff unless a compelling pull request with a decent sane fallback comes along.

That would mean, either raising an exception when an unclear command is given to execute, which isn't a behaviour I'd like to have, or complex, and fragile handling of scanning for markers that this might be a complex query.

We're bordering on the territory of having to implement something like the Rails html_safe concept on strings, only with shell safe, in order to have an API for people to mark a string as "I trust myself", so that we know that you know what you are doing isn't normal, and that we let you do it anyway.

I see your point, that actually, there's no way to use execute() with within() if you have a complex command, I'd compel you _not_ to put anything more than script calls in Capistrano, and simply execute 'my_script.sh' and put your complex, delicate shell logic in there, in an environment where it can be tested, and useful outside of Capistrano.

[thedeeno]

Sorry for the extensive code. I thought it may help since it seemed like we weren't on the same page.

Anyways, I don't think this just a matter of taste.

To me it looks like a broken API. Fact: strings with spaces are treated different than strings without them for the first parameter to execute. That's pretty obscure and, as you point out, pushes people to create stand alone scripts for even simple 2 word commands.

If we establish a context with as that context should be honored. upload! currently doesn't honor contexts and you call that out in the examples as something that should eventually be supported.

I guess I'm a little confused, forget the raw api, why can't the following work? Is there a limitation I don't know about?

on hosts do |host|
  as :provisor do
    execute "echo 'hello' "
  end
end
#=> Cmd: ( sudo su provisor -c "/usr/bin/env echo 'hello' " )

[thedeeno]

Granted this can be worked around by doing the following, it just seems unnecessary:

execute "echo", "'hello'" 

[trvsdnn]

This just got me too – it doesn't work how you'd expect it to. You naturally want to write executes how you would write exec, system, %x[], etc. And all of the within, with, etc sugar is less usable in the current state. It's not an easy problem to solve, but it doesn't seem like it needs to be solved entirely. Unless I'm missing something the shell escaping is only an issue when using user and group – handle those the way they are currently, and the others without the mapping?

I might take a look at SSHKit::Command and see if I can make anything of it.