Plugin script: Fix ES_HOME with spaces

[nik9000]

When ES_HOME has spaces we get funky "cannot find main class directory>"
errors. This makes them stop by escaping directories and using eval instead
of exec.

Closes #12504

[nik9000]

I'm not sure if this is 1.7.2 or 1.7.1.

[electrical]

@tlrx @spinscale could you guys review this? thanks!

[jaymode]

I tested this out in a directory with spaces and this fixes the issue described in the ticket. If we also have a space in one of the parameters like the plugin URL we still have an issue in that the parameter is split into two by the space.

bin/plugin install plugin -u "file:///path/with space/plugin.zip"

It will try to install space/plugin.zip

The fix is similar to what's already been done here and it would be awesome to get it included now:

# real getopt cannot be used because we need to hand options over to the PluginManager
while [ $# -gt 0 ]; do
  case $1 in
    -D*=*)
      properties="$properties \"$1\""
      ;;
    -D*)
      var="$1"
      shift
      properties="$properties \"$var\"=\"$1\""
      ;;
    *)
      args="$args \"$1\""
  esac
  shift
done

@nik9000 what do you think? If you'd prefer to keep it separate, we can and I can open up a different PR.

[nik9000]

@nik9000 what do you think? If you'd prefer to keep it separate, we can and I can open up a different PR.

It makes sense to me. This isn't really my area of expertise but I'm happy to add it.

BTW, this is against the 1.7 branch and I'm not sure that this is still an issue in 2.0. Is this another one of things we should just wait on 2.0 to fix?

[jaymode]

I also tested with 2.0 and it is still an issue there as well, so we definitely need to fix it in master.

[nik9000]

Cool. In that case I'll grab this issue in a bit and fix the spaces in
plugin paths issue. I'll also look at adding some bats tests. Then forward
port to 2.0.....

On Thu, Jul 30, 2015 at 10:17 AM, Jay Modi notifications@github.com wrote:

I also tested with 2.0 and it is still an issue there as well, so we
definitely need to fix it in master.

—
Reply to this email directly or view it on GitHub
#12508 (comment)
.

[nik9000]

Swap review for WIP because I have to add another fix and some bats tests.

[nik9000]

@tlrx do you have any special setup that you do to run bats? The bats stuff in 1.7 claims to be very destructive to you system so I spun up an ubuntu vm to test it in.

[nik9000]

Ok - ready for review I think.

[bookshelfdave]

$ES_DIR/config is intentionally not removed? might be worth a comment in the test

[nik9000]

Honestly? I have no idea - this is the same checks that are done when the installation directory doesn't have a space. I'd love there to be some method call for this but the other tests seemed to prefer copy and paste over a refactor and I was afraid to do it - especially as I'll have to forward port this to 1.7 and my bash-foo is weak.

[nik9000]

"I just copy and pasted like the last guy" <--- its a poor defense but in this case I'm sticking to it.

[tlrx]

$ES_DIR/config is intentionally not removed? might be worth a comment in the test

Configuration is not removed when removing a plugin. I think elasticsearch configuration is removed when purging the package.

[jaymode]

LGTM. Built and ran it in a directory with spaces and used a path with spaces as well.

[tlrx]

@tlrx do you have any special setup that you do to run bats? The bats stuff in 1.7 claims to be very destructive to you system so I spun up an ubuntu vm to test it in.

Yeah, always use a vm to run the tests! They install/remove packages, user and dirs. See https://github.com/elastic/elasticsearch/blob/master/TESTING.asciidoc#testing-scripts

[tlrx]

LGTM

BATS tests succeed on Debian 7.8, Debian 8, CentOS 6.6, CentOS 7, Ubuntu 12.04 and Ubuntu 15.04

[electrical]

LGTM. ready to merge i think.

[nik9000]

BATS tests succeed on Debian 7.8, Debian 8, CentOS 6.6, CentOS 7, Ubuntu 12.04 and Ubuntu 15.04

That is a good list.