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

configure command execution shells to sh/bash/zsh #659

Merged
merged 1 commit into from
Apr 18, 2016
Merged

Conversation

arlimus
Copy link
Contributor

@arlimus arlimus commented Apr 18, 2016

Run against docker container:

command('n=($(echo "123")); echo $n').stdout
=> ''

Now like this:

command('n=($(echo "123")); echo $n', shell: 'bash').stdout
=> '123\n'

Alternative idea for #645
All kudos to @alexpop for raising + original implementation + @srenatus for his input.

@alexpop alexpop merged commit 3ef64af into master Apr 18, 2016
@arlimus arlimus deleted the dr/command-shells branch April 18, 2016 10:16
@arlimus
Copy link
Contributor Author

arlimus commented Apr 18, 2016

(Moved from #645 )
Great discussion!

This implementation supports a few shells and mainly works via a wrapper; Easy to extend if we wanted to add shell-paths as suggested by @srenatus (though I would separate shell-type and shell-path in that case ;) ):

command('echo hello', shell: 'bash', path: '/bin/bash')

sh = ->(cmd) { 'bash -c ' + Shellwords.escape(x) }
command('echo hello', wrapper: sh)

resource

An alternative idea to all this would be to add a bash(...) resource for calling this one command.

bash('echo world', path: '/bin/bash') ...

Though i think it gets us into the domain of writing scripts, not just commands...

wrapper

Another idea is to initialize the shell once:

bash = command.shell('bash')
describe bash(...) ...

bash = command.shell('bash', path: '/bin/bash/or/somewhere/else')

bash = command.wrapper { |cmd| 'bash -c ' + Shellwords.escape(cmd) }

@arlimus
Copy link
Contributor Author

arlimus commented Apr 18, 2016

@chris-rock ping to our discussion this morning ^^

@chris-rock
Copy link
Contributor

I would like to harmonize usability with our service resource. Therefore we enable all the options to the command resource (wrapper idea), but combine it with the bash resource.

class Bash < Cmd
    name 'bash'

    def initialize(cmd, opts = {})
     opts['shell'] = opts['shell'] || 'bash'
     opts['path'] = opts['path'] || '/bin/bash'
     super(cmd, opts)
    end
  end
end

I am not sure if we should move so many unix/linux handling into cmd implementation.I would like to keep the command as simple as possible and move all the specific os handling to another resource (mabe unix/linux shell). Maybe we could establish the general wrapper logic, and leave the specific parameters to the inherited resources.

@arlimus
Copy link
Contributor Author

arlimus commented Apr 18, 2016

Apparently bash -c is enough to run streamed scripts. Now this doesn't read sha-bang calls to other interpreters or call options, which is a limitation from expectations...

@srenatus
Copy link
Contributor

draft-level nitpicking ;)

  def initialize(cmd, opts = {})
     opts['shell'] ||= 'bash'
     opts['path'] ||= '/bin/bash'
     super # same params are passed by default
  end

@arlimus
Copy link
Contributor Author

arlimus commented Apr 18, 2016

@chris-rock 👍 on that. In this case, bash and others would become the norm. Maybe command_wrapper or just without exposing the abstract resource and instead only implementing bash/...? This way users can always write their own wrappers:

bash = ->(x) { command("bash -c "+Shellwords.escape(x)) }

I really like your idea of moving that logic outside of the command resource.

@chris-rock
Copy link
Contributor

chris-rock commented Apr 18, 2016

@arlimus We could use unix_command and use the logic as you proposed.

@arlimus
Copy link
Contributor Author

arlimus commented Apr 18, 2016

Wouldn't that limit us? I.e. what if there are non-unix systems which we just wanted to wrap?
Would we still go with bash resources in that case?

unix_command('...', shell: 'bash')
# or
bash('...')

@chris-rock
Copy link
Contributor

Oh yeah, the wrapper logic should be in cmd. As long as we get the shell definition out of the core command implementation, I am fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Enhancement Improves an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants