Skip to content
This repository has been archived by the owner on Apr 26, 2020. It is now read-only.

fix(rsync.php): If local server run rsync to local #53

Closed
wants to merge 1 commit into from

Conversation

jbdelhommeau
Copy link

@jbdelhommeau jbdelhommeau commented May 6, 2016

Fix bug #52

Proposal fix.
Thank you @nmarniesse.

@mablae
Copy link
Contributor

mablae commented May 28, 2016

I also run into the same bug, would be awesome to get this fix merged.

@antonmedv
Copy link
Member

@johnny-bit ping

@johnny-bit
Copy link
Contributor

Hi @ALL ! Well, this is actually funny. rsync recipe was made to deploy code compiled locally to remote server. In case of local server, I though it should be sufficient to run rsync just like normal operation on "remote server". That's mostly because my dev machine is configured to act as a remote server too (that means I have ssh open, but fortunately I'm on masked network).

So... While I'm not opposed to this fix, I'm neither for it, since IMO better fix would be to configure own environment to act like remote server instead of local server. However, this fix adds very little complexity, so if all agree, i se no reason not to include this fix into recipe.

$identityFile = $server->getPrivateKey() ? ' -i ' . $server->getPrivateKey() : '';
$user = !$server->getUser() ? '' : $server->getUser() . '@';
}


runLocally("rsync -{$config['flags']} -e 'ssh$port$identityFile' {{rsync_options}}{{rsync_excludes}}{{rsync_includes}}{{rsync_filter}} '$src/' '$user$host:$dst/'", $config['timeout']);
Copy link
Contributor

Choose a reason for hiding this comment

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

This line will now be executed in both cases which is clearly a bug. It should be inside the else.

@mablae
Copy link
Contributor

mablae commented May 29, 2016

@johnny-bit This PR introduces a bug I think. #55 resolves this.

@johnny-bit
Copy link
Contributor

I haven't caught the bug at a simple look through. I still think that
ussing localServer for rsync adds unnecessary complexity for recipe. Now,
the only difference between both invocations of runLocally are
"$user$host". Why not set them in if/else block and have runLocally outside
of if/else block?

2016-05-29 19:34 GMT+02:00 Malte Blättermann notifications@github.com:

@johnny-bit https://github.com/johnny-bit This PR introduces a bug I
think. #55 #55 resolves this.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#53 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AKMVFWItoNLWKP1rqixc9JA1Wcw-CX4oks5qGc42gaJpZM4IYtHp
.

Pozdrawiam,
Hubert Kowalski

@mablae
Copy link
Contributor

mablae commented May 29, 2016

Saw #55 ? It solves this with early return statement and no else.

@mablae mablae mentioned this pull request Jun 2, 2016
@antonmedv
Copy link
Member

Please, rebase your PR.

@antonmedv antonmedv closed this Nov 13, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants