You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jclouds.apache.org by Ignasi Barrera <no...@github.com> on 2013/09/16 18:17:55 UTC
[jclouds] JCLOUDS-283: Don't use bash shopt in the wrapper script
(#150)
Don't use bash `shopt` in the wrapper script, as some images with a bash shell don't support it. See [JCLOUDS-283](https://issues.apache.org/jira/browse/JCLOUDS-283) for the details about the motivation and the applied changes.
I've tested the change with jclouds-chef (which uses a quite complex wrapped script) and succesfully deployed a Wordpress in EC2 with an Ubuntu AMI (us-east-1/ami-d0f89fb9). The chef bootstrap script makes use of the aliases that have been transformed to a function, so the applied changes shouldn't break existing code.
@zack-shoylev, @everett-toews (or any OpenStack ninja :D) could you try this PR with a `cirrus` image or any other that used to fail?
You can merge this Pull Request by running:
git pull https://github.com/nacx/jclouds 283-remove-shopt
Or you can view, comment on it, or merge it online at:
https://github.com/jclouds/jclouds/pull/150
-- Commit Summary --
* JCLOUDS-283: Don't use bash shopt in the wrapper script
-- File Changes --
M compute/src/test/resources/initscript_with_java.sh (18)
M compute/src/test/resources/initscript_with_jetty.sh (18)
M compute/src/test/resources/runscript.sh (18)
M compute/src/test/resources/runscript_adminUpdate.sh (4)
M scriptbuilder/src/main/java/org/jclouds/scriptbuilder/domain/ShellToken.java (2)
M scriptbuilder/src/main/resources/functions/setupPublicCurl.sh (14)
M scriptbuilder/src/test/java/org/jclouds/scriptbuilder/domain/ShellTokenTest.java (18)
M scriptbuilder/src/test/resources/test_ebs.sh (4)
M scriptbuilder/src/test/resources/test_find_pid.sh (2)
M scriptbuilder/src/test/resources/test_init.sh (4)
M scriptbuilder/src/test/resources/test_init_script.sh (4)
M scriptbuilder/src/test/resources/test_install_chef_gems_scriptbuilder.sh (4)
M scriptbuilder/src/test/resources/test_install_git_scriptbuilder.sh (18)
M scriptbuilder/src/test/resources/test_install_jdk_scriptbuilder.sh (18)
M scriptbuilder/src/test/resources/test_install_ruby_scriptbuilder.sh (18)
M scriptbuilder/src/test/resources/test_install_rubygems_scriptbuilder.sh (4)
M scriptbuilder/src/test/resources/test_runrun.sh (2)
M scriptbuilder/src/test/resources/test_runrun_header.sh (2)
M scriptbuilder/src/test/resources/test_script.sh (2)
M scriptbuilder/src/test/resources/test_seek_and_destroy.sh (2)
-- Patch Links --
https://github.com/jclouds/jclouds/pull/150.patch
https://github.com/jclouds/jclouds/pull/150.diff
Re: [jclouds] JCLOUDS-283: Don't use bash shopt in the wrapper script
(#150)
Posted by Adrian Cole <no...@github.com>.
yeah. what he said :)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/150#issuecomment-24703817
Re: [jclouds] JCLOUDS-283: Don't use bash shopt in the wrapper script
(#150)
Posted by Ignasi Barrera <no...@github.com>.
You can disable the wrapper script if you use the RunScriptOptions (and perhaps install bash this way).
But I totally agree with you. I think we can close the issue as Invalid and open a new one to refactor the entire scriptbuilder stuff to use the `sh` shell. It is going to be a bit more epic/risky, but the refactor should not be very difficult.
If you agree, I'll merge this pull request (because thinking again the alias-to-function change will come in handy in `sh`; it also has functions but they are declared in a different way, and we don't need to worry about aliases), close the issue, open the new one and start working on the refactor.
WDYT?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/150#issuecomment-24694034
Re: [jclouds] JCLOUDS-283: Don't use bash shopt in the wrapper script
(#150)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-pull-requests #228](https://jclouds.ci.cloudbees.com/job/jclouds-pull-requests/228/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/150#issuecomment-24526955
Re: [jclouds] JCLOUDS-283: Don't use bash shopt in the wrapper script
(#150)
Posted by Adrian Cole <no...@github.com>.
debate seems to continue on irc :) Seems the value of risking many jclouds tools to support a couple edge case images is questionable. Perhaps we can think about this the other way.. flag to support sh? Also, probably best that such as request came from an end user.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/150#issuecomment-24695194
Re: [jclouds] JCLOUDS-283: Don't use bash shopt in the wrapper script
(#150)
Posted by Zack Shoylev <no...@github.com>.
I will give it a try later today or tomorrow.
Thanks!!
Zack
________________________________
From: Ignasi Barrera [notifications@github.com]
Sent: Monday, September 16, 2013 11:17 AM
To: jclouds/jclouds
Cc: Zack Shoylev
Subject: [jclouds] JCLOUDS-283: Don't use bash shopt in the wrapper script (#150)
Don't use bash shopt in the wrapper script, as some images with a bash shell don't support it. See JCLOUDS-283<https://issues.apache.org/jira/browse/JCLOUDS-283> for the details about the motivation and the applied changes.
I've tested the change with jclouds-chef (which uses a quite complex wrapped script) and succesfully deployed a Wordpress in EC2 with an Ubuntu AMI (us-east-1/ami-d0f89fb9). The chef bootstrap script makes use of the aliases that have been transformed to a function, so the applied changes shouldn't break existing code.
@zack-shoylev<https://github.com/zack-shoylev>, @everett-toews<https://github.com/everett-toews> (or any OpenStack ninja :D) could you try this PR with a cirrus image or any other that used to fail?
________________________________
You can merge this Pull Request by running
git pull https://github.com/nacx/jclouds 283-remove-shopt
Or view, comment on, or merge it at:
https://github.com/jclouds/jclouds/pull/150
Commit Summary
* JCLOUDS-283: Don't use bash shopt in the wrapper script
File Changes
* M compute/src/test/resources/initscript_with_java.sh<https://github.com/jclouds/jclouds/pull/150/files#diff-0> (18)
* M compute/src/test/resources/initscript_with_jetty.sh<https://github.com/jclouds/jclouds/pull/150/files#diff-1> (18)
* M compute/src/test/resources/runscript.sh<https://github.com/jclouds/jclouds/pull/150/files#diff-2> (18)
* M compute/src/test/resources/runscript_adminUpdate.sh<https://github.com/jclouds/jclouds/pull/150/files#diff-3> (4)
* M scriptbuilder/src/main/java/org/jclouds/scriptbuilder/domain/ShellToken.java<https://github.com/jclouds/jclouds/pull/150/files#diff-4> (2)
* M scriptbuilder/src/main/resources/functions/setupPublicCurl.sh<https://github.com/jclouds/jclouds/pull/150/files#diff-5> (14)
* M scriptbuilder/src/test/java/org/jclouds/scriptbuilder/domain/ShellTokenTest.java<https://github.com/jclouds/jclouds/pull/150/files#diff-6> (18)
* M scriptbuilder/src/test/resources/test_ebs.sh<https://github.com/jclouds/jclouds/pull/150/files#diff-7> (4)
* M scriptbuilder/src/test/resources/test_find_pid.sh<https://github.com/jclouds/jclouds/pull/150/files#diff-8> (2)
* M scriptbuilder/src/test/resources/test_init.sh<https://github.com/jclouds/jclouds/pull/150/files#diff-9> (4)
* M scriptbuilder/src/test/resources/test_init_script.sh<https://github.com/jclouds/jclouds/pull/150/files#diff-10> (4)
* M scriptbuilder/src/test/resources/test_install_chef_gems_scriptbuilder.sh<https://github.com/jclouds/jclouds/pull/150/files#diff-11> (4)
* M scriptbuilder/src/test/resources/test_install_git_scriptbuilder.sh<https://github.com/jclouds/jclouds/pull/150/files#diff-12> (18)
* M scriptbuilder/src/test/resources/test_install_jdk_scriptbuilder.sh<https://github.com/jclouds/jclouds/pull/150/files#diff-13> (18)
* M scriptbuilder/src/test/resources/test_install_ruby_scriptbuilder.sh<https://github.com/jclouds/jclouds/pull/150/files#diff-14> (18)
* M scriptbuilder/src/test/resources/test_install_rubygems_scriptbuilder.sh<https://github.com/jclouds/jclouds/pull/150/files#diff-15> (4)
* M scriptbuilder/src/test/resources/test_runrun.sh<https://github.com/jclouds/jclouds/pull/150/files#diff-16> (2)
* M scriptbuilder/src/test/resources/test_runrun_header.sh<https://github.com/jclouds/jclouds/pull/150/files#diff-17> (2)
* M scriptbuilder/src/test/resources/test_script.sh<https://github.com/jclouds/jclouds/pull/150/files#diff-18> (2)
* M scriptbuilder/src/test/resources/test_seek_and_destroy.sh<https://github.com/jclouds/jclouds/pull/150/files#diff-19> (2)
Patch Links:
* https://github.com/jclouds/jclouds/pull/150.patch
* https://github.com/jclouds/jclouds/pull/150.diff
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/150#issuecomment-24524594
Re: [jclouds] JCLOUDS-283: Don't use bash shopt in the wrapper script
(#150)
Posted by Ignasi Barrera <no...@github.com>.
Yes, we can keep that. See the issue description in JIRA:
>**xpg_echo:** Turns on/off the interpretation of the backslash (to interpret or not the '\n', for example).
**expand_aliases:** Turns on/off the alias expanding. This allows to define alias in the script and use them alter.
>
>The first one can be removed, if all calls to the "echo" command that need to interpret the backslash are changed to "echo -e".
The second one, could be removed if the alias definitions are replaced by bash functions, which should be a direct change.
I changed the aliases to functions, but there was no need to change any echo. I checked all existing scripts and there were no echo commands that used the backslash to format the output.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/150#issuecomment-24650315
Re: [jclouds] JCLOUDS-283: Don't use bash shopt in the wrapper script
(#150)
Posted by Ignasi Barrera <no...@github.com>.
Thinking a bit more about this, I think it is not a good idea to change from bash:
* Moving to a different shell may massively break the script (that can be complex) what users have built. It can be too painful for the users to update all the scripts to get them working again.
* The existing predefined helper functions in the scriptbuilder project assume certain distributions. They rely on apt or yum to install software, for example, and that only works on Debian based or Red Hat/CentOS/Fedoras, etc, and all those distributions come with a bash shell. If someone wants to use a different one, most of those utilities won't work, so the best approach for them would be to build the entire script and run it without wrapping.
* If we want to support several shells (and let users configure the one to use) we need to either have the script fragments predefined for each shell, or have a template system that generates the appropriate syntax (something similar to what the ShellToken class does in a very simple way to generate Windows scripts), but I really don't like to rely on this. Apart from introducing a new (and important) challenge to implement (or delegate to an external tool), users would have to "learn" our template system so they can build portable scripts.
Given all this points, I think we should not change the current implementation (and don't merge this PR), and stick to bash. If someday a user requests certain features to be available for a different shell (say the AdminAccess related scripts) we can always "translate! that concrete scripts and make them available by other means, or just as a new independent statement.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/150#issuecomment-24700795
Re: [jclouds] JCLOUDS-283: Don't use bash shopt in the wrapper script
(#150)
Posted by Ignasi Barrera <no...@github.com>.
We are in front of a different issue then. It seems that the image does not come with the bash shell, and the sh one does not have the 'function' keyword. All the scripts assume a bash shell, so, as the issue appeared because cirrus comes with a different one, I see no point in merging this change.
WDYT?
Perhaps this can be bypassed by making two calls to the runScriptOnNode. The first one may run a command to install bash (without the wrapper script to avoid the mentioned failure), and the second call to run the desired stuff (such as chef or whatever). Could you try this too?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/150#issuecomment-24692063
Re: [jclouds] JCLOUDS-283: Don't use bash shopt in the wrapper script
(#150)
Posted by Andrew Phillips <no...@github.com>.
> but there was no need to change any echo
Thanks for explaining - that's what I was after ;-)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/150#issuecomment-24652818
Re: [jclouds] JCLOUDS-283: Don't use bash shopt in the wrapper script
(#150)
Posted by Zack Shoylev <no...@github.com>.
I was able to test this against a cirros image.
It works better - does not fail on shopt.
Now it seems to fails on:
function abort {
...
It seems to not support "function":
Status: 1
Error?:
Output: sh: function: not found
aborting:
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/150#issuecomment-24690635
Re: [jclouds] JCLOUDS-283: Don't use bash shopt in the wrapper script
(#150)
Posted by Andrew Phillips <no...@github.com>.
How will losing `xpg_echo` affect us? Is there any way to keep this behaviour without `shopt`..?
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/150#issuecomment-24650105
Re: [jclouds] JCLOUDS-283: Don't use bash shopt in the wrapper script
(#150)
Posted by Adrian Cole <no...@github.com>.
per IRC, probably best to target bin/sh as default in 1.7 provided there's a compatibility flag of some sort that allows old behavior (bash).
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/150#issuecomment-24694675
Re: [jclouds] JCLOUDS-283: Don't use bash shopt in the wrapper script
(#150)
Posted by CloudBees pull request builder plugin <no...@github.com>.
[jclouds-java-7-pull-requests #689](https://jclouds.ci.cloudbees.com/job/jclouds-java-7-pull-requests/689/) SUCCESS
This pull request looks good
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/150#issuecomment-24526662
Re: [jclouds] JCLOUDS-283: Don't use bash shopt in the wrapper script
(#150)
Posted by Adrian Cole <no...@github.com>.
on the surface, moving off bash to sh makes sense.. devil might be in details, so probably best to really test it (like with whirr, pallet, etc)
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/150#issuecomment-24694190
Re: [jclouds] JCLOUDS-283: Don't use bash shopt in the wrapper script
(#150)
Posted by Zack Shoylev <no...@github.com>.
I agree. Let's give it a try first and see if there are any mines on the way.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/150#issuecomment-24694236
Re: [jclouds] JCLOUDS-283: Don't use bash shopt in the wrapper script
(#150)
Posted by Zack Shoylev <no...@github.com>.
I think you are right! It must be lacking bash.
But this leads to another problem:
I believe this function is part of the wrapping script. So how are you going to use ScriptRunner if even simple commands you run fail because of the wrapping? Or am I off on how the script is wrapped?
The other issue:
When using the compute Template and template builder to create a new node, it in turn uses the ScriptRunner for some options: such as authorizing a public key on the image. So in some cases you don't get to run scripts on a node - it does not get created properly.
Also - is it possible for the script runner to stick to the minimum posix standard? (thinking http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html) Or maybe use some kind of detection.
It would be neat if it could work the same on very minimal images.
---
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/150#issuecomment-24692953