Bug: within(directory){ execute(string) } should either 'just work' or 'boom'

[ahoward]

docs aren't good enough to explain the design choice IMHO - just check stackoverflow...

one solution is just to fix it - use shellwords to properly escape the command

  require 'shellwords'

  def execute(*args, &block)
    if args.first.is_a?(String)
      command = Shellwords.escape(args.first)
    end

    # ...
  end

otherwise raise an exception

  def execute(*args, &block)
    if args.first.is_a?(String) and inside_within?
      raise "don't do that"
    end

    # ...
  end

the current behavior of doing

  within directory do  # silently ignored
    execute command
  end

just isn't POLS - the point of a library like cap is to be able to re-use code but, currently, each and every use must re-invent 'cd into a (properly escaped directory) and run commands', including handling the fact that

  within(does_not_exist) do # raises
  end

  execute "#{ does_not_exist }; command.sh" # reports a failed exit status that leads to debugging which part failed

a final solution would be to remove the 'within' API since it sometimes works, and sometimes does not, issuing no exception nor warning

[ahoward]

just also noticing that the code absolute string bashes user input already:

    def sanitize_command!
      command.to_s.strip!
      if command.to_s.match("\n")
        @command = String.new.tap do |cs|
          command.to_s.lines.each do |line|
            cs << line.strip
            cs << '; ' unless line == command.to_s.lines.to_a.last
          end
        end
      end
    end

so extending this to 'do the right thing' is absolutely in line with the current design

[leehambley]

docs aren't good enough to explain the design choice IMHO - just check stackoverflow...

Alright, that's a problem. Here's the thing, we need a list of commands that people try and do this thing with, to have some concrete arguments. The issue, briefly explained is that the command mapping (prefixing, suffixing, wrapping in sudo, etc) struggles (I dare say it's impossible) to do the right thing.

For the simplish cases of someone having a string such as ./configure --i=know what i'm doing that should work, but it's not only about shell escaping what they give in a way that we don't trash it, but that we can confidently and correct wrap all the possible myriad of options that the API gives. If we special-case within, how about as and with and all the other API methods? At some point it breaks down.

[leehambley]

What cases can you offer up as sane use-cases for strings in commands, and can we take those and see if we can make the whole API work reliably?

[ahoward]

there aren't sane use cases for either strings or arrays as *args as command arguments if you ask me - both are arbitrary. however, the code has a bunch of inconsistent string bashing in it already, eg an env with a " will 'just work'

    def environment_string
      environment_hash.collect do |key,value|
        key_string = key.is_a?(Symbol) ? key.to_s.upcase : key.to_s
        escaped_value = value.to_s.gsub(/"/, '\"')
        %{#{key_string}="#{escaped_value}"}
      end.join(' ')
    end

but a directory with with a space in it will boom

    def within(&_block)
      return yield unless options[:in]
      sprintf("cd #{options[:in]} && %s", yield)
    end

so it seems like backing up to 10,000 feet makes sense - is the goal of the library to construct correct commands with a consistent api then within, as, and with all need handled. if the goal is, as you have suggested in some other threads, to let the user deal with that, then the meta apis that manage env, working directory, etc should be removed and the interface consist only of strings the user manages so handling things like

directory = '/User Information'

execute "cd '#{ directory }'; echo 'it works'"

is 100% transparent

i'm just arguing for the goal to be 100% consistency and 100% either raw commands or _managedcommands - but not sometimes one or the other such that the clients of the libraries know which to expect at all times

[ahoward]

for a sane use case, here are two from only the first cap3 deployment i've done

sometimes you are going to run things that don't have a clean mapping from symbol to string

fbomb = "./script/fbomb"

execute "cd #{ current_path } && #{ fbomb } daemon stop; true"

a few things to note about this:

• execute("./script/fbomb".to_sym) is just fugly
• sometimes you want to chain something like "; true" on for commands

another example:

execute 'yes | some_poorly_designed_script'

which highlights your point about proper escaping ;-/

in any case the code to append the cwd onto within/as etc is, IMHO, just as complex and POLS as arbitrarily splitting strings with newlines and joining them on ';' - a hack, but one that is what the user wants 80/20

[leehambley]

Alright, so thanks for those suggestions, better than I've come across, whilst I could shoot them all down with "you should be doing X" (and, maybe that's the resolution we come up iwht, and we improve the docs for this a LOT), let's see if we can make them work.

For posterity:

$ yes | some_poorly_designed_script
$ cd #{ current_path } && ./script/fbomb daemon stop; true"

I'm sure we can build up a list. I'd like to look into some realistic scenarios, something like, how they should/would look inside contrived, if still realistic setups :

as "postges" do
  within "somedir" do
    with rails_env: :development do
      execute $somethingProblematic
    end
  end
end

I'm not 100% current on the implementation of as - it's undergone some changes recently, but given that user input, what would we expect?

Regarding the ;true hack, by the way, we've all been there, but there's a way to make that not fail in SSHKit, which is (apparently not documented ahem) which test() (analog of capture() and execute() uses as an option to not raise if the command exits non-zero.

The idea was to give a Ruby DSL so that people could stop shell scripting, let's see if we can grow a list (community is included) to come up with a comprehensive list of things uncovered by the API, and we can work towards a solution.

[ahoward]

i'm going to pin this tab to i keep track of this and have to crank on some work now but i want to drop a few notes in here for myself:

all commands have

• an env
• stdin
• stdout
• stderr
• a cwd
• a program
• moar

i wonder if generating a script per command to be run would be better than jamming it all into a bash/sh cli

[leehambley]

i wonder if generating a script per command to be run would be better than jamming it all into a bash/sh cli

Sounds like a rather large departure from what we've done to date, and what little uploading of scripts we have done has always run into problems with tempfiles, collisions of tempfile names, noexec on the upload directory (usually /tmp, so fair enough), problems with which shell actually to use, and then all the problems that come along with bash, login shells, dotfiles, etc. But don't let me stop your train of thought in that regard.

I actually looked a level deeper, which would be to send things like env vars (for example) on the ssh channel to setup the environment for the connection (they can be changed any time).

Another option would be to have Net-SSH give us a shell in a directory with the correct user, before trying to wrap the user's command, it would complicate pooling ("I need a shell as this user in this directory with these env on this host" becomes the query, not just "I need a shell on this host") - but these could also be "channels" on the main "connection", so the channels (lightweight, very fast to setup/teardown) could be the instrument for changing directory, parameters, etc... I'd really have to look into how persistent those channel parameters are... but food for thought

[ahoward]

yeah i hear you. i'll look a bit but my thoughts would be to generate all scripts locally and send them over stdin on the channel...

[ChristianVermeulen]

Man, this is the closest i've come to my bug.. For some weird reason I keep getting:

NoMethodError: undefined method `within' for main:Object

I am no ruby developer so I'm really working this with a lot of sample code and tutorials. But there is barely anything on the interwebs about writing customs tasks in rake files.

This is my rake file:

namespace :fileservice do
    task :migrate do
        within release_path do
            info 'Doing migrations'

            execute :php, fetch(:symfony_console_path), 'doctrine:migrations:migrate', '--no-interaction', fetch(:symfony_console_flags)
        end
    end
end

Does anybody know what is going on?

[mkreyman]

@ChristianVermeulen, I think the problem is that you need to tell it what host or what roles you want to run it on. Or to specify that you want it run locally with run_locally do...

namespace :fileservice do
  task :migrate do
    on roles(:db)
      within release_pasth do
...

Take a look at https://github.com/capistrano/sshkit/blob/master/EXAMPLES.md for more examples.

[ChristianVermeulen]

@mkreyman That was indeed the problem. Can't believe it took me so long to find that out!