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

Add plugin information for Verbose mode #18051

Merged
merged 8 commits into from May 10, 2016
Merged

Conversation

gmoskovicz
Copy link
Contributor

Fix for #16375

With this Pull Request, we will show extra information about plugins running the bin/elasticsearch-plugin command under verbose mode.

@gmoskovicz
Copy link
Contributor Author

@jasontedor @nik9000 Can you please verify this PR?

@jasontedor
Copy link
Member

Sure, I would be happy to give it a review.

catch(IOException ex) {
terminal.println(Terminal.Verbosity.VERBOSE, "Plugin properties file missing for plugin");
}
catch (Exception ex){
Copy link
Member

Choose a reason for hiding this comment

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

I dont think we should add leniency here. This will only hide the actual problem. Let the exception propagate.

Copy link
Member

Choose a reason for hiding this comment

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

This exception should immediately bubble up to the user. Catch it, add which plugin it died on with this as the cause and throw.

@clintongormley clintongormley added >enhancement :Core/Infra/Plugins Plugin API and infrastructure labels Apr 29, 2016
@gmoskovicz
Copy link
Contributor Author

@rjernst @jasontedor

Fixed code review comments. Please note that now, if the .description file is missing OR it is not supported (wrong JVM/ES version), we will raise an exception when running the bin/elasticsearch-plugin list command, no matter if using -v or not.

@gmoskovicz
Copy link
Contributor Author

The new format looks like

➜  elasticsearch-5.0.0-alpha2-SNAPSHOT bin/elasticsearch-plugin list -v
Plugins directory: /Users/Gabriel/Documents/ElasticSearch/elasticsearch-5.0.0-alpha2-SNAPSHOT/plugins
license
- Plugin information:
Name: license
Description: Internal Elasticsearch Licensing Plugin
Version: 2.2.0
 * Classname: org.elasticsearch.license.plugin.LicensePlugin
marvel-agent
- Plugin information:
Name: marvel-agent
Description: Elasticsearch Marvel Agent
Version: 2.2.0
 * Classname: org.elasticsearch.marvel.MarvelPlugin

@gmoskovicz
Copy link
Contributor Author

@rjernst @jasontedor does it looks better now? Ready to merge?

String output = terminal.getOutput();

//Should be 3 plugins, so if i split by plugin informations i should get 4 strings
assertTrue(output, output.split("Plugin information").length == 4);
Copy link
Member

Choose a reason for hiding this comment

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

I don't like assertTrue style assertions as the message when the assertion fails is useless. The matchers give much more useful information when an assertion fails. For example:

assertThat(output.split("Plugin information"), arrayWithSize(4));

That comment applies to all uses of assertTrue and assertFalse here.

Copy link
Member

Choose a reason for hiding this comment

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

I disagree with this comment. I think using assertTrue and assertFalse is just fine. The message is not useless when you add eg the output like is done here. No matter what the message says, someone looking into a test failure needs to look at the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe i can change all the messages from the class?

I was actually trying to follow the convention on the existing tests in this class. Although it seems that this class was using old/wrong formats?

Copy link
Member

@jasontedor jasontedor May 2, 2016

Choose a reason for hiding this comment

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

The message is not useless when you add eg the output like is done here.

This I disagree with. The output is just the contents of output, that doesn't say what failed. You get that for free using the matchers and I find it valuable.

No matter what the message says, someone looking into a test failure needs to look at the test.

This I agree with.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will change some of the assertions to use assertEquals. The message is actually much better than assert True.

@jasontedor
Copy link
Member

does it looks better now? Ready to merge?

I left more feedback.

@gmoskovicz
Copy link
Contributor Author

Added more fixes @jasontedor

IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> {
listPlugins(env);
});
assertTrue(e.getMessage(), e.getMessage().contains("Property [name] is missing in"));
Copy link
Member

Choose a reason for hiding this comment

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

Can we convert this off of assertTrue, the error message is useless?

@jasontedor
Copy link
Member

Added more fixes @jasontedor

Left some comments.

@gmoskovicz
Copy link
Contributor Author

Thanks @jasontedor . Fixed those as well.

Ready to :shipit: ? 😄

IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> {
listPlugins(env);
});
assertEquals(e.getMessage(), "Property [name] is missing in [" + env.pluginsFile().resolve("fake1").resolve(PluginInfo.ES_PLUGIN_PROPERTIES).toString() + "]");
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it might be too long for the line-length limit?

Environment env = createEnv();
PluginTestUtil.writeProperties(env.pluginsFile().resolve("fake1"),
"description", "fake desc");
IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> {
Copy link
Member

Choose a reason for hiding this comment

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

This lambda does not need to be a statement block.

@jasontedor
Copy link
Member

Ready to :shipit: ? 😄

I left more comments. Can you run gradle check?

@gmoskovicz
Copy link
Contributor Author

@jasontedor Gradle check was successful .

@jasontedor jasontedor merged commit 0660386 into elastic:master May 10, 2016
@jasontedor
Copy link
Member

LGTM. Thanks @gmoskovicz!

@gmoskovicz
Copy link
Contributor Author

Sweet! Thanks for all the help @jasontedor !

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

Successfully merging this pull request may close these issues.

None yet

4 participants