Fixes incorrect host for accessory logs follow with specified host

[matthewbjones]

Bug

Prior to this fix, if you ran the command: kamal accessory logs my-accessory -h server2.hostname.com -f

It would output:

INFO Following logs on ["server2.hostname.com"]...
INFO ssh -t [email protected] -p 22 'docker logs my-app-my-accessory --timestamps --tail 10 --follow 2>&1'

Notice it says it will follow logs on ["server2.hostname.com"] but the ssh command is using the incorrect host ssh -t [email protected].

Solution

Now if you run the same command, it will now use the correct host:

INFO Following logs on server2.hostname.com...
INFO ssh -t [email protected] -p 22 'docker logs my-app-my-accessory --timestamps --tail 10 --follow 2>&1'

[djmb]

Hmm interesting. I think that if -h server2.hostname.com is set, then only that host should show up in the hosts config so this change shouldn't be necessary. We'll need to track down why that's not the case.

[matthewbjones]

My understanding is that although the CLI correctly filters hosts, the Kamal::Commands::Accessory object's hosts method delegates to accessory_config.hosts which is returning the unfiltered configuration hosts. When the run_over_ssh method is run, it's getting the first host from hosts.first which is the non-filtered configuration host(s)

[matthewbjones]

I think the reason why this isn't a more universal problem is that the CLI does correctly filter hosts, but for most Kamal commands the CLI part of the codebase is the one that does the execution/SSH, but when it comes to Accessory, the CLI no longer runs the SSH commands and leaves it to the Accessory command to do it itself, and when the CLI makes the Accessory it does not provide the filtered hosts, so the Accessory always delegates to configured hosts.

I've poked around a bit at it again and I don't really see a path that doesn't involve changing the CLI to somehow specify the host(s), either by passing it into an Accessory's method like follow_logs, or, changing the way the CLI creates the initial Accessory, by modifying Kamal::Commander's accessory(name) to take in an optional host(s). The later may be preferred, as being smarter about how Accessory gets it hosts (not via delegation) would help fix the "root issue"

It looks like Kamal::Commander already has a concept of host/hosts, for example app():

def app(role: nil, host: nil)
  Kamal::Commands::App.new(config, role: role, host: host)
end

def accessory(name)
  Kamal::Commands::Accessory.new(config, name: name)
end

If this were to be improved to something like:

def app(role: nil, host: nil)
  Kamal::Commands::App.new(config, role: role, host: host)
end

def accessory(name, hosts: nil)
  Kamal::Commands::Accessory.new(config, name: name, hosts: hosts)
end

Then the Accessory now is instantiated with optional explicit hosts, and can remove the delegate :hosts, to: :accessory_config and can instead in initialize do something like @hosts = hosts || accessory_config.hosts

If you would like, I can update the PR to go more in this direction and try to align Accessory to be closer to how App handles things.

[matthewbjones]

@djmb Just wanted to give a polite nudge to see what your latest thoughts are. I'm more than willing to revise the PR to reflect my recent comment about updating the Kamal::Commander to pass in hosts when instantiating an Accessory in a similar way that it already passes in host for App.

[djmb]

I need to do some work on filtering the hosts/accessory hosts correctly so that this isn't necessary

[matthewbjones]

Okay great thanks. If I can be helpful at all just let me know.