-
Notifications
You must be signed in to change notification settings - Fork 463
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
[WFCORE-5241] in jboss-cli.sh enclose the JAVA_OPTS with double quote to escape whitespace #4452
Conversation
hmmm. please HOLD. There are test failures from CliScriptTestCase |
@soul2zimate , yes I approved too quickly...JAVA_OPTS is expected to contain multiple argument, and we can't ignore these white spaces. |
OK, so the test failed due to spaces between arguments in JAVA_OPTS when it get double quoted. |
@soul2zimate , I think that we need to keep exec. That has allowed us to resist to CLI script being patched during patch execution. |
b9e7d40
to
5c9efdc
Compare
I have updated the PR to convert JAVA_OPTS to an array in order to quote each element, the CI seem good now. |
@soul2zimate , very good! Would you mind evolving https://github.com/wildfly/wildfly-core/blob/master/testsuite/scripts/src/test/java/org/wildfly/scripts/test/CliScriptTestCase.java with a testcase? Thank-you. |
5c9efdc
to
077d6f5
Compare
script.start(MAVEN_JAVA_OPTS, "--commands=embed-server,:read-attribute(name=server-state),exit"); | ||
Map<String, String> env = new LinkedHashMap<>(MAVEN_JAVA_OPTS); | ||
// https://issues.redhat.com/browse/WFCORE-5241 Test with JAVA_OPTS parameter including whitespace | ||
env.put("JAVA_OPTS", "-Dtestparameter=\"something with space\""); |
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.
@soul2zimate I think you should check if JAVA_OPTS already exists and append to it. You can look at: https://github.com/wildfly/wildfly-core/blob/master/testsuite/scripts/src/test/java/org/wildfly/scripts/test/ScriptTestCase.java#L86
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.
hmmm, that didn't show at local run. I reckon teamcity have something more configured than my local one.
I pushed an update to check any existing JAVA_OPTS first.
077d6f5
to
d6aa433
Compare
Core - Full Integration Build 10261 outcome was FAILURE using a merge of d6aa433 Failed tests
|
d6aa433
to
0a1dee4
Compare
OK, Windows - JDK 11 result gives some weird outputs for CliScriptTestCase.testBatchScript
However, I don't have a Windows box to debug it. As the fix only applies to jboss-cli.sh, I excluded the test for Windows. |
@soul2zimate thank-you. @jmesnil , you can remove the hold label. |
… each element to allow exec them with whitespace inside
0a1dee4
to
db82190
Compare
@soul2zimate thanks |
Issue: https://issues.redhat.com/browse/WFCORE-5241
[WFCORE-5241] in jboss-cli.sh enclose the JAVA_OPTS with double quote to escape whitespace