RFC: Update API to enable connection re-use, allow streaming…

[pmeinhardt]

Related to: #41

Description

Update the top-level SSHKit API in order to reuse SSH connections across SSHKit operations, i.e. SSHKit.run/3, SSHKit.upload/4 and SSHKit.download/4.

The updated API could look as follows (feedback welcome):

hosts = ["1.eg.io", {"2.eg.io", port: 2222}]

# UPDATED: The context no longer receives a list of hosts/a single host.
# It exclusively manages the execution context on the remotes.
context =
  SSHKit.context()
  |> SSHKit.path("/var/www/phx")
  |> SSHKit.user("deploy")
  |> SSHKit.group("deploy")
  |> SSHKit.umask("022")
  |> SSHKit.env(%{"NODE_ENV" => "production"})

# NEW: Explicitly establish a connection
{:ok, conn} = SSHKit.connect(hosts)

# UPDATED: The top-level functions now require a conn struct.
{:ok, conn} = SSHKit.upload(conn, context, ".", recursive: true)
{:ok, conn} = SSHKit.run(conn, context, "yarn install")

# IDEA: Passing a context could be optional (different possible versions)
{:ok, conn} = SSHKit.run(conn, "apt-get install -y postgresql", context: context)
{:ok, conn} = conn |> SSHKit.within(context) |> SSHKit.run(conn, "apt-get install -y postgresql")

# NEW: Explicitly close the connection once we're done
{:ok, _} = SSHKit.close(conn)

cf. current README.md

Additional considerations

A major idea/API change that may play into this draft is how we handle stdout/stderr streaming which is an interesting case we did not handle in SSHKit 0.x but is definitely interesting to give quicker user feedback, e.g. on deployment cases as with bootleg (see the alt. capture implementation).

This might solve/play into #53 as well.

Make context optional?

With the API update, context could be made an optional or keyword argument, because for some things you want to run, you might not need any user/group/path/env/umask changes.

Update host struct

There was a request to include host information in result tuples #132 and Bootleg defines a %Host{} struct to assign a name, we should consider adding name and possibly tags to hosts.

Motivation and Context

At the moment, SSHKit is establishing a new connection for every command. The outlined API changes could allow us to reuse established connections for multiple consecutive operations and improve the speed with which server automation tasks can be carried out. Deployments with bootleg (cc @labzero) for instance could benefit from this.

Discussion: Alternative Designs

As #41 outlines, we could also use a (GenServer) process to cache connections. This however means that it would be harder/not possible to - for instance - open multiple connections to the same server.

It also seems more explicit and conceptually cleaner to separate execution context (SSHKit.context/0) from connection information (the new conn struct).

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have updated the documentation accordingly.
• I have added tests to cover my changes (with unit and/or functional tests).
• I have added a note to CHANGELOG.md if necessary (in the ## master section).

[pmeinhardt]

@brienw, @holetse, @rjanja, @svoynow here's a quick draft on how we could enable connection reuse with SSHKit.ex. Since this could possibly speed up deployments, I thought you might be interested 🙂

We haven't updated the package in a while and now with some distance I have a few new ideas.

I'd be happy about your thoughts on the API design. It adds another parameter to the main functions, but at the same time cleanly separates (execution) context from connection information.

[klappradla]

Ace @pmeinhardt 🙇

I vaguely remember we discussed this topic a while ago... 🤔 In my personal opinion, this makes the API way cleaner and a lot more "natural" to use (see my comment about upload, run and probably also download...)

[pmeinhardt]

Thanks a lot for the great round today @klappradla, @lwassermann and @andreasknoepfle ❤️ ❤️

Here's a quick braindump:

• Separating conn and context as outlined above looked like a clear and understandable option, even if it changes function signature and makes calls longer
• We discussed chaining/piping in the top-level API
  • piping in combination with {:ok, conn} and {:error, …} tuples didn't seem like the best option so far
  • with/1 looked like a more idiomatic fit
  • having more "flow" examples (in the readme) might be nice
• Looking at typical use-cases like labzero/bootleg we found that we need to address stdout/stderr streaming
  • there's at least two options: hook functions and messages to the invoking process + receive
  • hooks may have a small drawback if you need to aggregate output but could potentially be easier to use in the top-level API
  • receive appears a bit more flexible (either plain hooking or aggregation) and (maybe) more idiomatic?

I've updated the PR description as well to integrate some of the results.

Cheers 👋 and again much ❤️

[pmeinhardt]

As an additional note: mint uses a message-based API that we can draw inspiration from:

{:ok, conn} = Mint.HTTP.connect(:http, "httpbin.org", 80)
{:ok, conn, request_ref} = Mint.HTTP.request(conn, "GET", "/", [], "")

receive do
  message ->
    {:ok, conn, responses} = Mint.HTTP.stream(conn, message)
    IO.inspect responses
end

I think this could work very similarly for SSH connect, run, stream…

[pmeinhardt]

Closing in favor of #164 😊