Refactored the config.

[ghost]

This will be one heck of a code review. Let's address what I did here so you have a heads up.

• I had a goal to configure the config once and use it throughout the current simple_deploy run.
• I refactored the Config class into a Configuration module as part of that goal.
• The second move towards that goal was the addition of the ResourceManager class.
• I hate the Singleton design pattern as much as anyone, but in cases of configuration or logging it is useful. In this case it is making sure we only load the environment configuration once per run.
• I plan to have ResourceManager manage the logger instance too when I do that refactoring.
• It may look nuts, but this branch adds a few more passes of the logger instance. There is a method to my madness. That kept me from having to support the logger inside the Configuration module, which I viewed as a bad design. It also left me a trail of logger stuff I can use to more quickly refactor the logger.

The flow is pretty easy. The CLI classes know the environment the user specified so they run the configure. All of the other classes simply request the config instance from ResourceManager.

I know this code review will be painful because there are so many changes. Unfortunately, progress is hard. The only way I can see to clean up this code is to attack end to end for each problem. That results in lots of changes.

Thanks.

[thbishop]

@rickmendes What are your thoughts about possibly using module_function for the config/logger interface. I think it would still call the ResourceManager singleton under the covers:

# in lib/simple_deploy.rb
require ...
....

module SimpleDeploy

  module_function
  def config
    @config ||= SimpleDeploy::ResourceManager.instance.config
  end

end

and then calling it would look like:

SimpleDeploy.config.region

I think this buys you somewhat cleaner code in the caller and also a layer in which you can refactor ResourceManager or replace it without impacting anything else.

Logging could follow a similar pattern.

[thbishop]

Another option could be to have a module which is included in the classes which exposes config, logger, and whatever other common logic is needed.

[ghost]

@thbishop +1 for module_function

I think it is a lot cleaner. I also like putting it under SimpleDeploy because config, logger and maybe a few other things are central to every command. I would only use an additional module if these things were used for only a few commands.

I can make these changes tomorrow.

[ghost]

@thbishop I was thinking about module_function and the CLI config initialization too. What if the SimpleDeploy module also had an init_config method?

def init_config(environment)
  SimpleDeploy::ResourceManager.instance.config environment
end

[ghost]

@thbishop While making the module_function changes, I started to think about dropping ResourceManager entirely. Let's talk about it when you have time. Here is a snippet of what the code would look like.

module SimpleDeploy
  module_function

    def create_config(environment, custom_config = {})
      @config = SimpleDeploy::Configuration.configure environment, custom_config
    end

    def config
      @config
    end
end

[thbishop]

@rickmendes yep, brett and i were talking about the same thing earlier

[ghost]

@thbishop @brettweavnet This is why I still love code reviews even after decades in the software world. This whopping checkin not only has cleaner code, but it lays a much better foundation for handling logger and other potential common resources. Thanks for the ideas.

[thbishop]

@rickmendes after seeing how often we have this pattern:

@config_mock = mock 'config mock'
SimpleDeploy.should_receive(:config).and_return(@config_mock) 

it may be a useful to have a shared context with this. I think it could be leveraged by the logging refactor as well.

thoughts?

[ghost]

@thbishop I am a proponent of shared groups with rspec so this can be the first one on our list.

[ghost]

@thbishop I found 6 cases where config_stub was used instead of config_mock. Those also take arguments. I will add a context for those tomorrow.