Move the CLI into its own subproject by hub-cap · Pull Request #27114 · elastic/elasticsearch

hub-cap · 2017-10-25T21:14:59Z

Projects the depend on the CLI currently depend on core. This should not
always be the case. The EnvironmentAwareCommand will remain in :core,
but the rest of the CLI components have been moved into their own
subproject of :core, :core:cli.

Projects the depend on the CLI currently depend on core. This should not always be the case. The EnvironmentAwareCommand will remain in :core, but the rest of the CLI components have been moved into their own subproject of :core, :core:cli.

hub-cap · 2017-10-25T21:28:00Z

core/cli/build.gradle

+apply plugin: 'elasticsearch.build'
+apply plugin: 'ru.vyarus.animalsniffer'
+apply plugin: 'nebula.maven-base-publish'
+apply plugin: 'nebula.maven-scm'


hmm, prolly dont some of these. will test w/o them.

jasontedor

I left a few comments.

jasontedor · 2017-10-25T21:33:31Z

core/src/main/java/org/elasticsearch/cli/EnvironmentAwareCommand.java

    }

+    @Override
+    protected void beforeExecute() {


Should this be final?

jasontedor · 2017-10-25T21:33:40Z

core/cli/build.gradle

+
+dependencies {
+    compile 'net.sf.jopt-simple:jopt-simple:5.0.2'
+    compile "org.apache.lucene:lucene-core:${versions.lucene}"


Why is Lucene here, that seems unfortunate?

jasontedor · 2017-10-25T21:34:06Z

core/cli/src/main/java/org/elasticsearch/cli/SuppressForbidden.java

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


The indentation is off here.

jasontedor · 2017-10-25T21:34:17Z

core/cli/src/main/java/org/elasticsearch/cli/SuppressForbidden.java

+    import java.lang.annotation.Retention;
+    import java.lang.annotation.RetentionPolicy;
+    import java.lang.annotation.Target;
+/**


Can you spare a line of whitespace?

rjernst · 2017-10-25T22:29:03Z

core/build.gradle

  compile 'org.elasticsearch:securesm:1.1'

  // utilities
+  compile "org.elasticsearch:elasticsearch-cli:${version}"


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?

…s/separate_cli

hub-cap · 2017-11-01T18:22:54Z

@elasticmachine test this please. ~~I refute your claim of failure with this message.~~ I refute my claim of sanity.

hub-cap · 2017-11-01T21:12:22Z

@elasticmachine test this please. I think you are lying to me on purpose.

nik9000 · 2017-11-02T03:09:15Z

Skip tests as there are none... should we move them?

I haven't read the PR but I liked this commit message. In general my take is "yes", we should move whatever tests make sense.

hub-cap · 2017-11-13T21:08:56Z

@elasticmachine test this please. I no longer think you are lying to me. I think gradle is.

hub-cap · 2017-11-13T23:10:48Z

In general my take is "yes", we should move whatever tests make sense.

I will do this in a followup PR, as my priorities have shifted a bit. As is, we will need to create a new "cli test" framework so that we can move things like MockTerminal into it, since many classes in core use this. Then we can move the cli tests into this project, and depend on this cli test framework instead of the core test framework. Its a bit more work than i want to tackle with this PR, and I think itll make it larger than necessary. But its the right thing to do, I think, so Ill circle back soon (tm).

…ore runtime configurations (which also had cli in it)

…im having to add a es notice/license still.

hub-cap · 2017-11-14T03:31:35Z

yay build finally passed cc @jasontedor

hub-cap · 2017-11-14T03:37:39Z

I think we still have an issue w/ this in that the core project should not be depending on a license/notice from another subproject (ie, the core/licenses/elasticsearch-cli-*) and I still need to sort that, but it should be ready for review barring that change

jasontedor

Looks good, I left a few comments.

jasontedor · 2017-11-16T11:11:26Z

core/cli/build.gradle

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


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.

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

@hub-cap Separate is fine.

jasontedor · 2017-11-16T11:13:24Z

core/cli/src/main/java/org/elasticsearch/cli/Command.java

    }

+    /** Gets the shutdown hook thread if it exists **/
+    public Thread getShutdownHookThread() {


I don't think this needs to be public, only package-private?

jasontedor · 2017-11-16T11:14:11Z

core/cli/src/main/java/org/elasticsearch/cli/Command.java

-    protected boolean shouldConfigureLoggingWithoutConfig() {
-        return true;
-    }
+    protected void beforeExecute() throws Exception {}


Why does this declare a checked exception?

cruft from the refactor. itll be gone next push.

jasontedor

Sorry, one more nit.

jasontedor · 2017-11-17T13:41:16Z

core/cli/src/main/java/org/elasticsearch/cli/Command.java


    /** Gets the shutdown hook thread if it exists **/
-    public Thread getShutdownHookThread() {
+    protected Thread getShutdownHookThread() {


Can we get away with this being package visible only?

jasontedor

LGTM.

Projects the depend on the CLI currently depend on core. This should not always be the case. The EnvironmentAwareCommand will remain in :core, but the rest of the CLI components have been moved into their own subproject of :core, :core:cli.

* master: (31 commits) [TEST] Fix `GeoShapeQueryTests#testPointsOnly` failure Transition transport apis to use void listeners (#27440) AwaitsFix GeoShapeQueryTests#testPointsOnly #27454 Bump test version after backport Ensure nested documents have consistent version and seq_ids (#27455) Tests: Add Fedora-27 to packaging tests Delete some seemingly unused exceptions (#27439) #26800: Fix docs rendering Remove config prompting for secrets and text (#27216) Move the CLI into its own subproject (#27114) Correct usage of "an" to "a" in getting started docs Avoid NPE when getting build information Removes BWC snapshot status handler used in 6.x (#27443) Remove manual tracking of registered channels (#27445) Remove parameters on HandshakeResponseHandler (#27444) [GEO] fix pointsOnly bug for MULTIPOINT Standardize underscore requirements in parameters (#27414) peanut butter hamburgers Log primary-replica resync failures Uses TransportMasterNodeAction to update shard snapshot status (#27165) ...

* 6.x: (41 commits) [TEST] Fix `GeoShapeQueryTests#testPointsOnly` failure Transition transport apis to use void listeners (#27440) AwaitsFix GeoShapeQueryTests#testPointsOnly #27454 Ensure nested documents have consistent version and seq_ids (#27455) Tests: Add Fedora-27 to packaging tests #26800: Fix docs rendering Move the CLI into its own subproject (#27114) Correct usage of "an" to "a" in getting started docs Avoid NPE when getting build information Remove manual tracking of registered channels (#27445) Standardize underscore requirements in parameters (#27414) Remove parameters on HandshakeResponseHandler (#27444) [GEO] fix pointsOnly bug for MULTIPOINT peanut butter hamburgers Uses TransportMasterNodeAction to update shard snapshot status (#27165) Log primary-replica resync failures Add limits for ngram and shingle settings (#27411) Enforce a minimum task execution and service time of 1 nanosecond Fix place-holder in allocation decider messages (#27436) Remove newline from log message (#27425) ...

hub-cap added 2 commits October 25, 2017 16:02

Move the CLI into its own subproject

c4308ce

Projects the depend on the CLI currently depend on core. This should not always be the case. The EnvironmentAwareCommand will remain in :core, but the rest of the CLI components have been moved into their own subproject of :core, :core:cli.

nit cleanup on file

c6cc762

hub-cap added v6.1.0 v7.0.0 >non-issue labels Oct 25, 2017

hub-cap commented Oct 25, 2017

View reviewed changes

jasontedor reviewed Oct 25, 2017

View reviewed changes

rjernst reviewed Oct 25, 2017

View reviewed changes

hub-cap added 4 commits October 31, 2017 11:34

Merge branch 'master' of github.com:elastic/elasticsearch into plugin…

73745ea

…s/separate_cli

Incorporate changes from review

7eda8e2

Fixing the concurrent modification error

12dd213

Merge remote-tracking branch 'upstream/master' into plugins/separate_cli

e248c16

hub-cap added 4 commits November 1, 2017 14:07

Missed dep in tests

9c61c51

Missed another, grr

037259c

Fixing deplicenses for cli

dcdaaf8

Skip tests as there are none... should we move them?

9cd8a89

hub-cap added 3 commits November 6, 2017 13:24

Merge remote-tracking branch 'upstream/master' into plugins/separate_cli

62612ac

Merge remote-tracking branch 'upstream/master' into plugins/separate_cli

db9980e

Merge remote-tracking branch 'upstream/master' into plugins/separate_cli

87448d3

hub-cap added 2 commits November 13, 2017 18:14

Fixing the jarHell I had gotten myself into with adding the jar and c…

b1cfecb

…ore runtime configurations (which also had cli in it)

Funky cleanup with deps in the wrong place. I need to figure out why …

1e0a98c

…im having to add a es notice/license still.

hub-cap added 2 commits November 15, 2017 17:02

Cleanup on licenses

c179020

Merge remote-tracking branch 'upstream/master' into plugins/separate_cli

75db462

Removing jarHell since it does not have access to bootstrap.JarHell

21ed445

jasontedor reviewed Nov 16, 2017

View reviewed changes

hub-cap added 2 commits November 16, 2017 13:13

Merge remote-tracking branch 'upstream/master' into plugins/separate_cli

1fb5f1b

PR nits

94bbb74

jasontedor reviewed Nov 17, 2017

View reviewed changes

hub-cap added 2 commits November 17, 2017 11:23

package->default as per PR

905546b

Merge remote-tracking branch 'upstream/master' into plugins/separate_cli

2ead8d6

jasontedor approved these changes Nov 17, 2017

View reviewed changes

hub-cap added 2 commits November 17, 2017 18:56

Merge remote-tracking branch 'upstream/master' into plugins/separate_cli

529f7f5

Merge remote-tracking branch 'upstream/master' into plugins/separate_cli

2c366c1

hub-cap merged commit cb3e8f4 into elastic:master Nov 19, 2017

jpountz mentioned this pull request Nov 24, 2017

[CI] Vagrant tests fail with logging error #27521

Closed

jasontedor mentioned this pull request Dec 17, 2017

Fix publication of elasticsearch-cli to Maven #27853

Merged

jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Conversation

hub-cap commented Oct 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hub-cap commented Nov 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hub-cap commented Nov 1, 2017

Uh oh!

nik9000 commented Nov 2, 2017

Uh oh!

hub-cap commented Nov 13, 2017

Uh oh!

hub-cap commented Nov 13, 2017

Uh oh!

hub-cap commented Nov 14, 2017

Uh oh!

hub-cap commented Nov 14, 2017

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasontedor left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

hub-cap commented Nov 1, 2017 •

edited

Loading