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

Move the CLI into its own subproject #27114

Merged
merged 24 commits into from
Nov 19, 2017
Merged
Show file tree
Hide file tree
Changes from 18 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
c4308ce
Move the CLI into its own subproject
hub-cap Oct 25, 2017
c6cc762
nit cleanup on file
hub-cap Oct 25, 2017
73745ea
Merge branch 'master' of github.com:elastic/elasticsearch into plugin…
hub-cap Oct 31, 2017
7eda8e2
Incorporate changes from review
hub-cap Oct 31, 2017
12dd213
Fixing the concurrent modification error
hub-cap Nov 1, 2017
e248c16
Merge remote-tracking branch 'upstream/master' into plugins/separate_cli
hub-cap Nov 1, 2017
9c61c51
Missed dep in tests
hub-cap Nov 1, 2017
037259c
Missed another, grr
hub-cap Nov 1, 2017
dcdaaf8
Fixing deplicenses for cli
hub-cap Nov 1, 2017
9cd8a89
Skip tests as there are none... should we move them?
hub-cap Nov 1, 2017
62612ac
Merge remote-tracking branch 'upstream/master' into plugins/separate_cli
hub-cap Nov 6, 2017
db9980e
Merge remote-tracking branch 'upstream/master' into plugins/separate_cli
hub-cap Nov 7, 2017
87448d3
Merge remote-tracking branch 'upstream/master' into plugins/separate_cli
hub-cap Nov 13, 2017
b1cfecb
Fixing the jarHell I had gotten myself into with adding the jar and c…
hub-cap Nov 13, 2017
1e0a98c
Funky cleanup with deps in the wrong place. I need to figure out why …
hub-cap Nov 14, 2017
c179020
Cleanup on licenses
hub-cap Nov 15, 2017
75db462
Merge remote-tracking branch 'upstream/master' into plugins/separate_cli
hub-cap Nov 15, 2017
21ed445
Removing jarHell since it does not have access to bootstrap.JarHell
hub-cap Nov 16, 2017
1fb5f1b
Merge remote-tracking branch 'upstream/master' into plugins/separate_cli
hub-cap Nov 16, 2017
94bbb74
PR nits
hub-cap Nov 16, 2017
905546b
package->default as per PR
hub-cap Nov 17, 2017
2ead8d6
Merge remote-tracking branch 'upstream/master' into plugins/separate_cli
hub-cap Nov 17, 2017
529f7f5
Merge remote-tracking branch 'upstream/master' into plugins/separate_cli
hub-cap Nov 18, 2017
2c366c1
Merge remote-tracking branch 'upstream/master' into plugins/separate_cli
hub-cap Nov 19, 2017
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,7 @@ subprojects {
"org.elasticsearch.gradle:build-tools:${version}": ':build-tools',
"org.elasticsearch:rest-api-spec:${version}": ':rest-api-spec',
"org.elasticsearch:elasticsearch:${version}": ':core',
"org.elasticsearch:elasticsearch-cli:${version}": ':core:cli',
"org.elasticsearch.client:elasticsearch-rest-client:${version}": ':client:rest',
"org.elasticsearch.client:elasticsearch-rest-client-sniffer:${version}": ':client:sniffer',
"org.elasticsearch.client:elasticsearch-rest-high-level-client:${version}": ':client:rest-high-level',
Expand Down
8 changes: 7 additions & 1 deletion core/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ dependencies {
compile 'org.elasticsearch:securesm:1.1'

// utilities
compile 'net.sf.jopt-simple:jopt-simple:5.0.2'
compile "org.elasticsearch:elasticsearch-cli:${version}"
Copy link
Member

Choose a reason for hiding this comment

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

Do we actually need to publish this? If we first move the ES cli under a distribution/tools project, then this could could be something like distribution/tools/base-cli?

compile 'com.carrotsearch:hppc:0.7.1'

// time handling, remove with java 8 time
Expand Down Expand Up @@ -265,6 +265,12 @@ if (JavaVersion.current() > JavaVersion.VERSION_1_8) {
dependencyLicenses {
mapping from: /lucene-.*/, to: 'lucene'
mapping from: /jackson-.*/, to: 'jackson'
dependencies = project.configurations.runtime.fileCollection {
it.group.startsWith('org.elasticsearch') == false ||
// keep the following org.elasticsearch jars in
(it.name == 'jna' ||
it.name == 'securesm')
}
}

if (isEclipse == false || project.path == ":core-tests") {
Expand Down
36 changes: 36 additions & 0 deletions core/cli/build.gradle
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import org.elasticsearch.gradle.precommit.PrecommitTasks

apply plugin: 'elasticsearch.build'

archivesBaseName = 'elasticsearch-cli'

dependencies {
compile 'net.sf.jopt-simple:jopt-simple:5.0.2'
}

test.enabled = false
// Since CLI does not depend on :core, it cannot run the jarHell task
jarHell.enabled = false
Copy link
Member

Choose a reason for hiding this comment

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

I think we should refactor out the JAR hell check too, as there are other places where we will want to run JAR hell checks that will not depend on core.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as part of this PR or just as a separate issue @jasontedor ?

Copy link
Member

Choose a reason for hiding this comment

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

@hub-cap Separate is fine.


forbiddenApisMain {
signaturesURLs = [PrecommitTasks.getResource('/forbidden/jdk-signatures.txt')]
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,6 @@
import joptsimple.OptionParser;
import joptsimple.OptionSet;
import joptsimple.OptionSpec;
import org.apache.logging.log4j.Level;
import org.apache.lucene.util.SetOnce;
import org.elasticsearch.common.SuppressForbidden;
import org.elasticsearch.common.logging.LogConfigurator;
import org.elasticsearch.common.settings.Settings;

import java.io.Closeable;
import java.io.IOException;
Expand Down Expand Up @@ -55,12 +50,13 @@ public Command(String description) {
this.description = description;
}

final SetOnce<Thread> shutdownHookThread = new SetOnce<>();
private Thread shutdownHookThread;

/** Parses options for this command from args and executes it. */
public final int main(String[] args, Terminal terminal) throws Exception {
if (addShutdownHook()) {
shutdownHookThread.set(new Thread(() -> {

shutdownHookThread = new Thread(() -> {
try {
this.close();
} catch (final IOException e) {
Expand All @@ -75,16 +71,11 @@ public final int main(String[] args, Terminal terminal) throws Exception {
throw new AssertionError(impossible);
}
}
}));
Runtime.getRuntime().addShutdownHook(shutdownHookThread.get());
});
Runtime.getRuntime().addShutdownHook(shutdownHookThread);
}

if (shouldConfigureLoggingWithoutConfig()) {
// initialize default for es.logger.level because we will not read the log4j2.properties
final String loggerLevel = System.getProperty("es.logger.level", Level.INFO.name());
final Settings settings = Settings.builder().put("logger.level", loggerLevel).build();
LogConfigurator.configureWithoutConfig(settings);
}
beforeExecute();

try {
mainWithoutErrorHandling(args, terminal);
Expand All @@ -103,14 +94,10 @@ public final int main(String[] args, Terminal terminal) throws Exception {
}

/**
* Indicate whether or not logging should be configured without reading a log4j2.properties. Most commands should do this because we do
* not configure logging for CLI tools. Only commands that configure logging on their own should not do this.
*
* @return true if logging should be configured without reading a log4j2.properties file
* Setup method to be executed before parsing or execution of the command being run. Any exceptions thrown by the
* method will not be cleanly caught by the parser.
*/
protected boolean shouldConfigureLoggingWithoutConfig() {
return true;
}
protected void beforeExecute() throws Exception {}
Copy link
Member

Choose a reason for hiding this comment

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

Why does this declare a checked exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cruft from the refactor. itll be gone next push.


/**
* Executes the command, but all errors are thrown.
Expand Down Expand Up @@ -166,6 +153,11 @@ protected boolean addShutdownHook() {
return true;
}

/** Gets the shutdown hook thread if it exists **/
public Thread getShutdownHookThread() {
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 think this needs to be public, only package-private?

return shutdownHookThread;
}

@Override
public void close() throws IOException {

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/
package org.elasticsearch.cli;

import java.lang.annotation.ElementType;
import java.lang.annotation.Retention;
import java.lang.annotation.RetentionPolicy;
import java.lang.annotation.Target;

/**
Copy link
Member

Choose a reason for hiding this comment

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

Can you spare a line of whitespace?

* Annotation to suppress forbidden-apis errors inside a whole class, a method, or a field.
*/
@Retention(RetentionPolicy.CLASS)
@Target({ ElementType.CONSTRUCTOR, ElementType.FIELD, ElementType.METHOD, ElementType.TYPE })
public @interface SuppressForbidden {
String reason();
}

Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,6 @@

package org.elasticsearch.cli;

import org.elasticsearch.common.SuppressForbidden;

import java.io.BufferedReader;
import java.io.Console;
import java.io.IOException;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@
import joptsimple.OptionSet;
import joptsimple.OptionSpec;
import joptsimple.util.KeyValuePair;
import org.apache.logging.log4j.Level;
import org.elasticsearch.common.SuppressForbidden;
import org.elasticsearch.common.logging.LogConfigurator;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.env.Environment;
import org.elasticsearch.node.InternalSettingsPreparer;
Expand Down Expand Up @@ -102,6 +104,26 @@ private static void putSystemPropertyIfSettingIsMissing(final Map<String, String
}
}

@Override
protected final void beforeExecute() {
if (shouldConfigureLoggingWithoutConfig()) {
// initialize default for es.logger.level because we will not read the log4j2.properties
final String loggerLevel = System.getProperty("es.logger.level", Level.INFO.name());
final Settings settings = Settings.builder().put("logger.level", loggerLevel).build();
LogConfigurator.configureWithoutConfig(settings);
}
}

/**
* Indicate whether or not logging should be configured without reading a log4j2.properties. Most commands should do this because we do
* not configure logging for CLI tools. Only commands that configure logging on their own should not do this.
*
* @return true if logging should be configured without reading a log4j2.properties file
*/
protected boolean shouldConfigureLoggingWithoutConfig() {
return true;
}

/** Execute the command with the initialized {@link Environment}. */
protected abstract void execute(Terminal terminal, OptionSet options, Environment env) throws Exception;

Expand Down
1 change: 1 addition & 0 deletions distribution/tools/plugin-cli/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ apply plugin: 'elasticsearch.build'

dependencies {
provided "org.elasticsearch:elasticsearch:${version}"
provided "org.elasticsearch:elasticsearch-cli:${version}"
testCompile "org.elasticsearch.test:framework:${version}"
testCompile 'com.google.jimfs:jimfs:1.1'
testCompile 'com.google.guava:guava:18.0'
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,11 @@ public void close() throws IOException {
};
final MockTerminal terminal = new MockTerminal();
command.main(new String[0], terminal);
assertNotNull(command.shutdownHookThread.get());
assertNotNull(command.getShutdownHookThread());
// successful removal here asserts that the runtime hook was installed in Command#main
assertTrue(Runtime.getRuntime().removeShutdownHook(command.shutdownHookThread.get()));
command.shutdownHookThread.get().run();
command.shutdownHookThread.get().join();
assertTrue(Runtime.getRuntime().removeShutdownHook(command.getShutdownHookThread()));
command.getShutdownHookThread().run();
command.getShutdownHookThread().join();
assertTrue(closed.get());
final String output = terminal.getOutput();
if (shouldThrow) {
Expand Down
1 change: 1 addition & 0 deletions settings.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ List projects = [
'build-tools',
'rest-api-spec',
'core',
'core:cli',
'docs',
'client:rest',
'client:rest-high-level',
Expand Down
1 change: 1 addition & 0 deletions test/framework/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import org.elasticsearch.gradle.precommit.PrecommitTasks;
dependencies {
compile "org.elasticsearch.client:elasticsearch-rest-client:${version}"
compile "org.elasticsearch:elasticsearch:${version}"
compile "org.elasticsearch:elasticsearch-cli:${version}"
compile "com.carrotsearch.randomizedtesting:randomizedtesting-runner:${versions.randomizedrunner}"
compile "junit:junit:${versions.junit}"
compile "org.hamcrest:hamcrest-all:${versions.hamcrest}"
Expand Down