Suggestions for improvement

[vanstee]

• No way to easily stub dependencies of Command instance
  • initialize should receive request as argument
  • All state should be pulled out of request via command instance methods (memory_key, step, etc)
• Provide helpers to create response object with template in one go
  • render("users", json)
  • Consider expecting a response as return value for run_command
  • Allows return render("users", json), although render("users", json) and return should currently work
• Request should be initialized with arguments
  • Should receive ENV as a default argument (useful for DI in testing)
  • Should receive parsed input as a default argument
• Shouldn't override kernel method fail
  • Just use abort it does the same thing (prints message and exit 1)
  • Consolidate exception handling around run_command
• Command routing
  • Allow bundle method to receive router class (or block style creation of one)
  • Provide automatic router based on directory structure, default argument of above

Most of these improvements are to make testing simpler as most data can be more easily mocked out. Warning untested sudo-y code below, but notice there aren't any crazy mocks and stubs other than just a client being stubbed out.

class CogCmd::EC2::Find
  attr_reader :client

  def initialize(request, client = ::EC2::Client.new)
    super(request)
    @client = client
  end

  def run_command
    raise(Cog::Error, "Region option required") unless region
    render "find", client.instances(region)
  end

  def region
    options[:region]
  end
end

let(:client) { double(:client, instances: []) }

it "lists ec2 instances" do
  request = CogCmd::Request.new({
    "COG_OPT_REGION" => "us-east"
  })

  response = CogCmd::EC2.Find.new(request, client).run_command

  expect(response.body).to eq([])
end

it "fails if region is not specified" do
  request = CogCmd::Request.new({})

  expect { CogCmd::EC2.Find.new(request, client).run_command }.to raise(Cog:Error, "Region option required")
end

[christophermaier]

I wonder if commands should receive a list of args, a map of options, and the input... things we've already extracted from ENV and STDIN. That might also make testing easier, since you wouldn't need to manually reproduce all the various COG_X variables.

[vanstee]

Yeah, that's essentially what's happening since those are bundled up in a request object. I'd be ok with breaking those out though. I'm pretty much a fan of anything that helps us remove random access to ENV from inside an instance.