[WiP] Refactor common Kontena classes in monorepo form

[SpComb]

Some of the kontena components share code, which is currently copy-pasted between components, with small variations. Introduce a new top-level common/lib path in the form of an unpublished kontena-common gem, which is loaded directly from the local path. This requires the Docker images to be built in the top-level project context, but brings with it the advantages of a monorepo for tightly integrated code under active development: the ability to both change the common code and all callers in the same PR, and no need for separate versioning/publishing of the common code for every trivial change.

Changes

Refactor the agent to use the following kontena-common classes, intended to be shared with the server:

• Kontena::Logging
• Kontena::Wait::Helper

TODO

• Fix the kontena-common version to use ./VERSION
• Fix the release builds
• Also refactor the server, test components to use the common libs
• Figure out how to use the kontena-common gem from the cli code (not built as a Docker image, but published as a gem)
• CI integrations etc

[SpComb]

Using bundle exec to add the /opt/kontena/common/lib to the $LOAD_PATH... not sure if the best way to do it, but probably the fastest.

• kontena gem build /opt/kontena/common/kontena-common.gemspec && gem install kontena-common-*.gem?
• ???

[jakolehm]

Could we keep Dockerfile under agent?

[SpComb]

Yes, we could, but I think it's better to have the Dockerfile be in the actual docker build context dir... That way it's apparent that you have to build it with kontena $ docker build -t kontena/agent -f Dockerfile.agent .. If it is just agent/Dockerfile, then the obvious thing of kontena/agent $ docker build -t kontena/agent . will fail.

[SpComb]

TODO to read this from ../VERSION

[kke]

This already exists as lib/kontena/logging.rb and can be required by require 'kontena/logging'. I don't think this kontena-logging.rb adds much value.

[SpComb]

This is there to define the Kontena module... just require 'kontena/logging' will fail if it's the first thing being required, and nothing else has yet done module Kontena.

Could change these to just use require 'kontena/logging', but then each class would have to be like

module Kontena
  module Foo
    class Bar
      # actual code with too much indentation

Not so much a problem for the basic logging thing, but annoying enough already for the Kontena::Websocket::Client.

[SpComb]

AFAIK besides ruby gems don't really allow for requiring specific sub-parts of gems in general, you generally have a single top-level require per gem.

But I don't want to make kontena-common.rb require every possible thing either... so several separate top-level requires like this instead?

[SpComb]

@jakolehm @jnummelin @kke I'd like some initial feedback on the design before I go any further, to avoid wasted effort. Does the concept of a common load path for shared code between the different server/agent/cli/test components in the same repo make sense? Plan is to start with the Kontena::Logging and Kontena::Wait::Helper shared by the agent and server (and eventually e2e tests), and then the Kontena::Websocket::Client shared by the CLI and agent in #2560.

The intention isn't to be a replacement for creating separate gems, but a pragmatic alternative to copy-pasting code between the different components... if there's some function that would be useful in both the server and cli, the threshold for moving it into some external gem is pretty high, and just copy-pasting it is easier. This kontena-common is meant to be better than copy-pasting.

There's some issues with a single kontena-common gem, like mass dependencies - it doesn't make sense for the CLI to depend on msgpack/eventmachine etc just because some code common to the server and agent need it... have to:

• omit the dependencies from the kontena-common gem and list them in the individual components instead (still better than copy-pasting the code itself)
• have multiple such gems in this repo
• split those shared components into separate external gems

As regards shared components like the Kontena::Websocket::Client, starting to develop them in kontena-common could also be regarded as a kind of incubator... it's much easier to iterate on those components and refactor them when both the code and all the callers of that code are in the same monorepo... once the component is more mature and the code/interfaces are more stable, and the component would also be useful in other projects, then it makes sense to split it out as a separate project.

[kke]

There are up and downsides.

Pros:

• Shared code is shared and not duplicated

Cons:

• The dependency problem, either depend on unnecessary msgpack etc or end up with a handful of separate kontena-core- gems.
• CLI can't use unpublished gems unless they are vendored (unpacked to cli/vendor) because gem install != bundle install.
• Using published gems will actually make it more multi than monorepo, which can be either a good or a bad thing. With opto and plugins it seems to work out ok.
• Reducing repetition by splitting things into separate components can actually cause increased repetition as the all of the subprojects have similar "boilerplate" such as spec helpers, initializers and what not.
• Risk of namespacing issues and accidental overlapping. There are already mixed conventions in the CLI, Kontena::MainCommand vs Kontena::Cli::NodeCommand, Kontena::PluginManager instead of Kontena::Cli::PluginManager etc.

Some observations:

• Including the waithelper will bring in Kontena::Logging as well.
• What else would be under Kontena::Wait namespace other than the Helper? There's already things like Kontena::Cli::Helpers::HealthHelper. Perhaps this should then be Kontena::Common::Helpers::WaitHelper or something like that, but it's a bit lengthy. Maybe just Kontena::WaitHelper.
• If it's a separate gem, it may just as well be top level Wait.until, no need for namespace. Instead you would have some configuration options such as Wait.logger = Kontena::Logging.logger.
• We have a lot of including going on and should probably work towards reducing it instead or at least pay attention to method visibility. This sort of utility/helper methods should not appear as public instance methods on classes where the module is included. See 1 - 2 - 3

I think a pattern for something like KontenaHelper could be:

module KontenaUtil
  extend SingleForwardable

  autoload :WaitUntil, 'kontena_util/wait_until'  
  def_delegator :WaitUntil, :wait_until
end

module KontenaUtil
  module WaitUntil
    module_function
    def wait_until(*opts)
      ...
    end
  end
end

class Foo
  include KontenaUtil::WaitUntil

  def execute
    wait_until(:xyz) do 
      ..
   end
end

class Bar
  def self.execute
    KontenaUtil.wait_until(:xyz) do
      ..
    end
  end
end

> Foo.new.wait_until => private method wait_until called
> Foo.new.public_methods.grep(/wait_until/) => nothing found
> Foo.new.execute => ok
> Bar.execute => ok

Another option is kontena-common/core-ext which would extend Kernel (or Kontena) with the helpers. Stdlib does something like this, for example URI("https://foo") works because it does def URI(..) in the Kernel module. In this case i suppose it would be something like WaitUntil(foofoo) do ... . The dry-rb folks are doing something similar.

And a note about module/class nesting:

module A
  module B
    def self.foo
      "foo"
    end
  end
end

module A::C
  def self.bar
    B.foo
  end
end

module A
  module D
    def self.baz
      B.foo
    end
  end
end

A::D.baz
=> "foo"

A::B.bar
=> uninitialized constant A::B::C (NameError)
Did you mean?  A::C

If you use explicit nesting, you can do something like:

require 'kontena-common/wait_helper'

module Kontena
  module Cli
    module Stack
      class InstallCommand < Kontena::Command
         def execute
           WaitHelper.wait_until('xx') do
             ..
           end
       end
    end
  end
end

If you don't, you can't:

require 'kontena-common/wait_helper'

class Kontena::Cli::Stack::InstallCommand < Kontena::Command
  def execute
    WaitHelper.wait_until('xx') { ... } 
  end
end
=> uninitialized constant Kontena::Cli::Stack::InstallCommand::WaitHelper

Why this happens, I have no idea. Anyway, explicit nesting is often better anyway as you don't hit those "parent module namespace does not exist yet if you require 'foofoo/foofoo'" situations.

[SpComb]

Thanks for the response

Risk of namespacing issues and accidental overlapping. There are already mixed conventions in the CLI, Kontena::MainCommand vs Kontena::Cli::NodeCommand, Kontena::PluginManager instead of Kontena::Cli::PluginManager etc.

Yeah... this is a problem with the agent, because it has all of its code under lib/kontena... I think most of that should be moved to app, dropping the Kontena:: prefix from the agent stuff, like the server does?

What else would be under Kontena::Wait namespace other than the Helper? There's already things like Kontena::Cli::Helpers::HealthHelper. Perhaps this should then be Kontena::Common::Helpers::WaitHelper or something like that, but it's a bit lengthy. Maybe just Kontena::WaitHelper.

Yeah, the Kontena::Wait::Helper isn't ideal now, I reckon I would fix it to be implemented as module-level functions like:

Kontena::Wait.wait_until { ... }

And then perhaps a separate mixin-module that just provides identical instance methods:

class Foo
  include Kontena::Wait::Helper
  
  def foo
    wait_until { ... }
  end
end

However, there's not really that much benefit to mixing in those methods... what about?

Kontena.wait_util { ... }

In preference to monkey-patching Kernel... still easy to type, easy to read (and figure out where it's from)

[SpComb]

CLI can't use unpublished gems unless they are vendored (unpacked to cli/vendor) because gem install != bundle install.

Yes... I need to figure out how that works, the gems probably do need to get published in the end in order to be able to use them in the CLI.

However, having the common gems in the same git repo and using the same version numbering would still be an advantage.

[kke]

And then perhaps a separate mixin-module that just provides identical instance methods:

(
That's what module_function is for. Just write module_function to the top of a module and your entire module becomes a lot better. (or finetune by module_function :method_name each appropriate method)

module Baz
  module_function

  def foo
    puts "bar"
  end

  def bar
    puts "foo"
  end
end

Baz.foo
Baz.bar
Class.new { include Baz }.new.foo # private method foo called 

)