Refactor Configuration Hierarchies #153

Closed
theory opened this Issue Mar 26, 2014 · 2 comments

Comments

Projects
None yet
1 participant
@theory
Collaborator

theory commented Mar 26, 2014

In the engine-config branch (c4485af), I started moving around the canonical references to a number of attributes App::Sqitch::Engine, in trying to find an easy-ish way to resolve #151. Unfortunately, it turned out to be way more than I wanted to take on for the amount of time I had, mostly because App::Sqitch and App::Sqitch::Engine are now all up in each other's business---and I was going to end up with Plan affected, too!

I think this needs a pretty major re-think, instead. The current architecture is a result of the original implementation supporting only one configuration, engine, and plan, but now, especially with targets, we need to be able to support many plans and engines at once.

So I think we to re-organize things to better handle the hierarchy of options and configuration. The hierarchy is, essentially, for any given attributes, an appropriate value should be sought out in this order:

  1. Command-line option
  2. Target attribute (read from config)
  3. Engine config value (core.$engine_key.$attr)
  4. Core config (core.$attr)
  5. Reasonable default

Not all attributes will need to search all these places, and some may not have reasonable defaults, so should die, instead. But in any event, I think there needs to be some sort of nexus for these sorts of lookups. It should have:

  1. An object representing command-line options
  2. An object representing target config
  3. An object representing engine config
  4. An object representing core config

I think this ought to be a singleton object created for every run. Perhaps it could also handle the command-line options for specific commands, too. It would be the first thing created when the app starts. Perhaps App::Sqitch->run creates it, then uses it to figure out what to do. Perhaps it has an attribute representing the command, too (derived from the command-line parsing, after all).

In this scenario, App::Sqitch itself just becomes the nexus for creating and dispatching objects, and ceases to be the place where options and configuration are handled. Maybe something like this:

my $core = App::Sqitch::Core->setup(@ARGV);
$core->command->execute;

So App::Sqitch::Core is the new singleton object I describe. It knows what the command is, loads it up and returns it, but the command has no reference to the core object itself (no circular references). The command can call App::Sqitch::Core->instance to get the singleton to interrogate when it needs it. Perhaps all of the utility methods currently in App::Sqitch could go there, too.

Or maybe App::Sqitch itself needs to be rewritten this way, with no ::Core. There are a lot of references back to App::Sqitch itself all over the place that would need to be ripped out. So maybe simpler would be to leave those, and instead have it not be a singleton, but just change its interface so that it no longer represents options and config in its attributes but just stores the option and config objects as attributes.

Yes, I think that's the way to go. Less work. Yes, it changes the interface of App::Sqitch quite a lot, but I doubt anyone will complain.

@theory theory added todo labels Mar 26, 2014

@theory

This comment has been minimized.

Show comment
Hide comment
@theory

theory Mar 27, 2014

Collaborator

Some thoughts as I tried to go to sleep last night after writing this. Modify go to:

  1. Parse options, determine the command
  2. Load the config
  3. Parse the command options and load the command
  4. Let command determine the engine from either:
    1. --engine
    2. a target in the command-line
    3. A default target in the config
  5. Have the command load the the plan, if it needs it, based on either:
    1. --plan-file
    2. Target config
    3. Engine config
    4. Core config
    5. Default
  6. Have Command load the engine, if it needs it; let it determine its attributes from:
    1. Command-line options
    2. Target config
    3. Engine config
    4. Core config
    5. Defaults
  7. Execute the command

Likely will require some fiddling, but the idea is to load the most specific stuff first, and continue with more general things. So always prefer to find attribute values following this hierarchy of configuration:

  1. Command-line args
  2. Target config
  3. Engine config
  4. Core config

I think the Target object will contain the canonical locations for these things. It should be accessible via an attribute in the Sqitch object, as should Engine.

Collaborator

theory commented Mar 27, 2014

Some thoughts as I tried to go to sleep last night after writing this. Modify go to:

  1. Parse options, determine the command
  2. Load the config
  3. Parse the command options and load the command
  4. Let command determine the engine from either:
    1. --engine
    2. a target in the command-line
    3. A default target in the config
  5. Have the command load the the plan, if it needs it, based on either:
    1. --plan-file
    2. Target config
    3. Engine config
    4. Core config
    5. Default
  6. Have Command load the engine, if it needs it; let it determine its attributes from:
    1. Command-line options
    2. Target config
    3. Engine config
    4. Core config
    5. Defaults
  7. Execute the command

Likely will require some fiddling, but the idea is to load the most specific stuff first, and continue with more general things. So always prefer to find attribute values following this hierarchy of configuration:

  1. Command-line args
  2. Target config
  3. Engine config
  4. Core config

I think the Target object will contain the canonical locations for these things. It should be accessible via an attribute in the Sqitch object, as should Engine.

@theory

This comment has been minimized.

Show comment
Hide comment
@theory

theory Oct 24, 2014

Collaborator

This is done with merge f972abd. I added a new Target class that splits out the engine and target configuration from Engine and Sqitch. It's a simple class, but since nearly every file touched App::Sqitch, it was a lot of work to refactor things. But it's a much better division of labor this way, and gets us excellent hierarchical configuration.

Collaborator

theory commented Oct 24, 2014

This is done with merge f972abd. I added a new Target class that splits out the engine and target configuration from Engine and Sqitch. It's a simple class, but since nearly every file touched App::Sqitch, it was a lot of work to refactor things. But it's a much better division of labor this way, and gets us excellent hierarchical configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment