Issue: SSHKit::Backend::Netssh binding change in run method breaks lookup in 'normal' block scope

[dspaeth-faber]

Hi,

in SSHKit::Backend::Netssh#run instance_exec is used to change the binding of the given block. With this binding change it is difficult to encapsulate an action in a service object for instance you have a cap-task:

  desc 'Setup nginx configuration'
  task :setup do
    Nginx.new(self).setup
  end

  class Nginx < SimpleDelegator
   ...
   def upload_files
        on roles nginx_roles do |host|
          set :host_server_name, host.properties.fetch(:host_server_name)
          template 'assets.host.site', tmp_config_file
       end
  end

  def tmp_config_file
    'remote_file_name'
  end
   ...
 end

This does not work because the block is executed in the context of the SSHKit::Backend::Netssh object context and does not know the method tmp_config_file.

Why do I want to do such things? Testing! Rake and Cap tasks are difficult to test. When I encapsulate the actions I can test the a little.

regards dieter

[leehambley]

This is too big of a change to test. Others work around this by injecting a module into the backend class. Others by passing the execution context into their own object. Testing, is by and large a waste of time, without going full-stack and integration testing things.

If you can refactor the whole gem to work compatibly without instance_exec, please try, we couldn't.

[dspaeth-faber]

There is an easy work around. You can use the block binding as additional parameter for instance_exec.

  def run
        instance_exec(host,@block.binding , &@block)
  end

Testing, is by and large a waste of time, without going full-stack and integration testing things.

I won't start a flame but I have another opinion. Cost / effort must have the right ratio and setting up a vagrant on a ci server or some thing similar to test full stack is realy expensive. Backend::Local won't work because of different initializer signatures.

[dspaeth-faber]

Or maybe some thing like this:

 def method_missing(method_name, *args)
    @block.binding.eval ("#{method_name} #{args.map{|a| a.is_a?(String) ? "'#{a}'": a }.join(',')}")
 end

[dspaeth-faber]

I did create a small quick and dirty gist to demonstrate how the dsl could be changed https://gist.github.com/dspaeth-faber/312019ef3c8ccd1fb1d9 maybe it is of any interest.

I think it would be a good thing to seperate DSL & Commands.

[dspaeth-faber]

@leehambley I did workaround this issue with a custom dsl. It's now available as gem without any promises of any compatibility.

[gstark]

I just ran into this too. I implemented this method inside my class (in your case Nginx) -- I don't like the name, but it works.

def remote_on(*args, &block)
  on(*args) do
    sshkit = self
    block.call(sshkit, host)
  end
end

and you could use it like:

def upload_files
  remote_on some_roles do |sshkit, host|
    sshkit.execute "..."
  end
end

Perhaps a different version of on could be added that works like my remote_on method and works around the instance_exec?

[mattbrictson]

We have not really publicized it yet (so consider its API subject to change), but there is a new Capistrano::Plugin class that might be a good place to experiment with solutions to this problem.

The point of Plugin is to make it possible to write Capistrano tasks that are easier to test and distribute. I ran into the same instance_exec issues there, and I'm interested in a more robust solution. My dream is that something like your Nginx class could be written as class Nginx < Capistrano::Plugin and would inherit behavior and APIs that would fix these problems and encourage testability.

Anyway, perhaps we could introduce remote_on (I agree it needs a better name) as a private helper in Capistrano::Plugin as a way to test the waters in a way that wouldn't disrupt the wider Capistrano end-user community but would be helpful to developers who are creating reusable, testable task libraries.

Thoughts?

For reference, here is the PR that introduced Capistrano::Plugin: https://github.com/capistrano/capistrano/pull/1561

[leehambley]

If I'm reading this correctly the bigger (and recurring) problem is that people are surprised by the internal use of instance_exec and surprised that the variable scope changes. I dare say this comes up often enough that it mandates a rewrite of SSHKit, not special-casing some individual methods that aren't subject to the instance_exec "problem".

[mattbrictson]

My sense is that the majority of Capistrano users are well-served by the current API, so this does not warrant a rewrite. It is advanced users who are writing task libraries where there is friction. Perhaps better documentation would be the best way to address this.

[leehambley]

The issue with instance_exec is one of my most regretted design decisions with SSHKit, as I don't really understand the issue and how it specifically affects library authors, could someone volunteer to write the docs, or we add it as a checklist item to the Capistrano plugin ticket?