Skip to content

Proof of concept - move stuff out Idephix class - #91

Open
micheleorselli wants to merge 14 commits into
masterfrom
move_stuff_out_idx
Open

Proof of concept - move stuff out Idephix class#91
micheleorselli wants to merge 14 commits into
masterfrom
move_stuff_out_idx

Conversation

@micheleorselli

Copy link
Copy Markdown
Contributor

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?

use Idephix\Console\Command\ListCommand;

class Application extends BaseApplication
class Application extends BaseApplication implements TaskExecutor

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃憤

@ftassi

ftassi commented Aug 29, 2016

Copy link
Copy Markdown
Contributor

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)

@micheleorselli

micheleorselli commented Aug 29, 2016

Copy link
Copy Markdown
Contributor Author

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

@ftassi

ftassi commented Aug 29, 2016

Copy link
Copy Markdown
Contributor

@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)?

@micheleorselli

Copy link
Copy Markdown
Contributor Author

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

Michele Orselli and others added 12 commits February 24, 2017 15:48
moar code moving

typo

started fixing tests

more testfixes
fixes InitIdxFileTest

fixes ExtensionTest

adds operations test

fixes travis tests

moar travis fixes

moar travis fixes

moar travis fixes

moar travis fixes, the last one I swear

fixes tests on php 5.3
Merge pull request #99 from kea/run-task-once
moving the logic from the Idephix file to refactored Context class
see commit cc19f53
@ftassi
ftassi force-pushed the move_stuff_out_idx branch from 3bc9554 to 083fb57 Compare February 24, 2017 16:45
@ftassi

ftassi commented Feb 24, 2017

Copy link
Copy Markdown
Contributor

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

ftassi commented Feb 24, 2017

Copy link
Copy Markdown
Contributor

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

@ftassi

ftassi commented Feb 24, 2017

Copy link
Copy Markdown
Contributor

@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

@micheleorselli

Copy link
Copy Markdown
Contributor Author

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

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

Development

Successfully merging this pull request may close these issues.

3 participants