Skip to content

Conversation

@rjanja
Copy link

@rjanja rjanja commented May 10, 2017

When running a command on multiple machines, the output from that command is not currently associated with the remote connections. This PR attempts to change that by creating a UUID for each host on context creation, and passing that UUID around as needed.

We also have use for an output formatter but I believe this belongs more in SSHKit than individual apps. This PR includes two basic formatters, SSHKit.Formatters.PrettyFormatter and SSHKit.Formatters.SilentFormatter and defaults to the latter to remain as quiet as it is today. The formatter can be specified in calls to SSHKit.run and SSHKit.SSH.run. I didn't want to go too hog wild with implementing it elsewhere before seeking your feedback.

Thoughts?

Thanks!


  • Documented the change if necessary
  • Tested this PRs change (with unit and/or functional tests)
  • Added this PRs change to CHANGELOG.md (in the #master section) if necessary.

@pmeinhardt
Copy link
Contributor

Hi @rjanja, thanks a lot for opening a PR 👌

@tessi and I discussed the matter of associating hosts and command output a bit last week.

Here's my (personal, debatable) point of view (I'm leaning towards keeping this package rather minimal):

  • I would like SSHKit itself not to output anything to :stdout/:stderr.

    Since it's intended as a base layer for building all kinds of applications, output should probably be handled by those. Or, from another point of view: If I'm embedding a package to perform SSH operations, I may not want it to just print to the console. I'd want it to just handle the SSH part and leave it up to the application to print or not print results.

    At the same time, the notion of having callbacks for various events (connect, exec, receive…) could be useful?

  • Associating hosts and command execution results, could be as simple as an Enum.zip/2.

    The list of result tuples results = SSHKit.run(context, command) is guaranteed to be in the same order as the context.hosts. Therefore, if you wish to have host information along with the command result, you can simply do Enum.zip(results, context.hosts). No need for extra options and more code?

    Nonetheless, I see that a way of annotating hosts with custom information is useful. E.g. to attach a certain "role" to a host (like "web", "db", "load-balancer"…) or even an application-specific identifier.

    How about adding :tags/:meta/… (have to think about a name) to host structs, which can be freely assigned by the application using SSHKit? This way, you could assign roles, UUIDs, … or other host-related meta information needed by your application.

Like I said, this is just laying out my thoughts/internal roadmap at the moment. Feedback very much welcome.

@rjanja
Copy link
Author

rjanja commented May 10, 2017

Hi @pmeinhardt, thank you for the thoughtful response. I can appreciate your wanting to keep this package minimal. Submitting such an invasive PR wasn't my first choice, but I felt the output might be desirable in order to maintain some parity with sshkit and to make SSHKit.ex more useful out of the box. Perhaps that could be done with an SSHKit-output-like addon package, though.

Regarding SSHKit not outputting anything itself, I think the notion of callbacks would be necessary and not just useful. Consider an application where it's desirable to get the output of a long-running command on a remote machine. In its current implementation, there is no output until the command has finished executing or the communication channel has been closed, I believe. Although the current implementation allows for the setting of accumulator and capturing functions to be used, circumventing these is akin to reimplementing parts of the context-aware functions in SSHKit. (As it stands today, we will be doing more of this.)

For specifying callbacks, I would be interested in hooking into the most of the operations here: connections, uploads, downloads, and commands. I'm not sure what this would look like, but I'll take a peek around and see if I can find how other libraries are doing it.

I do like your Enum.zip/2 idea for the returning of remote command execution results, and the labeling of hosts with meta information.

I would also like to see %Host{} and %Context{} structs being passed around as strict arguments where practical, as this would help inform end users like myself of the lines being drawn between convenience, multi-server functions and lower-level single-connection methods.

Thanks for all your hard work on this package! I'm happy to chip in where I can and where it makes sense to do so. Cheers!

@rjanja rjanja closed this May 10, 2017
@brienw brienw deleted the rjanja/host-uuid-formatters branch October 2, 2017 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants