Proposal: integrate Airbrussh with next Capistrano release

[mattbrictson]

@leehambley, @robd and I have discussed making Airbrussh the default formatter in the next version of Capistrano. I'd like to use this thread to hammer out the details.

Here's my TODO list so far:

• Add Airbrussh to capistrano.gemspec (see Make Airbrussh the default formatter for Capistrano #1541)
• Change default format from :pretty to :airbrussh (see Make Airbrussh the default formatter for Capistrano #1541)
• Mention Airbrussh in Capistrano's README, so that people wanting to contribute/report bugs about the new default formatter know to go to the Airbrussh repo (see Add Airbrussh to list of related repos in README #1588)
• Move the Airbrussh repo from mattbrictson/airbrussh to capistrano/airbrussh
• Add basic Airbrussh documentation to capistranorb.com

When it gets down to the details, things are a bit more tricky. The first thing on my mind is configuration; I'm sure other issues will come up.

Configuration

Option 1: Maintain the current Airbrussh configuration mechanism.

Airbrussh.configure do |config|
  ...
end

Option 2: Introduce settings in Capistrano's load:defaults that are then passed down to Airbrussh during the boot process (probably inside configure_backend).

# These get passed through to Airbrussh
set_if_empty :log_file, "log/capistrano.log"
set_if_empty :console_color, :auto
set_if_empty :console_truncate, :auto
set_if_empty :display_command_output, false

I'm leaning towards option 2, because I don't think end users should need to know Airbrussh exists. Like SSHKit, it is used behind the scenes, and most people don't need to access it directly. I expect people's mindset will be "how do I change Capistrano's output", not "how do I change Airbrussh's output". Does that sound right?

Edit: I've decided on Option 3:

set :format_options, color: true, command_output:false, ...

Other thoughts?

Any other tasks or consequences of the merge that I haven't covered here? Let me know and I will add to the checklist.

[robd]

Hey @mattbrictson - it's great that you are picking this up again.

I'd like to propose a rather different approach, which is to move the formatter code from the Airbrussh repository directly into the SSHKit repository. My main motivations for this approach are:

1. It removes the need for version management / multiple version support / CI setup between Airbrussh and SSHKit, which has been the cause of quite a bit of the Airbrussh work.
2. Refactoring in SSHKit becomes easier because you don't have to search across multiple projects and run multiple builds
3. It would give us a chance to consolidate the tests and use the SSHKit vagrant setup for proper acceptance tests
4. Some of the features implemented in Airbrussh might be useful in other formatters (eg rake task highlighting)

What do you think about this?

/cc @leehambley

[leehambley]

So I hear you @robd and @mattbrictson and I exchanged a couple of emails before he (very wisely) brought the issue to GH.

I'm in favour of making Airbrussh the default formatter for Capistrano but not for SSHKit, logic being that SSHKit already includes a bunch of formatters, and they are bordering on being too complex already.

Adding another would push me over the edge of wanting them to live in separate gems (to some extent).

If we then frame the question as should Airbrussh be part of Capistrano I think the answer is a no, since it's also useful for people who are unhappy with the SSHKit default formatter.

With regard to 4) I can see that argument, too but honestly I'm not too familiar with the particulars and magic of Airbrussh, I'm quite fond of the standard output (although, admittedly I did design it!)

[leehambley]

Quick note, I'll go with the majority on this, but I wanted to keep SSHKit simple and lean, the most common use-case I see for SSHKit in my own consulting role is within automation, synchroization and coordination of test systems, build clusters, etc etc, also for checking on results of Rake tasks and stuff remotely, in these setups having a small, stable gem is invaluable.

[robd]

@leehambley Understood. I don't have experience of using SSHKit on it's own and I understand about the problem of adding more code to SSHKit. Also agreed Airbrussh shouldn't live in Capistrano. I think keeping the existing SSHKit default formatters and only changing the Cap one makes sense.

My feeling is that if we make Airbrussh 'Officially Supported', then we should check the Airbrussh build passes for each SSHKit PR, otherwise SSHKit and Airbrussh may diverge and we may have to field support issues about 'my version of Capistrano / SSHKit doesn't work with Airbrussh'.

When I was working in SSHKit, it was hard to know if what I was doing would break Airbrussh. Therefore, I had to have this sort of 2 repository setup where I commit into SSHKit, then run the build the changes in Airbrussh. In some cases I did break Airbrussh and @mattbrictson had to spot it an let me know. Some of these breakages required changes to the work already committed in SSHKit, more PRs etc.

It's not the end of the world, but it added quite a bit of overhead. For other contributors I don't think it's going to be feasible to ask them to run their prospective changes against Airbrussh before each SSHKit PR is merged. I envisage that the core team will handle the Airbrussh changes to make it work with SSHKit, and provide support on which versions can work with which, but for some SSHKit changes this may be a non trivial amount of work. I'm not sure whether the benefit of keeping the SSHKit code size down is worth the effort of dealing with the version / configuration management and support.

Some code in Airbrussh is around the Capistrano integration, so this could be moved into the Capistrano repo rather than the SSHKit one. There is some code in Airbrussh which duplication of code already in SSHKit (eg the Colors class which is now in SSHKit). I think there would be some opportunities for refactoring duplicated concepts between Airbrussh and the other formatters, so I'm not sure that the final impact on the SSHKit code base would be that severe. I'd be happy to work on this.

To put some concrete numbers on this, I think the core new classes are the following:

command_formatter.rb - 89 loc
console.rb - 87 loc
console_formatter.rb - 121 loc
delegating_formatter.rb - 45 loc
formatter.rb - 17 loc
log_file_formatter.rb - 43 loc

I think these files would no longer be needed in SSHKit:

capistrano.rb
colors.rb
configuration.rb
version.rb

On the testing side, I believe that the formatter acceptance-type test tests quite a bit of the SSHKit formatter functionality and was useful for spotting regressions in the Pretty formatter. Ideally, I'd like to have something like this in SSHKit anyway.

Hope this helps to explain my thinking a bit.

[mattbrictson]

Let's back up a bit and clarify the goals of this integration. What are we trying to accomplish?

1. Improve the out-of-the-box experience of Capistrano (nicer logging output)
2. Make the Airbrussh code easier to maintain and less susceptible to breakage due to upstream changes
3. Give the community a better set of APIs/base classes/utilities (extracted from Airbrussh) that other developers can use to make their own formatters
4. Keep SSHKit as stable and as narrowly-focused as possible

Is that about right?

If (1) is the top priority, then adding Airbrussh to capistrano.gemspec is the easiest way forward. Or we could include adding Airbrussh to your project's Gemfile as part of the installation instructions, or display "considering installing Airbrussh!" as a post-cap install message. However, this would not help with (2) and (3).

Another approach could be introducing an sshkit-formatters gem. There is some precedent here with minitest vs mintiest-reporters (we could look at those projects for inspiration). Airbrussh's code would be merged into this new gem, and perhaps we would move all but the most trivial formatter out of SSHKit code and into sshkit-formatters as well. This would allow a nice foundation of shared formatting code to be built up while keeping SSHKit itself lean. Does it address the testability, though (point 2)? Not sure. It also feels more ambitious than other approaches we've discussed so far.

[robd]

Is that about right?

This sounds right to me.

One thing I'd add: I see having a small number of gems as also being in service of goal (1) since it reduces the number of combinations that need to be managed and updated by us and users.

I'm not in favour of splitting out a sshkit-formatters gem because I prefer to have immediate feedback if a change made in SSHKit breaks some piece of formatting or requires a change to the formatters.

I think splitting repos makes sense if the changes we are making can just take place in one of the repositories without impacting about the other one, but I'm not sure whether this would be the case with the current coupling between the formatters, commands, config etc. I worry that having to manage changes across 2 repos would have the same drawbacks as we currently have with the separate Airbrussh repo.

[mattbrictson]

Maybe the first step should be to refactor in order to reduce coupling and make the code less brittle? That would be a good thing regardless of whether/how we merge.

For example, the formatters are tightly coupled to the Command class, which is complex and has mutable state. What if we changed the formatter API so that it no longer used Commands at all, but a much simpler, narrowly-focused immutable object like an Event?

Further, we could do an Airbrussh 1.0 release that removes all legacy support, and targets only the latest SSHKit and Capistrano. That would slim down the code quite a bit and put us in a better place to carry out an eventual merge.

Anyway, the more we talk about this, the more I think that this is shaping up to be a lot of work for what, at the end of the day, is just log formatting. What is the simplest way forward?

[robd]

Anyway, the more we talk about this, the more I think that this is shaping up to be a lot of work for what, at the end of the day, is just log formatting. What is the simplest way forward?

I think it really depends on whether there is the desire to combine Airbrussh into SSHKit. If it's only me that thinks this is preferable then go for the plan you outlined above.

What is your preference? Do you think Airbrussh should stay as a separate gem?

[mattbrictson]

What is your preference? Do you think Airbrussh should stay as a separate gem?

Well, Airbrussh already works pretty well as an independent gem. No monkey patching is needed to plug it into SSHKit, because SSHKit already has a documented extension point for formatters.

Yes, changes to SSHKit sometimes break Airbrussh, but that is par for the course. If we combine the codebases, breakages will still happen, they will just be identified more quickly. I feel like a better solution is to make the formatter API/extension point more robust and less sensitive to internal state, so breakages will be less likely to occur.

So I guess I am leaning toward keeping Airbrussh as a separate gem.

Regarding your earlier points, I think we can address them, at least to some extent.

1. [merging] removes the need for version management / multiple version support / CI setup between Airbrussh and SSHKit, which has been the cause of quite a bit of the Airbrussh work.

We can mitigate this somewhat by eliminating legacy SSHKit support in the next version of Airbrussh.

2. Refactoring in SSHKit becomes easier [if we merge] because you don't have to search across multiple projects and run multiple builds

True. But I am hoping that if we come up with an improved, more robust formatter extension point (with test coverage that approximates how Airbrussh uses that API), then downstream dependencies will be less likely to break due to future refactoring.

3. [merging] would give us a chance to consolidate the tests and use the SSHKit vagrant setup for proper acceptance tests

Capistrano also has a Vagrant setup, so we could put some acceptance tests there (since Capistrano will be using Airbrussh formatting)

4. Some of the features implemented in Airbrussh might be useful in other formatters (eg rake task highlighting)

We could use the same approach as minitest-reporters to allow Airbrussh and other community-contributed formatters to share code within one repository, while keeping SSHKit stable and narrowly-focused.

[robd]

I guess in this case we just have different experiences about the pain of managing the different versions. For me it was informed by trying to work across them during the work I did on trying to improve the formatter API earlier this year, going backwards and forwards trying to get everything to work together, setting up multiple version tracking CI, tests etc.

Even if you remove support for the legacy versions of SSHKit, we will still face these same questions of cross Airbrussh / SSHKit / Capistrano compatibility in the future unless we essentially freeze the formatter API as it is. I think this is what will happen by default, because it will be less appealing to work on changes which change the API if this also means:

• Work out what versions of Airbrussh / SSHKit / Capistrano will work together
• Set up multiple versions of tests which work across the different versions
• Set up gemspecs so that people don't end up with the wrong versions installed
• Managing issues by users related to having the wrong version of some gem

I don't find that type of work enjoyable.

I think managing that ecosystem would be an acceptable trade off if we had 10 different complex formatters with loads of different people working on them, or if the formatters were thousands of lines of code. But essentially we have 2 formatters (simple/pretty which are basically a single formatter with an option), and Airbrussh. If they were side by side in the same repo I think much more duplication could be removed, similar to the work I did earlier in the year. The dot and black hole formatters are so simple as to be no-ops. There are only a handful of people working on them.

As an example, I have been putting off splitting out the CommandResult object from the Command object because I was hopeful of being able to work on everything in the same repo (and partly because I'd like to wait for the next SSHKit release). When I think about the work involved it does feel like it would be harder and take longer doing it across the 2 projects and this puts me off working on it.

Anyway, I think my protestations have failed to sway you! Since I am in the minority we should go for the plan above but I think this is a missed opportunity to make our lives less complicated.

[leehambley]

(Thanks for the discourse guys, I'm in a bit of a pinch with http://harrow.io (huge release, mailing, etc) so I might be out of the loop until the dark side of the weekend, so far all seems like we're on the same page)

[mattbrictson]

How about I start with a minor refactor, like removing Airbrussh::Colors and depending on SSHKit's instead. I'll get some firsthand experience coordinating cross-repo changes and see what that's like. Maybe it will make me reconsider the approach.

[leehambley]

Cool Matt, o have to get a new SSHKit release out this week, anyway, if
there's something I can do to help, let me know.
On 26 Oct 2015 6:55 p.m., "Matt Brictson" [email protected] wrote:

How about I start with a minor refactor, like removing Airbrussh::Colors
and depending on SSHKit's instead. I'll get some firsthand experience
coordinating cross-repo changes and see what that's like. Maybe it will
make me reconsider the approach.

—
Reply to this email directly or view it on GitHub
#1509 (comment)
.

[robd]

@mattbrictson I think reusing the SSHKit Color class would be a good change either way. Another change I have thought about is whether this class should be renamed to Colors (plural) as it is in Airbrussh, since it doesn't represent a single color.

[mattbrictson]

@leehambley @robd The changes needed to integrate Airbrussh into Capistrano turned out easier than I expected, and I don't see any need to merge them gems.

Here are the PRs that accomplish the integration:

• Allow all Formatters to accept options hash sshkit#308
• Allow Airbrussh to be configured with an options Hash mattbrictson/airbrussh#70
• Make Airbrussh the default formatter for Capistrano #1541

[leehambley]

@leehambley @robd The changes needed to integrate Airbrussh into Capistrano turned out easier than I expected, and I don't see any need to merge them gems.

Great news, I just saw capistrano/sshkit#308 and the same occurred to me. Let's move forward on this then.

[mattbrictson]

I'm going to consider this issue to be done. 😄