Remote binary paths may overwrite local binary paths (and vice versa) #2170
Comments
This is not true. This closure/memorization happens per host. So different hosts and localhost will store different results. Please provide a minimal example to show your case. Maybe something else is happening there. |
You're right and I feel stupid now. I've been using the ...
$remoteLs = runLocally(sprintf("%s ls-remote $opt $repository $ref", get('bin/git')));
if (strstr($remoteLs, "\n")) {
throw new Exception("Could not determine target revision. '$ref' matched multiple commits.");
}
if (!$remoteLs) {
throw new Exception("Could not resolve a revision from '$ref'.");
}
$targetRevision = substr($remoteLs, 0, strpos($remoteLs, "\t"));
}
// Compare commit hashes. We use strpos to support short versions.
$targetRevision = trim($targetRevision);
$lastDeployedRevision = trim(run(sprintf('cd {{deploy_path}}/current && %s rev-parse HEAD', get('bin/git'))));
...From what I understand these two lines will have the same value returned from runLocally(sprintf("%s ls-remote $opt $repository $ref", get('bin/git')));
...
run(sprintf('cd {{deploy_path}}/current && %s rev-parse HEAD', get('bin/git')))Both of the So it's not a problem with Deployer at all, it's just a problem with this specific |
|
Yes, it’s not right to use get with local commands. Can you make pr to fix it? I think it’s a easy fix. |
|
Sure, though I was thinking about reworking the If a closure was passed instead of a string, then the function would push the localhost context to the stack, run the closure and then pop the context from the stack. What do you think? |
bsacharski
added a commit
to bsacharski/deployer
that referenced
this issue
Sep 26, 2020
As noted in the issue deployphp#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.
bsacharski
added a commit
to bsacharski/deployer
that referenced
this issue
Sep 26, 2020
As noted in the issue deployphp#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.
bsacharski
added a commit
to bsacharski/deployer
that referenced
this issue
Sep 26, 2020
As noted in the issue deployphp#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.
bsacharski
added a commit
to bsacharski/deployer
that referenced
this issue
Sep 26, 2020
As noted in the issue deployphp#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.
bsacharski
added a commit
to bsacharski/deployer
that referenced
this issue
Sep 26, 2020
As noted in the issue deployphp#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.
|
With #2175 merged this can be closed. Thank you 👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
I have recently discovered bug where the values like
bin/git(defined incommon.phprecipe) would point to wrong binary.This was caused by the local and remote machines having different file structure:
/bin/git/usr/bin/gitDuring the deploy procedure the
get('bin/git')would've been called twice:return locateBinaryPath('git');to be called.Due to existing
getimplementation, the first time thatbin/gitwas accessed, the closure was executed and its result has been saved in place of that closure.As such, the second time the
bin/githas been accessed, the "cached" value has been returned immediately.tl;dr; using the
get(),combined with closure that contains call tolocateBinaryPath(), will result in the value being set to the binary path of the first host that the method was invoked on.To reproduce
/usr/bin/git/bin/gitget('bin/git')against remote hostget('bin/git')on local machineExpected behavior
The paths to binaries returned by the
get()should always reflect real path to the binary on the machine that thegetis being invoked on.IMO this could be done by either:
getknow its current context (so it would differentiate between different hosts)setmethod as "protected", meaning it would never be replaced with the result of invocation (simpler, but would impact the performance).Environment
remote was running on CentOS
local was running in alpine-based docker container
The text was updated successfully, but these errors were encountered: