gemmellr commented on code in PR #4565:
URL: https://github.com/apache/activemq-artemis/pull/4565#discussion_r1276148633
##########
pom.xml:
##########
@@ -586,17 +587,23 @@
<!-- License: Apache 2.0 -->
</dependency>
<dependency>
- <groupId>com.github.rvesse</groupId>
- <artifactId>airline</artifactId>
- <version>${airline.version}</version>
- <exclusions>
- <exclusion>
- <groupId>com.github.rvesse</groupId>
- <artifactId>airline-backcompat-javaxinject</artifactId>
- </exclusion>
- </exclusions>
+ <groupId>info.picocli</groupId>
+ <artifactId>picocli</artifactId>
+ <version>${picocli.version}</version>
<!-- License: Apache 2.0 -->
</dependency>
+ <dependency>
+ <groupId>info.picocli</groupId>
+ <artifactId>picocli-shell-jline3</artifactId>
+ <version>${picocli.version}</version>
+ <!-- License: Apache 2.0 -->
+ </dependency>
+ <dependency>
+ <groupId>org.jline</groupId>
+ <artifactId>jline</artifactId>
+ <version>${jline.version}</version>
+ <!-- License: BSD 3-Clause -->
+ </dependency>
Review Comment:
Should update the binary assembly LICENCE file to cover this addition since
it isnt ALv2.
##########
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/Create.java:
##########
@@ -36,11 +34,12 @@
import org.apache.activemq.artemis.nativo.jlibaio.LibaioContext;
import org.apache.activemq.artemis.nativo.jlibaio.LibaioFile;
import org.apache.activemq.artemis.utils.FileUtil;
+import picocli.CommandLine;
/**
* CLI action that creates a broker instance directory.
*/
-@Command(name = "create", description = "Create a new broker instance.")
[email protected](name = "create", description = "Create a new broker
instance.")
Review Comment:
Do these actually need to be prefixed? Wouldnt it just work the same as
before if you simply switched the imports and left the types unchanged where
the name is the same? If so the lines would then be shorter/nicer and the diff
would be much simpler (just adding the s to make 'name' become 'names').
Same for most of the other files.
##########
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/Artemis.java:
##########
@@ -194,62 +186,102 @@ public static Object execute(boolean inputEnabled,
boolean useSystemOut, File ar
* Useful on test cases
*/
private static Object internalExecute(File artemisHome, File
artemisInstance, File etcFolder, String[] args) throws Exception {
- return internalExecute(artemisHome, artemisInstance, etcFolder, args,
ActionContext.system());
+ return internalExecute(artemisHome, artemisInstance, etcFolder, args,
new ActionContext());
}
public static Object internalExecute(File artemisHome, File
artemisInstance, File etcFolder, String[] args, ActionContext context) throws
Exception {
- Action action = builder(artemisInstance).build().parse(args);
- action.setHomeValues(artemisHome, artemisInstance, etcFolder);
+ boolean isInstance = artemisInstance != null ||
System.getProperty("artemis.instance") != null;
+ CommandLine commandLine = buildCommand(isInstance, !isInstance);
+
+ Object userObject = parseAction(commandLine, args);
+ if (userObject instanceof Action) {
+ Action action = (Action) userObject;
+ action.setHomeValues(artemisHome, artemisInstance, etcFolder);
+ if (action.isVerbose()) {
+ context.out.print("Executing " + action.getClass().getName() + "
");
+ for (String arg : args) {
+ context.out.print(arg + " ");
+ }
+ context.out.println();
+ context.out.println("Home::" + action.getBrokerHome() + ",
Instance::" + action.getBrokerInstance());
+ }
- if (action.isVerbose()) {
- context.out.print("Executing " + action.getClass().getName() + " ");
- for (String arg : args) {
- context.out.print(arg + " ");
+ return action.execute(context);
+ } else {
+ if (userObject instanceof Runnable) {
+ ((Runnable) userObject).run();
+ }
+ }
+ return null;
+ }
+
+ /*
+ Pico-cli traditionally would execute user objects that implement Runnable.
+ However as we used airline before, we needed parse for the proper action.
+ This method here is parsing the arg and find the proper user object in the
hierarchy of sub-commands
+ and return it to the caller.
+ */
+ private static Object parseAction(CommandLine line, String[] args) {
+ CommandLine.ParseResult parseResult = line.parseArgs(args);
+ if (parseResult != null) {
+ for (; parseResult.hasSubcommand(); parseResult =
parseResult.subcommand()) {
Review Comment:
While instead of for?
##########
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/Upgrade.java:
##########
@@ -106,8 +106,16 @@ public Object run(ActionContext context) throws Exception {
throw new IOException(etcFolder + " does not exist for etc");
}
+ StringBuffer javaOption = new StringBuffer();
Review Comment:
StringBuilder?
Name could be more obvious than the singular of the original plural list,
e.g javaOptionsArgs...or javaOptionsString (and build the value here rather
than later at the point it is passed).
##########
artemis-distribution/src/main/resources/bin/artemis:
##########
@@ -91,6 +91,9 @@ if $cygwin ; then
fi
exec "$JAVACMD" $JAVA_ARGS $ARTEMIS_CLUSTER_PROPS \
+ --add-opens java.base/java.lang=ALL-UNNAMED \
+ --add-opens java.base/java.io=ALL-UNNAMED \
+ --add-opens java.base/java.util=ALL-UNNAMED \
Review Comment:
Ick. Maybe add a comment to say it is for the CLI stuff.
Have you tried it on JDK17 and 21EA to check nothing different is needed
there than on 11?
##########
docs/user-manual/en/using-server.md:
##########
@@ -50,6 +50,30 @@ The following highlights some important folders on the
distribution:
- `user-manual` - The user manual is placed under the web folder.
+## Artemis Shell
+
+You can activate the artemis shell by typing:
+
+```shell
+./artemis shell
+```
+
+This will start a JLine3 terminal with auto-completion:
Review Comment:
Sentence fragment, seems to end mid:
---
Also, this drop-in about the shell usage also doesnt really fit with the
rest of the existing information on the page below that all assumes you are
**not** using it. It could do with some elaboration / description linking and
contrasting the different usages so the two routes are clear, and some new user
actually following this page linearly isnt met with a sequence that essentially
says 'do foo' followed by 'do this incompatible thing that requires you didnt
do the earlier thing we already said'.
##########
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/Artemis.java:
##########
@@ -194,62 +186,102 @@ public static Object execute(boolean inputEnabled,
boolean useSystemOut, File ar
* Useful on test cases
*/
private static Object internalExecute(File artemisHome, File
artemisInstance, File etcFolder, String[] args) throws Exception {
- return internalExecute(artemisHome, artemisInstance, etcFolder, args,
ActionContext.system());
+ return internalExecute(artemisHome, artemisInstance, etcFolder, args,
new ActionContext());
}
public static Object internalExecute(File artemisHome, File
artemisInstance, File etcFolder, String[] args, ActionContext context) throws
Exception {
- Action action = builder(artemisInstance).build().parse(args);
- action.setHomeValues(artemisHome, artemisInstance, etcFolder);
+ boolean isInstance = artemisInstance != null ||
System.getProperty("artemis.instance") != null;
+ CommandLine commandLine = buildCommand(isInstance, !isInstance);
+
+ Object userObject = parseAction(commandLine, args);
+ if (userObject instanceof Action) {
+ Action action = (Action) userObject;
+ action.setHomeValues(artemisHome, artemisInstance, etcFolder);
+ if (action.isVerbose()) {
+ context.out.print("Executing " + action.getClass().getName() + "
");
+ for (String arg : args) {
+ context.out.print(arg + " ");
+ }
+ context.out.println();
+ context.out.println("Home::" + action.getBrokerHome() + ",
Instance::" + action.getBrokerInstance());
+ }
- if (action.isVerbose()) {
- context.out.print("Executing " + action.getClass().getName() + " ");
- for (String arg : args) {
- context.out.print(arg + " ");
+ return action.execute(context);
+ } else {
+ if (userObject instanceof Runnable) {
+ ((Runnable) userObject).run();
+ }
Review Comment:
Maybe do something if it isnt? Should it ever get here otherwise?
##########
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/commands/Create.java:
##########
@@ -712,11 +711,15 @@ public Object run(ActionContext context) throws Exception
{
File logFolder = createDirectory("log", directory);
File oomeDumpFile = new File(logFolder, "oom_dump.hprof");
- if (javaOptions == null || javaOptions.length() == 0) {
- javaOptions = "";
+ StringBuffer javaOption = new StringBuffer();
Review Comment:
Same comment as other file. Same impl too...maybe add a helper.
##########
artemis-cli/src/main/java/org/apache/activemq/artemis/cli/Artemis.java:
##########
@@ -194,62 +186,102 @@ public static Object execute(boolean inputEnabled,
boolean useSystemOut, File ar
* Useful on test cases
*/
private static Object internalExecute(File artemisHome, File
artemisInstance, File etcFolder, String[] args) throws Exception {
- return internalExecute(artemisHome, artemisInstance, etcFolder, args,
ActionContext.system());
+ return internalExecute(artemisHome, artemisInstance, etcFolder, args,
new ActionContext());
}
public static Object internalExecute(File artemisHome, File
artemisInstance, File etcFolder, String[] args, ActionContext context) throws
Exception {
- Action action = builder(artemisInstance).build().parse(args);
- action.setHomeValues(artemisHome, artemisInstance, etcFolder);
+ boolean isInstance = artemisInstance != null ||
System.getProperty("artemis.instance") != null;
+ CommandLine commandLine = buildCommand(isInstance, !isInstance);
+
+ Object userObject = parseAction(commandLine, args);
+ if (userObject instanceof Action) {
+ Action action = (Action) userObject;
+ action.setHomeValues(artemisHome, artemisInstance, etcFolder);
+ if (action.isVerbose()) {
+ context.out.print("Executing " + action.getClass().getName() + "
");
+ for (String arg : args) {
+ context.out.print(arg + " ");
+ }
+ context.out.println();
+ context.out.println("Home::" + action.getBrokerHome() + ",
Instance::" + action.getBrokerInstance());
+ }
- if (action.isVerbose()) {
- context.out.print("Executing " + action.getClass().getName() + " ");
- for (String arg : args) {
- context.out.print(arg + " ");
+ return action.execute(context);
+ } else {
+ if (userObject instanceof Runnable) {
+ ((Runnable) userObject).run();
+ }
+ }
+ return null;
+ }
+
+ /*
+ Pico-cli traditionally would execute user objects that implement Runnable.
+ However as we used airline before, we needed parse for the proper action.
+ This method here is parsing the arg and find the proper user object in the
hierarchy of sub-commands
+ and return it to the caller.
+ */
+ private static Object parseAction(CommandLine line, String[] args) {
+ CommandLine.ParseResult parseResult = line.parseArgs(args);
+ if (parseResult != null) {
+ for (; parseResult.hasSubcommand(); parseResult =
parseResult.subcommand()) {
}
- context.out.println();
- context.out.println("Home::" + action.getBrokerHome() + ",
Instance::" + action.getBrokerInstance());
}
+ if (parseResult == null) {
+ throw new RuntimeException("Cannot match arg::" +
Arrays.toString(args));
+ }
+ return parseResult.commandSpec().userObject();
+ }
+
+ public static CommandLine buildCommand(boolean includeInstanceCommands,
boolean includeHomeCommands) {
+ return buildCommand(includeInstanceCommands, includeHomeCommands, true);
- action.checkOptions(args);
- return action.execute(context);
}
- private static CliBuilder<Action> builder(File artemisInstance) {
- String instance = artemisInstance != null ?
artemisInstance.getAbsolutePath() : System.getProperty("artemis.instance");
- CliBuilder<Action> builder =
Cli.<Action>builder("artemis").withDescription("ActiveMQ Artemis Command Line").
-
withCommand(HelpAction.class).withCommand(Producer.class).withCommand(Transfer.class).withCommand(Consumer.class).
-
withCommand(Browse.class).withCommand(Mask.class).withCommand(PrintVersion.class).withDefaultCommand(HelpAction.class);
-
- builder.withGroup("perf").withDescription("Perf tools group (example
./artemis perf client)")
- .withDefaultCommand(PerfClientCommand.class)
- .withCommands(PerfProducerCommand.class, PerfConsumerCommand.class,
PerfClientCommand.class);
-
- builder.withGroup("check").withDescription("Check tools group
(node|queue) (example ./artemis check node)").
- withDefaultCommand(HelpCheck.class).withCommands(NodeCheck.class,
QueueCheck.class);
-
- builder.withGroup("queue").withDescription("Queue tools group
(create|delete|update|stat|purge) (example ./artemis queue create)").
- withDefaultCommand(HelpQueue.class).withCommands(CreateQueue.class,
DeleteQueue.class, UpdateQueue.class, StatQueue.class, PurgeQueue.class);
-
- builder.withGroup("address").withDescription("Address tools group
(create|delete|update|show) (example ./artemis address create)").
-
withDefaultCommand(HelpAddress.class).withCommands(CreateAddress.class,
DeleteAddress.class, UpdateAddress.class, ShowAddress.class);
-
- if (instance != null) {
- builder.withGroup("activation")
- .withDescription("activation tools group (sync) (example ./artemis
activation list)")
- .withDefaultCommand(ActivationSequenceList.class)
- .withCommands(ActivationSequenceList.class,
ActivationSequenceSet.class);
- builder.withGroup("data").withDescription("data tools group
(print|imp|exp|encode|decode|compact|recover) (example ./artemis data print)").
-
withDefaultCommand(HelpData.class).withCommands(RecoverMessages.class,
PrintData.class, XmlDataExporter.class, XmlDataImporter.class,
DecodeJournal.class, EncodeJournal.class, CompactJournal.class);
- builder.withGroup("user").withDescription("default file-based user
management (add|rm|list|reset) (example ./artemis user list)").
-
withDefaultCommand(HelpUser.class).withCommands(ListUser.class, AddUser.class,
RemoveUser.class, ResetUser.class);
- builder = builder.withCommands(Run.class, Stop.class, Kill.class,
PerfJournal.class);
- } else {
- builder.withGroup("data").withDescription("data tools group
(print|recover) (example ./artemis data print)").
-
withDefaultCommand(HelpData.class).withCommands(RecoverMessages.class,
PrintData.class);
- builder = builder.withCommands(Create.class, Upgrade.class);
+ public static CommandLine buildCommand(boolean includeInstanceCommands,
boolean includeHomeCommands, boolean includeShell) {
+ Artemis artemis = new Artemis();
+
+ CommandLine commandLine = new CommandLine(artemis);
+ artemis.setCommandLine(commandLine);
+
+ HelpAction help = new HelpAction();
+ help.setCommandLine(commandLine);
+ commandLine.addSubcommand(help);
+
+ commandLine.addSubcommand(new AutoCompletion());
+
+ if (includeShell) {
+ commandLine.addSubcommand(new Shell(commandLine));
+ }
+
+ commandLine.addSubcommand(new Producer()).addSubcommand(new
Transfer()).addSubcommand(new Consumer()).addSubcommand(new
Browse()).addSubcommand(new Mask()).addSubcommand(new PrintVersion());
+
+ commandLine.addSubcommand(new PerfGroup(commandLine));
+ commandLine.addSubcommand(new CheckGroup(commandLine));
+ commandLine.addSubcommand(new QueueGroup(commandLine));
+ commandLine.addSubcommand(new AddressGroup(commandLine));
+
+ if (includeInstanceCommands) {
+ commandLine.addSubcommand(new ActivationGroup(commandLine));
+ commandLine.addSubcommand(new DataGroup(commandLine));
+ commandLine.addSubcommand(new UserGroup(commandLine));
+
+ commandLine.addSubcommand(new Run());
+ commandLine.addSubcommand(new Stop());
+ commandLine.addSubcommand(new Kill());
+ commandLine.addSubcommand(new PerfJournal());
+ }
+
+ if (includeHomeCommands) {
+ if (!includeInstanceCommands) {
+ // Data is already preent in InstanceCommands
Review Comment:
typo: preent
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
