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

Output plugin info only in verbose mode #12908

Closed
wants to merge 1 commit into from

Conversation

kubum
Copy link
Contributor

@kubum kubum commented Aug 15, 2015

Hi

Moved output to verbose level and additionally changed the plugin info output format

Before:

PluginInfo{name='cloud-aws', description='The Amazon Web Service (AWS) Cloud plugin allows to use AWS API for the unicast discovery mechanism and add S3 repositories.', site=false, jvm=true, classname=org.elasticsearch.plugin.cloud.aws.CloudAwsPlugin, isolated=true, version='2.1.0-SNAPSHOT'}

After:

- Plugin information:
Name: cloud-aws
Description: The Amazon Web Service (AWS) Cloud plugin allows to use AWS API for the unicast discovery mechanism and add S3 repositories.
Site: false
Version: 2.1.0-SNAPSHOT
JVM: true
* Classname: org.elasticsearch.plugin.cloud.aws.CloudAwsPlugin
* Isolated: true

Fixes #12907

What do you think @clintongormley?

@nik9000
Copy link
Member

nik9000 commented Aug 15, 2015

LGTM. I'm a little weary of making toString's multline line but I think its fine for this.

@kubum
Copy link
Contributor Author

kubum commented Aug 15, 2015

@nik9000 I also thought about this, maybe better to extract put multiline info to some kind of displayInfo method and leave toString as it is?

@nik9000
Copy link
Member

nik9000 commented Aug 15, 2015

@nik9000 I also thought about this, maybe better to extract put multiline info to some kind of displayInfo method and leave toString as it is?

The toString was already super long so it'd be hard to use in debugging. I'm fine with the pr as it stands. If someone is debugging and the toString is a problem they can change it.

That gets me to thinking - can you think of any way to test this? We can certainly add something to the bats tests that we run in the qa/vagrant repository. Those are undergoing major surgery right now so it might be simpler to just create a followup issue to test this with bats.

@clintongormley clintongormley changed the title Fixes #12907 - Plugin info verbose Output plugin info only in verbose mode Aug 17, 2015
@kubum
Copy link
Contributor Author

kubum commented Aug 17, 2015

Hi @nik9000

Ah, I didn't know that there are bash tests. Is this file 50_plugins.bats would be a good place for output test? I think I can figure out how to do add test for it.

However, if you are sure we can just do it in a separate issue.

@nik9000
Copy link
Member

nik9000 commented Aug 17, 2015

However, if you are sure we can just do it in a separate issue.

Go ahead and add them - I'm in the middle of refactoring them now but we can handle a merge if we have to. Just try to keep your change confined so its easier to merge.

TESTING.asciidoc has some information on the best ways to run those tests. They are reasonably destructive so its best to run them in a VM which TESTING.asciidoc describes how to do.

@kubum
Copy link
Contributor Author

kubum commented Aug 23, 2015

Hi @nik9000

I tried to run tests today from master and unfortunately they are failing.

Could you please advice something on this?

First of all, all tests use -u param and fail with:

ERROR: Unrecognized option: -u

I removed it and tried to run again:

 ✗ [PLUGINS] install jvm-example plugin
   (in test file /elasticsearch/qa/vagrant/src/test/resources/packaging/scripts/50_plugins.bats, line 69)
     `[ "$status" -eq 0 ]' failed
   -> Installing jvm-example... Trying http://download.elastic.co/elasticsearch/release/org/elasticsearch/plugin/jvm-example/2.1.0/jvm-example-2.1.0.zip ... Failed: IOException[Can't get http://download.elastic.co/elasticsearch/release/org/elasticsearch/plugin/jvm-example/2.1.0/jvm-example-2.1.0.zip to /tmp/jvm-example8389380809329658167.zip]; nested: FileNotFoundException[http://download.elastic.co/elasticsearch/release/org/elasticsearch/plugin/jvm-example/2.1.0/jvm-example-2.1.0.zip]; nested: FileNotFoundException[http://download.elastic.co/elasticsearch/release/org/elasticsearch/plugin/jvm-example/2.1.0/jvm-example-2.1.0.zip]; ERROR: failed to download out of all possible locations..., use --verbose to get detailed information

Apparently resource is not available anymore. Is there a plan to move it? or use from somewhere else?

I just wanted to add a regexp to check the verbose and non-verbose output of the jvm plugin install :)

@nik9000
Copy link
Member

nik9000 commented Aug 24, 2015

Could you please advice something on this?

I'll have a look. Right now those tests aren't yet run as part of everyone's development process so they might not be working properly.

@nik9000
Copy link
Member

nik9000 commented Aug 25, 2015

I'll have a look. Right now those tests aren't yet run as part of everyone's development process so they might not be working properly.

Bleh - ok I have a fix proposed for this in #13076 but its a larger change than just fixing the -u issue. Bleh. Sorry. Lets not hold this up for that.

I think I still need another review if I can get it - @tlrx, do you have an opinion on this?

@nik9000 nik9000 added v2.1.0 and removed v2.0.0 labels Aug 25, 2015
@nik9000
Copy link
Member

nik9000 commented Aug 25, 2015

Swapped the v2.0.0 out for v2.1.0 because we're trying to stabilize 2.0 and don't want to push too much to it.

@nik9000 nik9000 self-assigned this Aug 25, 2015
@nik9000
Copy link
Member

nik9000 commented Aug 25, 2015

Verified that it looks as good as @kubum says it does:

$ ./bin/plugin install jvm-example -v -u file:///elasticsearch/qa/vagrant/target/testroot/elasticsearch-jvm-example-2.1.0-SNAPSHOT.zip
-> Installing jvm-example...
Trying file:/elasticsearch/qa/vagrant/target/testroot/elasticsearch-jvm-example-2.1.0-SNAPSHOT.zip ...
Downloading .DONE
Verifying file:/elasticsearch/qa/vagrant/target/testroot/elasticsearch-jvm-example-2.1.0-SNAPSHOT.zip checksums if available ...
NOTE: Unable to verify checksum for downloaded plugin (unable to find .sha1 or .md5 file to verify)
- Plugin information:
Name: jvm-example
Description: Demonstrates all the pluggable Java entry points in Elasticsearch
Site: false
Version: 2.1.0-SNAPSHOT
JVM: true
 * Classname: org

@nik9000
Copy link
Member

nik9000 commented Sep 2, 2015

Verified that it looks as good as @kubum says it does:

Ok - I just forgot about this for a week. Sorry! Right now the vagrant testing is much more under control. If you rebase on elastic's master branch you should be able to work on tests for this if you want. If you don't want to try your hand at bats testing then I think this is fine to merge and I'll write the tests in a followup pull request.

@kubum
Copy link
Contributor Author

kubum commented Sep 7, 2015

Hi @nik9000

I tried to run master bats tests and got

vagrant@vagrant-ubuntu-trusty-64:/elasticsearch/qa/vagrant/target/testroot$ sudo bats $BATS/*.bats
bats: /elasticsearch/qa/vagrant/src/test/resources/packaging/scripts/25_tar_plugins.bats does not exist
/usr/libexec/bats-exec-suite: line 20: let: count+=: syntax error: operand expected (error token is "+=")
vagrant@vagrant-ubuntu-trusty-64:/elasticsearch/qa/vagrant/target/testroot$ ls -la /elasticsearch/qa/vagrant/src/test/resources/packaging/scripts/
total 76
drwxr-xr-x 1 vagrant vagrant   510 Sep  7  2015 .
drwxr-xr-x 1 vagrant vagrant   102 Aug 11 20:50 ..
-rw-r--r-- 1 vagrant vagrant  2590 Sep  7 22:18 20_tar_package.bats
lrwxr-xr-x 1 vagrant vagrant    22 Sep  7  2015 25_tar_plugins.bats -> plugin_test_cases.bash
-rw-r--r-- 1 vagrant vagrant  4625 Sep  7 22:18 30_deb_package.bats
-rw-r--r-- 1 vagrant vagrant  3506 Sep  7 22:18 40_rpm_package.bats
lrwxr-xr-x 1 vagrant vagrant    22 Sep  7  2015 50_plugins.bats -> plugin_test_cases.bash
-rw-r--r-- 1 vagrant vagrant  5850 Sep  7 22:29 50_plugins_.bats
-rw-r--r-- 1 vagrant vagrant  3692 Sep  7 22:18 60_systemd.bats
-rw-r--r-- 1 vagrant vagrant  2704 Sep  7 22:18 70_sysv_initd.bats
-rw-r--r-- 1 vagrant vagrant  3480 Sep  7 22:18 80_upgrade.bats
-rw-r--r-- 1 vagrant vagrant 14479 Sep  7 22:18 packaging_test_utils.bash
-rw-r--r-- 1 vagrant vagrant  5850 Sep  7 22:18 plugin_test_cases.bash
-rw-r--r-- 1 vagrant vagrant  3541 Sep  7 22:18 plugins.bash
-rw-r--r-- 1 vagrant vagrant  2545 Sep  7 22:18 tar.bash

Apparently it's not really working well with symlinks? I was following the TESTING.asciidoc. Do you have any ideas where is the issue?

@nik9000
Copy link
Member

nik9000 commented Sep 9, 2015

Apparently it's not really working well with symlinks? I was following the TESTING.asciidoc. Do you have any ideas where is the issue?

Hmmm - I'm not sure.

What is your host OS?

What happens when you sudo bats $BATS/25_tar_plugins.bats?

Master isn't going to work properly without #13422 because master has jumped to Java 8.

I'd be fine merging as is and filing a second issue for the bats test if you are fine with it.

@kubum
Copy link
Contributor Author

kubum commented Sep 9, 2015

Hi @nik9000

I'm totally happy if it can be merged as it is. Just wanted to make it better :) but probably better to do it as part of the second issue.

I did it in vagrant trusty box.

@nik9000
Copy link
Member

nik9000 commented Sep 10, 2015

I did it in vagrant trusty box.

What is the host OS? Like, what OS is running vagrant/virtuabox? I'm wondering what might be up with the symlinks.

@kubum
Copy link
Contributor Author

kubum commented Sep 13, 2015

Ah, sorry.

It's Mac OS X Yosemite 10.10.3.
Vagrant 1.7.4.
VirtualBox 5.0.2r102096

Hope this helps

@nik9000
Copy link
Member

nik9000 commented Sep 14, 2015

Hope this helps

Its what I asked for but it doesn't help! Its pretty much the same as what I have. As much as I'd love to get this working for you I think the right thing here is to just merge as is and create a followup issue for checking the output. I'll try and do both of those today.

As far as why the tests don't work for you - I'm not sure. They seem to work fine for other folks that tried them. The symlink is happy which is wonderful. I'm not sure what's up.

PluginManager plugin info printing
nik9000 added a commit that referenced this pull request Sep 14, 2015
#12908

Moved output to verbose level and additionally changed the plugin info output format

Before:

```shell
PluginInfo{name='cloud-aws', description='The Amazon Web Service (AWS) Cloud plugin allows to use AWS API for the unicast discovery mechanism and add S3 repositories.', site=false, jvm=true, classname=org.elasticsearch.plugin.cloud.aws.CloudAwsPlugin, isolated=true, version='2.1.0-SNAPSHOT'}
```

After:

```shell
- Plugin information:
Name: cloud-aws
Description: The Amazon Web Service (AWS) Cloud plugin allows to use AWS API for the unicast discovery mechanism and add S3 repositories.
Site: false
Version: 2.1.0-SNAPSHOT
JVM: true
* Classname: org.elasticsearch.plugin.cloud.aws.CloudAwsPlugin
* Isolated: true
```

Fixes #12907
@nik9000 nik9000 closed this Sep 14, 2015
@nik9000
Copy link
Member

nik9000 commented Sep 14, 2015

Merged to master. I'm backporting it to 2.x right now.

I have no idea why github used the red closed symbol rather than the merged symbol. Its merged! Well, half way.

@nik9000
Copy link
Member

nik9000 commented Sep 14, 2015

Thanks for writing this @kubum !

@nik9000
Copy link
Member

nik9000 commented Sep 14, 2015

And merged to 2.x. All done!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants