-
Notifications
You must be signed in to change notification settings - Fork 568
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use eval in jenkins-run #968
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is wrong and you really should leave $PARAMS
alone. It probably needs to be solved on line 53.
@@ -75,4 +75,4 @@ if [ "$JENKINS_ENABLE_ACCESS_LOG" = "yes" ]; then | |||
PARAMS+=("--simpleAccessLogger.file=/var/log/jenkins/access_log") | |||
fi | |||
|
|||
$JAVA_CMD "${PARAMS[@]}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is my understanding that this way of using bash arrays actually should have the proper quotes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not about quotes in PARAMS array, it's about quotes in JENKINS_JAVA_CMD , JENKINS_JAVA_OPTIONS, JENKINS_HOME or JENKINS_WAR.
It can't be solved on line 53: if it's quoted on line 53, since it's used without quotes on line 78. If you look at most of other scripts written in this way, you'll see that it's a common pattern to use some wrapper to start the command (like I wasn't able to find any way to set JENKINS_JAVA_OPTS to some value which has spaces in it without using eval on line 78. notice: JENKINS_JAVA_OPTS has to be a string, it can't be an array (and there's no way to change this, at least without dropping Systemd env file and switching to |
Can you elaborate this? I may be missing something, but won't this work as the last line: $JENKINS_JAVA_CMD "$JENKINS_JAVA_OPTIONS" -DJENKINS_HOME=$JENKINS_HOME -jar $JENKINS_WAR "${PARAMS[@]}" Simple example of why I think this works: $ cat test.sh
#!/bin/bash
set -x
echo "$BLA"
$ BLA='"abc" "def"' ./test.sh
+ echo '"abc" "def"'
"abc" "def"
$ BLA="'abc' 'def'" ./test.sh
+ echo ''\''abc'\'' '\''def'\'''
'abc' 'def' |
here's when it doesn't work:
|
@ekohl was that example sufficient? |
I'll be mostly offline for the next 2 weeks. Some other reviewer should feel free to jump in. |
It's required to support escaping spaces in JENKINS_JAVA_CMD, JENKINS_JAVA_OPTIONS, JENKINS_HOME and JENKINS_WAR variables.
Made obsolete in #1035 |
It's required to support escaping spaces in JENKINS_JAVA_CMD, JENKINS_JAVA_OPTIONS, JENKINS_HOME and JENKINS_WAR variables.
For example, it's impossible to set
hudson.model.DirectoryBrowserSupport.CSP
without this, since CSP policy has to contain spaces, and escaping spaces wasn't possible inJENKINS_JAVA_OPTS
.