Plugin framework

[eguzki]

3scale Toolbox CLI is based on cri library for building command line tools.
Plugin system also uses cri to leverage easy to develop, flexible and extensible plugin system.

3scale Toolbox will load plugins installed in gems or $LOAD_PATH. Plugins are discovered via Gem::find_files, then loaded.
Few requirements must be met for a plugin to be loaded:

• Plugin must be named ${plugin_name}_command.rb and placed at /3scale_toolbox/commands/ from your root load path
• Plugin must implement module function ThreeScaleToolbox::Commands.command_${plugin_name}_definition and return instance of Cri::Command from cri

Nothing better than simple Hello World plugin to illustrate.

$ cat lib/3scale_toolbox/commands/foo_command.rb
module ThreeScaleToolbox
  module Commands
    def self.command_foo_definition
      Cri::Command.define do
        name        'foo'
        usage       'foo [options]'
        summary     'foo command'
        description 'This command does a lot of stuff.'

        flag :h, :help, 'show help for this command' do |_, cmd|
          puts cmd.help
          exit 0
        end

        run do |opts, args, cmd|
          puts "Hello World"
        end
      end
    end
  end
end

$ RUBYOPT=-Ilib 3scale foo
Hello World

$ RUBYOPT=-Ilib 3scale foo -h
NAME
    foo - foo command

USAGE
    3scale foo [options]

DESCRIPTION
    This command does a lot of stuff.

OPTIONS
    -h --help         show help for this command

OPTIONS FOR 3SCALE
    -h --help         show help for this command
    -v --version      Prints the version of this command

[mikz]

When this happens? Rubygems should be always available since ruby 1.9.

[eguzki]

right!

[andrewdavidmackenzie]

Looks great.
Interested to know what info (e.g. creds, from other common options) will be passed from toolbox into command for it to use...

[eguzki]

Update, copy and import source code has been copied from exe/3scale-command to files expected by plugin framework. No need to review commands themselves. Not a single line of code changed, just input parameter parsing.

Tried to keep track of the changes committing git mv, then refactoring to command format

[eguzki]

@andrewdavidmackenzie providing common info and tools has been design feature from the very beginning. This PR is about plugin framework. Next step is implementation of remotes feature where your point will be illustrated and offered for discussion.

[andrewdavidmackenzie]

Sure, was a somewhat forward looking comment, related to the "Interface" that "ThreeScaleToolbox::Commands.command_${plugin_name}_definition" must implement....

[mikz]

This does not feel right. It is pretty standard to match the name of the file with the name of the module.
I don't see anything wrong with ThreeScaleToolbox::CLI.

[mikz]

This pattern is repeated in every command.

It signals few things to me. Looks like this file has more than one responsibility and should be split. And that we should have an example how this split looks like (copy service, copy account, copy provider, each in own file). Finally that there is no way to extend existing commands, just add them to the root.

[eguzki]

Example how splitting would look like:

$ cat lib/3scale_toolbox/commands/copy_command.rb
require 'my_namespace/copy'

module ThreeScaleToolbox
  module Commands
    def self.command_copy_definition
      MYNAMESPACE.copy_command
    end
  end
end

$ cat lib/my_namespace/copy.rb
require 'my_namespace/copy_service'

module MYNAMESPACE
  def self.root_command
     Cri::Command.define do                                                       
       name        'copy'                                                         
       usage       'copy <command> [options]'                                     
       summary     '3scale CLI copy'                                              
       description '3scale CLI copy tools to manage your API from the terminal.'                                                      
     end                                                                          
  end

  def self.copy_command
    root_command.tap { |command| command.add_command copy_service_command  }
  end
end      

$ cat lib/my_namespace/copy_service.rb
module MYNAMESPACE
  def self.copy_service_command
     Cri::Command.define do                                                       
       name        'service'                                                         
       usage       'service [options] <service_id>'                                     
       summary     '3scale CLI copy service'                                              
       description '3scale CLI copy service tools to manage your API from the terminal.'        
       run do |opts, args, cmd|                 
         # Do the work
       end 
     end                                                                          
  end
end      

Hopefully it is clearer now. I think spliting responsabilities is not a limitation, can be done easilly. I did this way to keep everything in a single file like it was before. Focus on plugin system.

[mikz]

I'm sorry but I don't really like that. Imo commands should be classes/modules and not methods. Reopening a module to add a method to some global bucket when you can just define class and give it a name feels wrong to me. IMHO Class/Module should be defined in one file (except for case when modules are used as plain namespaces with no methods) and this violates that.

For the sake of this example I think the "copy" command and its subcommands will be shipped with the toolbox, so they should use the same namespace.

lib/3scale_toolbox/commands/copy_command.rb
lib/3scale_toolbox/commands/copy_command/copy_service.rb
lib/3scale_toolbox/commands/copy_command/copy_account.rb
lib/3scale_toolbox/commands/copy_command/copy_provider.rb

Another beef that I start to see is the naming juggling. Changing the name of the method that defines the command is super confusing and hard to follow. I'd definitely like more just constants and some well defined API. I understand that you need to chain the definitions, but I think that could be improved to be more idiomatic. Just provide some API to do that.

Here is a few alternatives.

module ThreeScaleToolbox::Commands::CopyCommand
  def self.command
     Cri::Command.define do  
       ...
     end
  end

  def self.subcommands
    [ CopyService ] 
  end
end

module ThreeScaleToolbox::Commands::CopyCommand::CopyService
  def self.command
    Cri::Command.define do 
      ...
    end
  end
end

module ThreeScaleToolbox::Commands::CopyCommand
  SUBCOMMANDS = []

  def self.extended(subcommand)
    SUBCOMMANDS << subcommand
  end

  def self.command
     Cri::Command.define do  
       ...
     end
  end

  def self.subcommands
    SUBCOMMANDS
  end
end

module ThreeScaleToolbox::Commands::CopyCommand::CopyService
  extend ThreeScaleToolbox::Commands::CopyCommand

  def self.command
    Cri::Command.define do 
      ...
    end
  end
end

module ThreeScaleToolbox::Commands::CopyCommand
  SUBCOMMANDS = []

  def self.extended(subcommand)
    SUBCOMMANDS << subcommand
  end

  def self.command
     Cri::Command.define do  
       ...
     end.tap do |command|
       SUBCOMMANDS.each(&command.method(:add_command))
     end
  end
end

module ThreeScaleToolbox::Commands::CopyCommand::CopyService
  extend ThreeScaleToolbox::Commands::CopyCommand

  def self.command
    Cri::Command.define do 
      ...
    end
  end
end

I'm not saying these are great or better. Just some food for thought.

[mikz]

I don't think it is a good idea to let plugins reopen this module.

I think a class/module should be defined in one file and thats it. Then in theory it could be frozen, so no other files can reopen it. Not that we would actually do so, but it sets some expectations and mindset.

I think more reasonable API would be to let submodules register into some command set.

For example if you require foo_command then it could at the and of the file/module add itself to 3scale toolbox ThreeScaleToolbox::CLI.add_subcommand self.

This would allow extending (copy service, copy account, copy provider, ...) non root (copy subcommand) commands like said in https://github.com/3scale/3scale_toolbox/pull/50/files#r201001411.

I think we should have an API that works on any level, not just the first one.
And one that uses own namespace and public API instead of reopening modules from different gem.

[eguzki]

Both issues you underline were intentionally defined as they are based on RubyGems plugin system design.

• Only extending first level commands
• Placing commands under a known namespace here

##                                            
# \Commands will be placed in this namespace  
                                              
module Gem::Commands                          
end

In fact, all rubygems built-in commands are defined in one file each and all of them under Gem::Commands namespace.

WRT only extending at command level (not allowing extending subcommands), it is indeed less flexible. But Rubygems does not allow it (not sure if heroku allows it either) and seemed to me a good starting point. Besides, it felt to me that subcommands should belong (at least in 99% of use cases) to main comand plugin project.

If extending subcommands is a requirement, we should highlight it clearly. It was not in requirements initially.

Can you elaborate more on your idea about letting submodules register into some command set? I'll try to figure out how to do it.

[eguzki]

I guess you mean

$ cat lib/3scale_toolbox/commands/foo_command.rb
module MyModuleName
  def self.command_definition
    Cri::Command.define do
      name        'foo'
      usage       'foo [options]'
      summary     'foo command'
      description 'This command does a lot of stuff.'
      run do |opts, args, cmd|
        puts "Hello World"
      end
    end
  end
end

ThreeScaleToolbox::CLI.add_command MyModuleName.command_definition

$ RUBYOPT=-Ilib 3scale foo
Hello World

This is a little change. No problem to do it. Although I like the way it is now. In order to discover plugins automatically, there needs to be some well known filepath pattern that is passed to Gem::find_files. Right now it is defined as: 3scale_toolbox/commands/*_command.rb, so using modules like MyModuleName in a file called 3scale_toolbox/commands/foo_command.rb does not seem right.

But still we need to define a way to extend on any level.

[mikz]

@eguzki Yes, I meant that. I'm not against letting plugins use the 3scale_toolbox namespace. That could be easily lifted in the future. My main beef is reopening a module to add there a method that does not belong to the namespace of the library. In the future we can look for file named 3scale_toolbox_plugin.rb in any of the gems and just load it.

With this pattern you should get the subcommands almost for free. They can register themselves into the main command or the main command can register them.

module ThreeScaleToolbox::Commands::Copy
  require_relative './copy/service'
  require_relative './copy/account'
end

module ThreeScaleToolbox::Commands::Copy::Service
  ...
  ThreeScaleToolbox::Commands::Copy.add_command self
end

module ThreeScaleToolbox::Commands::Copy::Account
  ...
  ThreeScaleToolbox::Commands::Copy.add_command self
end

Of course it is less than ideal because you actually have to require them, but that is not a big deal when you are in a scope of one module/command. Plugins packaged as gems could easily register themselves anywhere.