Idephix icon indicating copy to clipboard operation
Idephix copied to clipboard

Proof of concept - move stuff out Idephix class

Open micheleorselli opened this issue 9 years ago • 8 comments

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?

micheleorselli avatar Aug 26 '16 16:08 micheleorselli

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 avatar Aug 29 '16 07:08 ftassi

@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 avatar Aug 29 '16 07:08 micheleorselli

@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:

  • TaskProvider and MethodProvider maybe (not really sure)?

ftassi avatar Aug 29 '16 07:08 ftassi

@ftassi "Provider" sounds good to me. Plus is a term already used in the symfony ecosystem (UserProvider, *Provider in Silex)

micheleorselli avatar Aug 29 '16 07:08 micheleorselli

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 👍

ftassi avatar Feb 24 '17 16:02 ftassi

I would not merge this into master I rather prefer to substitute master implementation with this one, to avoid any extra conflict

ftassi avatar Feb 24 '17 16:02 ftassi

@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 avatar Feb 24 '17 23:02 ftassi

@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

  1. What happens if no env is specified?
  2. 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?

micheleorselli avatar Feb 27 '17 10:02 micheleorselli