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

Added a cli infrastructure #7094

Merged
merged 1 commit into from Aug 2, 2014

Conversation

Projects
None yet
4 participants
@uboness
Copy link
Contributor

commented Jul 30, 2014

CliTool is a base class for command-line interface tools (such as the plugin manager and potentially others). It supports the following:

  • single or multi command tool
  • help printing infrastructure (based on help files)
  • consistent mechanism of parsing arguments (based on commons-cli lib)
  • separation of argument parsing and command execution (for easier unit testing)
  • terminal abstraction (will use System.console() when available)

@spinscale spinscale added the review label Jul 31, 2014

String cmdName = args[0];
cmd = config.cmd(cmdName);
if (cmd == null) {
terminal.printError("unknown command [%s]. Use [-h] option to list available commands", cmdName);

This comment has been minimized.

Copy link
@spinscale

spinscale Jul 31, 2014

Member

possible use of config.printUsage here as well or directly list available commands to be more user friendly?

This comment has been minimized.

Copy link
@uboness

uboness Jul 31, 2014

Author Contributor

thought about it... decided to only leave it with a comment about -h... could very well be that the user just accidentally mistyped and didn't want to make things too verbose


private static void print(Class clazz, String name, final Terminal terminal) {
terminal.println(Terminal.Verbosity.SILENT);
try (InputStream input = clazz.getResourceAsStream(name + HELP_FILE_EXT)) {

This comment has been minimized.

Copy link
@spinscale

spinscale Jul 31, 2014

Member

Is this still fast, when we have a packaged application? Does getResourceAsStream() need to read all classpath jars, until it finds a corresponding file? We should make sure here that response times for command line tools are crazy fast.

This comment has been minimized.

Copy link
@uboness

uboness Jul 31, 2014

Author Contributor

should definitely be fast enough... anyway... for help for sure

*
*/
public interface Callback<T> {

This comment has been minimized.

Copy link
@spinscale

spinscale Jul 31, 2014

Member

could potentially be replaced by an inlined guava Function<Void, T> in Streams, so this class is not needed?

This comment has been minimized.

Copy link
@uboness

uboness Jul 31, 2014

Author Contributor

function as a callback? that would be too hacky IMO


@Test
public void testOK() throws Exception {
Terminal terminal = new TerminalMock();

This comment has been minimized.

Copy link
@spinscale

spinscale Jul 31, 2014

Member

could be a instance variable, as used in all tests

This comment has been minimized.

Copy link
@uboness

uboness Jul 31, 2014

Author Contributor

it's not used by all tests... some tests need their own mock, so I kept it on a test scope (I think it's clearer)

@spinscale

View changes

src/test/java/org/elasticsearch/common/cli/CliToolTests.java Outdated
SingleCmdTool tool = new SingleCmdTool("tool", terminal, cmd);
int status = tool.execute();
assertThat(executed.get(), is(true));
assertThat(status, is(CliTool.ExitStatus.OK.status()));

This comment has been minimized.

Copy link
@spinscale

spinscale Jul 31, 2014

Member

what about creating reusable assertion methods here, along the lines

private void assertExecuted(tool, executed, OK)

This comment has been minimized.

Copy link
@uboness

uboness Jul 31, 2014

Author Contributor

hmmm.. interesting... will have a look at that

@spinscale

View changes

src/test/java/org/elasticsearch/common/cli/CliToolTests.java Outdated
assertThat(status, is(CliTool.ExitStatus.CODE_ERROR.status()));
}

public void testMultiCommand() {

This comment has been minimized.

Copy link
@spinscale

spinscale Jul 31, 2014

Member

no test annotation (not needed, but for consistency)

This comment has been minimized.

Copy link
@uboness

uboness Jul 31, 2014

Author Contributor

agreed

@spinscale

View changes

src/test/java/org/elasticsearch/common/cli/CliToolTests.java Outdated
}
}

public void testMultiCommand_UnknownCommand() {

This comment has been minimized.

Copy link
@spinscale

spinscale Jul 31, 2014

Member

no test annotation

This comment has been minimized.

Copy link
@uboness

uboness Jul 31, 2014

Author Contributor

+1

@spinscale

View changes

src/test/java/org/elasticsearch/common/cli/CliToolTests.java Outdated
assertThat(writeCounter.get(), is(3));
}

public void testMultiCommand_ToolHelp() {

This comment has been minimized.

Copy link
@spinscale

spinscale Jul 31, 2014

Member

no test annotation

This comment has been minimized.

Copy link
@uboness

uboness Jul 31, 2014

Author Contributor

+1

@spinscale

View changes

src/test/java/org/elasticsearch/common/cli/CliToolTests.java Outdated
assertThat(writeCounter.get(), is(3));
}

public void testMultiCommand_CmdHelp() {

This comment has been minimized.

Copy link
@spinscale

spinscale Jul 31, 2014

Member

annotation

This comment has been minimized.

Copy link
@uboness

uboness Jul 31, 2014

Author Contributor

+1

@spinscale

View changes

src/test/java/org/elasticsearch/common/cli/TerminalTests.java Outdated
terminal.print(Terminal.Verbosity.SILENT, "text");
assertThat(printed.get(), is(true));
printed.set(false);
terminal.print(Terminal.Verbosity.NORMAL, "text");

This comment has been minimized.

Copy link
@spinscale

spinscale Jul 31, 2014

Member

resetting the state here makes the test hard to read... can you again maybe create a helper method, that does this

assertPrinted(terminal, SILENT)
assertNotPrinted(terminal, SILENT)
@spinscale

This comment has been minimized.

Copy link
Member

commented Jul 31, 2014

Left a few comments, mostly about the tests.

I like the idea a in general, a bit worried about the performance (didnt test anything, maybe I am plain wrong here), but I'd like to see the PluginManager adapted as part of this PR to make sure everything works as we expect and all features are covered instead of finding out later, that we cannot do everything needed with this.

@javanna

View changes

src/main/java/org/elasticsearch/common/cli/CliTool.java Outdated
*
* Two modes are supported:
*
* - Singe command mode. The tool exposes a single command that can potentially accept arguments (eg. CLI options).

This comment has been minimized.

Copy link
@javanna

javanna Jul 31, 2014

Member

s/Singe/Single

@javanna

View changes

src/main/java/org/elasticsearch/common/cli/CliTool.java Outdated
* Two modes are supported:
*
* - Singe command mode. The tool exposes a single command that can potentially accept arguments (eg. CLI options).
* - Multi command mode. The tool support multiple command, each for different tasks, each potentially accepts arguments.

This comment has been minimized.

Copy link
@javanna

javanna Jul 31, 2014

Member

s/support multiple command/supports multiple commands

@uboness

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2014

@spinscale thanks for the review... I'd like have the plugin manager implemented in a separate PR - its own PR as part of the work there is to change how it works (right now, aside from being a mess, it's also not consistent with its supported options).

If we find things missing in the infra, we can always add those later.

@uboness

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2014

updated based on comments by @spinscale

  • change help file resolution... command help file should now be named:
<tool_name>-<cmd_name>.help
Added a cli infrastructure
CliTool is a base class for command-line interface tools (such as the plugin manager and potentially others). It supports the following:
  - single or multi command tool
  - help printing infrastructure (based on help files)
  - consistent mechanism of parsing arguments (based on commons-cli lib)
  - separation of argument parsing and command execution (for easier unit testing)
  - terminal abstraction (will use System.console() when available)

@uboness uboness merged commit 5ccc7be into elastic:master Aug 2, 2014

@uboness uboness removed the review label Aug 2, 2014

@clintongormley clintongormley changed the title Added a cli infrastructure Internal: Added a cli infrastructure Sep 8, 2014

@clintongormley clintongormley changed the title Internal: Added a cli infrastructure Added a cli infrastructure Jun 7, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.