Skip to content
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

fix: wrap args with quotes #212

Closed
wants to merge 2 commits into from
Closed

fix: wrap args with quotes #212

wants to merge 2 commits into from

Conversation

michael-wirth
Copy link
Contributor

closes #211

@vorburger
Copy link
Collaborator

vorburger commented Jan 3, 2019 via email

@michael-wirth
Copy link
Contributor Author

Would you be willing to add a non regression test for the problem which this fixes to the ./test.sh?

I added a test to check if all involved scripts/entrypoints correctly handle special command-line arguments

  • java -jar /deployments/app.jar -> works
  • /opt/run-java/run-java.sh -> fails
  • /usr/local/s2i/run -> fails

the test will fail until fabric8io-images/run-java-sh#76 has been fixed and released as well

Copy link
Collaborator

@vorburger vorburger left a comment

Choose a reason for hiding this comment

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

some comments from when I looked over this more. I'll merge it anyway and up to you if you want to follow-up.

echo "APP TEST FAILED (with entrypoint ${entrypoint})"
docker logs ${container_id}
docker rm -f ${container_id}
return -123
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit pick: wrong indent

-e LANG=en_US.UTF-8 -e PARAMETER_THAT_MAY_NEED_ESCAPING="&'\"|< é\\(" ${name} \
${entrypoint} --commandLineArgValueThatMayNeedEscaping="&'\"|< é\\(" --killDelay=1 --exitCode=0)

# sleep is required because after docker run returns, the container is up but our server may not quite be yet
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this left over comment (as it does not sleep here, contrary to orig test)

local entrypoint=$2

local container_id=$(docker run --name ${name}-test -d \
-e LANG=en_US.UTF-8 -e PARAMETER_THAT_MAY_NEED_ESCAPING="&'\"|< é\\(" ${name} \
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious about the use of -e LANG=en_US.UTF-8 here.. is it required just for this particular test, or should be added in general??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

UTF-8 is requieredto pass in arguments with special characters. if you renove it then the test will fail. I'm not sure but I guess its because the test JAR (as 99% of all Jars) was build with UTF-8 encoding)


local container_id=$(docker run --name ${name}-test -d \
-e LANG=en_US.UTF-8 -e PARAMETER_THAT_MAY_NEED_ESCAPING="&'\"|< é\\(" ${name} \
${entrypoint} --commandLineArgValueThatMayNeedEscaping="&'\"|< é\\(" --killDelay=1 --exitCode=0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure what --killDelay=1 --exitCode=0 does / is needed for... docker run or spring-cloud-deployer-spi-test-app-1.3.4.RELEASE-exec.jar arguments? Perhaps worth an inline comment to clarify to future readers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

--killDelay=1 --exitCode=0 are arguments for the test application to define if the the app should be killed after running its test and what exitcode should be returned. i'll add some comments

@vorburger
Copy link
Collaborator

I've locally pulled this as-is and seen ./test.sh fail, and then rebased it on top of #214 and seen it pass; therefore now merged (as 8fb67ed & 11edaf0) despite minor review feedback.

@vorburger vorburger closed this Jan 16, 2019
vorburger added a commit to vorburger/s2i that referenced this pull request Jan 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support command line arg values with spaces
2 participants