Change the working directory in run script to APP_HOME if set #228
Conversation
|
Can one of the admins verify this patch? |
|
[test] |
mcyprian
force-pushed
the
app_home-cd
branch
from
7413916
to
e80dd8e
Compare
|
Can one of the admins verify this patch? |
|
Rebased against master branch after merge of #234 . |
mcyprian
force-pushed
the
app_home-cd
branch
from
e80dd8e
to
d722019
Compare
|
[test] |
2.7/s2i/bin/run
Outdated
| @@ -35,6 +35,13 @@ function get_default_web_concurrency() { | |||
| echo $default | |||
| } | |||
|
|
|||
| APP_HOME=$(readlink -f ${APP_HOME:-.}) | |||
There was a problem hiding this comment.
Double quotes needed around APP_HOME
There was a problem hiding this comment.
When trying this APP_HOME=$(readlink -f ${"APP_HOME":-.}) it ends up with an error bash: ${"APP_HOME":-.}: bad substitution, maybe I don't understand your point here correctly
There was a problem hiding this comment.
I guess APP_HOME=$(readlink -f "${APP_HOME:-.}")
|
It will be after next week before I can look at this properly. I will be finally released then from major thing I had to get done and will have free time again. :-) |
|
[test-openshift] |
src/s2i/bin/run
Outdated
| @@ -35,6 +35,13 @@ function get_default_web_concurrency() { | |||
| echo $default | |||
| } | |||
|
|
|||
| APP_HOME=$(readlink -f ${APP_HOME:-.}) | |||
There was a problem hiding this comment.
APP_HOME in the readlink call needs to be double-quoted to avoid word splitting
src/s2i/bin/run
Outdated
| if [[ -z "$APP_MODULE" && -f "./wsgi.py" ]]; then | ||
| APP_MODULE=wsgi | ||
| elif [[ -z "$APP_MODULE" && -f $setup_py ]]; then | ||
| APP_MODULE="$(python $setup_py --name)" |
There was a problem hiding this comment.
Same for $setup_py and HOME in this block.
|
Otherwise lgtm, I think its worth it to get this merged in. |
|
Can you wait until I have had another look before merging. I recollect that originally this change wasn't even doing things in the way that I had originally asked for and felt was required. I am on holidays right now, but will try and look later today or tomorrow. |
|
For For Django case, are there any possible risks with |
|
[test][test-openshift] |
|
Thanks for your comments @GrahamDumpleton. The I am not aware of any issues caused by |
|
Can one of the admins verify this patch? |
pkubatrh
force-pushed
the
app_home-cd
branch
from
d45b33b
to
f8ddc3f
Compare
|
Rebased onto current master for latest bits [test-openshift] |
Since 'readlink -f' never returns '.' the check would always be true.
|
Removed the check for @GrahamDumpleton @torsava Would either one of you guys have some spare time to review as well? |
|
[test-openshift] |
|
I've pointed some things in the comments. Besides that it looks good! |
|
New changes pushed [test-openshift] |
|
[test-openshift] |
#170 was not updated for quite some time, so I went through originally proposed changes and the discussion. I tried to make up something that resolves problem described in #166, doesn't break any of checks implemented in run script and simplify logic of the script. I would like to hear your opinion on this @GrahamDumpleton @pkubatrh.