[issue-2170] Modified runLocally function to accept closure

[bsacharski]

Q	A
Bug fix?	Yes
New feature?	Yes
BC breaks?	No
Deprecations?	No
Fixed tickets	#2170

As noted in the issue #2170, the logic in the recipe/check_remote.php would try to get the binary path of local and remote git executable.

However, due to specific implementation, the get('bin/git') would have been called twice in the context of remote host, instead of invoking it once on remote and once on local machine.

The runLocally function has thus been reworked, to accept Closure, which will be invoked in the context of the localhost machine.
Once a Closure has been passed, the function will push the Localhost to the Context, invoke the closure, store the stdout (if available), pop the Localhost from the Context and finally return the stdout.

For compatibility reasons, the function also accepts the string argument, however it is wrapped in a closure and once again passed to the runLocally function.

The recipe/check_remote.php file has been reworked, to utilize the Closure, to make sure that the get('bin/git') is being called in the Localhost context.

[antonmedv]

This is an interesting idea, but it repeats task functionality. Also if check_remote will be running on a few hosts run part of code will be executed multiple times.
Task can be executed in localhost context:

task('deploy:check_local', function () {
        $remoteLs = run(sprintf("%s ls-remote $opt $repository $ref", get('bin/git')));
})->local();

[bsacharski]

This is an interesting idea, but it repeats task functionality.

I'm not sure whether it repeats the functionality. It can achieve the same thing, but IMO represents different intentions:

• task()->local() represents a task that is to be executed on a local machine
• runLocally() represents a command that should explicitly be executed on a local machine

With runLocally we can mix contexts when needed, to have a task executed mostly on a remote host, with only one or two commands that need to be executed on a localhost.

Also if check_remote will be running on a few hosts run part of code will be executed multiple times.

Technically, we could add a check that would prevent multiple calls, but I would concentrate on that once we discover that performance really is an issue.

Task can be executed in localhost context:

task('deploy:check_local', function () {
        $remoteLs = run(sprintf("%s ls-remote $opt $repository $ref", get('bin/git')));
})->local();

To me it looks like it splits the deploy:check_remote step too much - it would probably end up with:

task('deploy:check_version', [
  'deploy:check_local', // this is executed only once on local machine, sets the 'targetRevision` value
  'deploy:check_remote', // this has to grab a value set by the `check_local` and compare it against remote version
]);

Not sure if such separation is desired.

[antonmedv]

I like the idea in general, but for me, it seems a more complex thing to add. We have local tasks and on host functionality.
What about thinking in this direction (it should work from my perspective):

on(localhost(), function () {
    $remoteLs = run(sprintf("%s ls-remote $opt $repository $ref", get('bin/git')));
});

[bsacharski]

Looks nice. Do you want me to create a new PR with check_remote reworked to utilize on()?

[antonmedv]

Yes, it will be cool. Also, check if this functionality actually work))