Proof of concept - move stuff out Idephix class
Tried to move out some responsibilities from Idephix class which is until now a God object. Tests are obviously broken, I'm more interested to undestrand if this approach could work. Now responsibilities are divided as follow:
- Idephix is a builder, it creates and wires all the objects
- Operations encapsulate low level operation such as local, remote, ...
- Application is a TaskExecutor: it can execute tasks
- Context is a facade towards operations, task execution and config object
MethodExtension is something I didn't touch yet, but I suppose it could be moved inside Operations or by definining a decorator for operations
what do you guys think?
Looks good to me 👍
With this new structure Extensions should wire methods inside operations, but keep in mind that extensions can also register tasks (Have a look a \Idephix\Extension)
@ftassi I was thinking about splitting Extension interface in 2:
- ExtensionMethod (or MethodExtension or MethodProvider): for adding methods
- ExtensionTask (or TaskExtension or TaskProvider): for adding tasks
makes sense?
@micheleorselli I'm not sure, but it won't hurt, so why not. We need some time and user feedbacks to figure out how extensions will really be used. I'd try to find a better name though:
-
TaskProviderandMethodProvidermaybe (not really sure)?
@ftassi "Provider" sounds good to me. Plus is a term already used in the symfony ecosystem (UserProvider, *Provider in Silex)
I squashed some commits and merged latest upgrade from master. If everything is fine we should merge this as soon as we can as this PR is a pain to maintain because changes a lot of responsibilities.
@micheleorselli @kea @dymissy have a look at this and give me your 👍
I would not merge this into master I rather prefer to substitute master implementation with this one, to avoid any extra conflict
@micheleorselli with this PR Idephix\Context loses its \Idephix\DictionaryAccess implementations. Without get and set method the user won't be able to access configurations for current env. Instead now there is a getConfig method that will retrieve the entire config object. Was this intended?
I don't see any harm in having a direct and convenient access to current configurations through the Context. Even if the currentEnv is exposed by the context, as I user, I don't want be bothered by the need of using it to select the current configuration set.
Was there any implementation issue with the get and set methods? What do you think? If we keep the current implementation we have to fix the doc http://idephix.readthedocs.io/en/latest/writing_tasks.html#accessing-configuration-from-tasks
@ftassi it wasn't intended. I'm +1 for adding a get to the Context object which acts as a proxy for Config. There are 2 things we should think about
- What happens if no env is specified?
- Should we allow users to access config values defined outside environment? This would allow defining globals params, env independent like
array(
'ssh_params' => array(
'user' => 'scouting',
),
'envs' => array(
'stage' => array(
'hosts' => array('10.250.2.8'),
'remote_dir' => '/var/www/vhosts/api.scouting.ideato.it/htdocs/current'
),
'prod' => array(
'hosts' => array('10.20.12.50'),
'remote_dir' => '/var/www/vhosts/api.scoutingmi.it/htdocs/current'
),
),
'ssh_client' => new \Idephix\SSH\SshClient(),
'extensions' => array(
'rsync' => new \Idephix\Extension\Project\Rsync(),
),
);
a $context->get('ssh_params.user') should first look in the current environment (if present) and then fallback to general config.
Makes sense?