You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/06/14 08:35:15 UTC
[GitHub] [ignite-3] valepakh opened a new pull request, #876: IGNITE-17093 Map error codes for cli commands
valepakh opened a new pull request, #876:
URL: https://github.com/apache/ignite-3/pull/876
https://issues.apache.org/jira/browse/IGNITE-17093
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] valepakh commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
valepakh commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896882364
##########
modules/cli/src/main/java/org/apache/ignite/cli/deprecated/spec/ClusterReplCommandSpec.java:
##########
@@ -84,7 +84,7 @@ public static class InitClusterCommandSpec extends BaseCommand {
/** {@inheritDoc} */
@Override
- public void run() {
+ public Integer call() {
Review Comment:
No it doesn't. I'll split the commands so the REPL commands will stay Runnables.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] valepakh commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
valepakh commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896830295
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/CliCommandsErrorTest.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertAll;
+
+import io.micronaut.configuration.picocli.MicronautFactory;
+import io.micronaut.context.ApplicationContext;
+import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
+import jakarta.inject.Inject;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigUpdateSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigUpdateSubCommand;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import picocli.CommandLine;
+
+/**
+ * Base class for testing error handling in CLI commands that use REST.
+ */
+@MicronautTest
+public class CliCommandsErrorTest {
+ private static final String NODE_URL = "http://localhost:10300";
+
+ @Inject
+ private ApplicationContext context;
+
+ private CommandLine cmd;
+ private StringWriter sout;
+ private StringWriter serr;
+ private int exitCode = Integer.MIN_VALUE;
+
+ private void setUp(Class<?> cmdClass) {
+ cmd = new CommandLine(cmdClass, new MicronautFactory(context));
+ sout = new StringWriter();
+ serr = new StringWriter();
+ cmd.setOut(new PrintWriter(sout));
+ cmd.setErr(new PrintWriter(serr));
+ }
+
+ private void execute(Class<?> cmdClass, String urlOptionName, String urlOptionValue, List<String> additionalOptions) {
+ setUp(cmdClass);
+ List<String> options = new ArrayList<>();
+ options.add("--" + urlOptionName + "=" + urlOptionValue);
Review Comment:
There are different option names and different option values which are passed from the parameters for tests from the cmdClassAndOptionsProvider method.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896896480
##########
modules/cli/src/main/java/org/apache/ignite/cli/call/connect/ConnectCall.java:
##########
@@ -54,20 +52,16 @@ public CallOutput<String> execute(ConnectCallInput input) {
try {
String configuration = api.getNodeConfiguration();
setJdbcUrl(configuration, nodeUrl);
- } catch (ApiException e) {
+ } catch (ApiException | IllegalArgumentException e) {
session.setConnectedToNode(false);
- if (e.getCause() instanceof ConnectException) {
- return DefaultCallOutput.failure(new ConnectCommandException("Can not connect to " + input.getNodeUrl()));
- }
- return DefaultCallOutput.failure(e);
+ return DefaultCallOutput.failure(new IgniteCliApiException(e, "get node configuration", nodeUrl));
Review Comment:
'get node configuration' -> 'connect'
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] valepakh commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
valepakh commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896885781
##########
modules/cli/src/main/java/org/apache/ignite/cli/core/exception/IgniteCliApiException.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.core.exception;
+
+/**
+ * Top level runtime exception for throwing the error message from REST API to user.
+ */
+public class IgniteCliApiException extends RuntimeException {
+
+ private final String call;
+ private final String url;
+
+ /**
+ * Creates a new instance of {@code IgniteCliApiException}.
+ *
+ * @param cause the cause.
+ * @param call REST API call.
+ * @param url endpoint URL.
+ */
+ public IgniteCliApiException(Throwable cause, String call, String url) {
+ super(cause);
+ this.call = call;
+ this.url = url;
+ }
+
+ /**
+ * Gets the REST API call.
Review Comment:
It was intented to be a call name, but probably it would be better to pass a real command name used.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896894341
##########
modules/cli/src/main/java/org/apache/ignite/cli/call/status/StatusReplCall.java:
##########
@@ -40,9 +39,7 @@
public class StatusReplCall implements Call<EmptyCallInput, Status> {
private final NodeManager nodeManager;
-
private final CliPathsConfigLoader cliPathsCfgLdr;
Review Comment:
I don't think that removing the empty lines is a good idea.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] valepakh closed pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
valepakh closed pull request #876: IGNITE-17093 Map error codes for cli commands
URL: https://github.com/apache/ignite-3/pull/876
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896822137
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/CliCommandsErrorTest.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertAll;
+
+import io.micronaut.configuration.picocli.MicronautFactory;
+import io.micronaut.context.ApplicationContext;
+import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
+import jakarta.inject.Inject;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigUpdateSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigUpdateSubCommand;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import picocli.CommandLine;
+
+/**
+ * Base class for testing error handling in CLI commands that use REST.
+ */
+@MicronautTest
+public class CliCommandsErrorTest {
+ private static final String NODE_URL = "http://localhost:10300";
+
+ @Inject
+ private ApplicationContext context;
+
+ private CommandLine cmd;
+ private StringWriter sout;
+ private StringWriter serr;
+ private int exitCode = Integer.MIN_VALUE;
+
+ private void setUp(Class<?> cmdClass) {
+ cmd = new CommandLine(cmdClass, new MicronautFactory(context));
+ sout = new StringWriter();
+ serr = new StringWriter();
+ cmd.setOut(new PrintWriter(sout));
+ cmd.setErr(new PrintWriter(serr));
+ }
+
+ private void execute(Class<?> cmdClass, String urlOptionName, String urlOptionValue, List<String> additionalOptions) {
+ setUp(cmdClass);
+ List<String> options = new ArrayList<>();
+ options.add("--" + urlOptionName + "=" + urlOptionValue);
Review Comment:
I believe explicit option passing is obvious in the context of testing CLI. Could we just use "--node-url=something"?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896891978
##########
modules/cli/src/main/java/org/apache/ignite/cli/commands/configuration/node/NodeConfigShowReplSubCommand.java:
##########
@@ -59,42 +56,23 @@ public class NodeConfigShowReplSubCommand extends BaseCommand {
/** {@inheritDoc} */
@Override
- public void run() {
+ public Integer call() {
var input = NodeConfigShowCallInput.builder().selector(selector);
if (session.isConnectedToNode()) {
input.nodeUrl(session.getNodeUrl());
} else if (nodeUrl != null) {
input.nodeUrl(nodeUrl);
} else {
spec.commandLine().getErr().println("You are not connected to node. Run 'connect' command or use '--node-url' option.");
- return;
+ return 1;
}
- CallExecutionPipeline.builder(call)
+ return CallExecutionPipeline.builder(call)
.inputProvider(input::build)
.output(spec.commandLine().getOut())
.errOutput(spec.commandLine().getErr())
.decorator(new JsonDecorator())
- .exceptionHandler(new ShowConfigReplExceptionHandler())
.build()
.runPipeline();
}
-
- private static class ShowConfigReplExceptionHandler implements ExceptionHandler<ApiException> {
Review Comment:
Whey did you remove this handler?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] valepakh commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
valepakh commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896931665
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/CliCommandsErrorTest.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertAll;
+
+import io.micronaut.configuration.picocli.MicronautFactory;
+import io.micronaut.context.ApplicationContext;
+import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
+import jakarta.inject.Inject;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigUpdateSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigUpdateSubCommand;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import picocli.CommandLine;
+
+/**
+ * Base class for testing error handling in CLI commands that use REST.
+ */
+@MicronautTest
+public class CliCommandsErrorTest {
+ private static final String NODE_URL = "http://localhost:10300";
+
+ @Inject
+ private ApplicationContext context;
+
+ private CommandLine cmd;
+ private StringWriter sout;
+ private StringWriter serr;
+ private int exitCode = Integer.MIN_VALUE;
+
+ private void setUp(Class<?> cmdClass) {
+ cmd = new CommandLine(cmdClass, new MicronautFactory(context));
+ sout = new StringWriter();
+ serr = new StringWriter();
+ cmd.setOut(new PrintWriter(sout));
+ cmd.setErr(new PrintWriter(serr));
+ }
+
+ private void execute(Class<?> cmdClass, String urlOptionName, String urlOptionValue, List<String> additionalOptions) {
+ setUp(cmdClass);
+ List<String> options = new ArrayList<>();
+ options.add("--" + urlOptionName + "=" + urlOptionValue);
+ options.addAll(additionalOptions);
+ exitCode = cmd.execute(options.toArray(new String[0]));
+ }
+
+ static List<Arguments> cmdClassAndOptionsProvider() {
+ return List.of(
+ Arguments.arguments(NodeConfigShowSubCommand.class, "node-url", List.of()),
Review Comment:
As to why the local installation is required for the status command - because the REST endpoint is not implemented yet? It's used to get a number of running nodes.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896902803
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/CliCommandsErrorTest.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertAll;
+
+import io.micronaut.configuration.picocli.MicronautFactory;
+import io.micronaut.context.ApplicationContext;
+import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
+import jakarta.inject.Inject;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigUpdateSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigUpdateSubCommand;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import picocli.CommandLine;
+
+/**
+ * Base class for testing error handling in CLI commands that use REST.
+ */
+@MicronautTest
+public class CliCommandsErrorTest {
+ private static final String NODE_URL = "http://localhost:10300";
+
+ @Inject
+ private ApplicationContext context;
+
+ private CommandLine cmd;
+ private StringWriter sout;
+ private StringWriter serr;
+ private int exitCode = Integer.MIN_VALUE;
+
+ private void setUp(Class<?> cmdClass) {
+ cmd = new CommandLine(cmdClass, new MicronautFactory(context));
+ sout = new StringWriter();
+ serr = new StringWriter();
+ cmd.setOut(new PrintWriter(sout));
+ cmd.setErr(new PrintWriter(serr));
+ }
+
+ private void execute(Class<?> cmdClass, String urlOptionName, String urlOptionValue, List<String> additionalOptions) {
+ setUp(cmdClass);
+ List<String> options = new ArrayList<>();
+ options.add("--" + urlOptionName + "=" + urlOptionValue);
Review Comment:
What stops you from passing '--cluster-url' instead of 'cluster-url' in cmdClassAndOptionsProvider?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896832512
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/CliCommandsErrorTest.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertAll;
+
+import io.micronaut.configuration.picocli.MicronautFactory;
+import io.micronaut.context.ApplicationContext;
+import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
+import jakarta.inject.Inject;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigUpdateSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigUpdateSubCommand;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import picocli.CommandLine;
+
+/**
+ * Base class for testing error handling in CLI commands that use REST.
+ */
+@MicronautTest
+public class CliCommandsErrorTest {
+ private static final String NODE_URL = "http://localhost:10300";
+
+ @Inject
+ private ApplicationContext context;
+
+ private CommandLine cmd;
+ private StringWriter sout;
+ private StringWriter serr;
+ private int exitCode = Integer.MIN_VALUE;
+
+ private void setUp(Class<?> cmdClass) {
+ cmd = new CommandLine(cmdClass, new MicronautFactory(context));
+ sout = new StringWriter();
+ serr = new StringWriter();
+ cmd.setOut(new PrintWriter(sout));
+ cmd.setErr(new PrintWriter(serr));
+ }
+
+ private void execute(Class<?> cmdClass, String urlOptionName, String urlOptionValue, List<String> additionalOptions) {
+ setUp(cmdClass);
+ List<String> options = new ArrayList<>();
+ options.add("--" + urlOptionName + "=" + urlOptionValue);
+ options.addAll(additionalOptions);
+ exitCode = cmd.execute(options.toArray(new String[0]));
+ }
+
+ static List<Arguments> cmdClassAndOptionsProvider() {
+ return List.of(
+ Arguments.arguments(NodeConfigShowSubCommand.class, "node-url", List.of()),
+ Arguments.arguments(NodeConfigUpdateSubCommand.class, "node-url", List.of("config")),
+ Arguments.arguments(ClusterConfigShowSubCommand.class, "cluster-url", List.of()),
+ Arguments.arguments(ClusterConfigUpdateSubCommand.class, "cluster-url", List.of("config"))
Review Comment:
Could you use more real world value like '{key: value}'. Than it is obvious what is beeng tested here.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] valepakh commented on pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
valepakh commented on PR #876:
URL: https://github.com/apache/ignite-3/pull/876#issuecomment-1155121547
This PR also fixes some of the problems in the https://issues.apache.org/jira/browse/IGNITE-17109, see first comment.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896839737
##########
modules/cli/src/main/java/org/apache/ignite/cli/deprecated/spec/ClusterReplCommandSpec.java:
##########
@@ -84,7 +84,7 @@ public static class InitClusterCommandSpec extends BaseCommand {
/** {@inheritDoc} */
@Override
- public void run() {
+ public Integer call() {
Review Comment:
Does exit code make any sense in REPL? If no then I don't think we even have to return it.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896879412
##########
modules/cli/src/main/java/org/apache/ignite/cli/core/call/CallExecutionPipeline.java:
##########
@@ -90,24 +90,25 @@ public static <I extends CallInput, T> CallExecutionPipelineBuilder<I, T> builde
return new CallExecutionPipelineBuilder<>(call);
}
- /** {@inheritDoc} */
- public void runPipeline() {
+ /**
+ * Runs the pipeline.
+ *
+ * @return exit code.
+ */
+ public int runPipeline() {
I callInput = inputProvider.get();
CallOutput<T> callOutput = call.execute(callInput);
if (callOutput.hasError()) {
- exceptionHandlers.handleException(ExceptionWriter.fromPrintWriter(errOutput), callOutput.errorCause());
- return;
+ return exceptionHandlers.handleException(ExceptionWriter.fromPrintWriter(errOutput), callOutput.errorCause());
}
- if (callOutput.isEmpty()) {
- return;
+ if (!callOutput.isEmpty()) {
Review Comment:
Why did you decided to revert the expresion? Now it is less readable but I can't get the reason.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] valepakh commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
valepakh commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896896021
##########
modules/cli/src/main/java/org/apache/ignite/cli/commands/configuration/node/NodeConfigShowReplSubCommand.java:
##########
@@ -59,42 +56,23 @@ public class NodeConfigShowReplSubCommand extends BaseCommand {
/** {@inheritDoc} */
@Override
- public void run() {
+ public Integer call() {
var input = NodeConfigShowCallInput.builder().selector(selector);
if (session.isConnectedToNode()) {
input.nodeUrl(session.getNodeUrl());
} else if (nodeUrl != null) {
input.nodeUrl(nodeUrl);
} else {
spec.commandLine().getErr().println("You are not connected to node. Run 'connect' command or use '--node-url' option.");
- return;
+ return 1;
}
- CallExecutionPipeline.builder(call)
+ return CallExecutionPipeline.builder(call)
.inputProvider(input::build)
.output(spec.commandLine().getOut())
.errOutput(spec.commandLine().getErr())
.decorator(new JsonDecorator())
- .exceptionHandler(new ShowConfigReplExceptionHandler())
.build()
.runPipeline();
}
-
- private static class ShowConfigReplExceptionHandler implements ExceptionHandler<ApiException> {
Review Comment:
Running node returns its config regardless of whether the cluster is initialized or not.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896767816
##########
modules/cli/src/integrationTest/java/org/apache/ignite/cli/deprecated/ItConfigCommandTest.java:
##########
@@ -123,10 +123,10 @@ public void setWithWrongData() {
"network.foo=\"bar\""
);
- //assertEquals(1, exitCode); // TODO
+ assertEquals(1, exitCode);
assertThat(
err.toString(UTF_8),
- both(startsWith("Command node config update failed with reason: Got error while updating the node configuration."))
+ both(startsWith("An error occurred when calling \"update node configuration\""))
Review Comment:
'update node configuration' is not the name of existing command
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896773724
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/cliconfig/CliConfigSubCommandTest.java:
##########
@@ -37,11 +36,12 @@ void noKey() {
// When executed without arguments
execute();
- // Then
Review Comment:
If you remove '// Than', so remove '// When' as well
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r898966697
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/UrlOptionsNegativeTest.java:
##########
@@ -64,28 +69,45 @@ private void setUp(Class<?> cmdClass) {
private void execute(Class<?> cmdClass, String urlOptionName, String urlOptionValue, List<String> additionalOptions) {
setUp(cmdClass);
List<String> options = new ArrayList<>();
- options.add("--" + urlOptionName + "=" + urlOptionValue);
+ options.add(urlOptionName + urlOptionValue);
options.addAll(additionalOptions);
exitCode = cmd.execute(options.toArray(new String[0]));
}
static List<Arguments> cmdClassAndOptionsProvider() {
return List.of(
- Arguments.arguments(NodeConfigShowSubCommand.class, "node-url", List.of()),
- Arguments.arguments(NodeConfigUpdateSubCommand.class, "node-url", List.of("config")),
- Arguments.arguments(ClusterConfigShowSubCommand.class, "cluster-url", List.of()),
- Arguments.arguments(ClusterConfigUpdateSubCommand.class, "cluster-url", List.of("config"))
+ Arguments.arguments(NodeConfigShowSubCommand.class, "--node-url=", List.of(), false),
+ Arguments.arguments(NodeConfigUpdateSubCommand.class, "--node-url=", List.of("{key: value}"), false),
+ Arguments.arguments(ClusterConfigShowSubCommand.class, "--cluster-url=", List.of(), false),
+ Arguments.arguments(ClusterConfigUpdateSubCommand.class, "--cluster-url=", List.of("{key: value}"), false),
+ Arguments.arguments(NodeConfigShowReplSubCommand.class, "--node-url=", List.of(), true),
+ Arguments.arguments(NodeConfigUpdateReplSubCommand.class, "--node-url=", List.of("{key: value}"), true),
+ Arguments.arguments(ClusterConfigShowReplSubCommand.class, "--cluster-url=", List.of(), true),
+ Arguments.arguments(ClusterConfigUpdateReplSubCommand.class, "--cluster-url=", List.of("{key: value}"), true),
+ Arguments.arguments(ConnectCommand.class, "", List.of(), true)
+ // TODO https://issues.apache.org/jira/browse/IGNITE-17091
+ // Arguments.arguments(StatusCommand.class, "--cluster-url=", List.of()),
+ // TODO https://issues.apache.org/jira/browse/IGNITE-17102
+ // Arguments.arguments(ClusterShowCommand.class, "--cluster-url=", List.of()),
+ // TODO https://issues.apache.org/jira/browse/IGNITE-17092
+ // Arguments.arguments(TopologyCommand.class, "--cluster-url", List.of()),
+ // TODO https://issues.apache.org/jira/browse/IGNITE-17162
+ // Arguments.arguments(ClusterCommandSpec.InitClusterCommandSpec.class, "---cluster-url=",
+ // List.of("--cluster-name=cluster", "--meta-storage-node=test"))
);
}
@ParameterizedTest
@MethodSource("cmdClassAndOptionsProvider")
@DisplayName("Should display error when wrong port is given")
- public void incorrectPortTest(Class<?> cmdClass, String urlOptionName, List<String> additionalOptions) {
+ void incorrectPort(Class<?> cmdClass, String urlOptionName, List<String> additionalOptions, boolean isReplCommand) {
Review Comment:
Just split this test into two repli and non-repl. Please, don't use flags like `isReplCommand`
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] kgusakov commented on pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
kgusakov commented on PR #876:
URL: https://github.com/apache/ignite-3/pull/876#issuecomment-1163054835
Shouldn't TODOs with the 17093 issue number be removed (or maybe recreated with another number) or fixed in this PR?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896906025
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/CliCommandsErrorTest.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertAll;
+
+import io.micronaut.configuration.picocli.MicronautFactory;
+import io.micronaut.context.ApplicationContext;
+import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
+import jakarta.inject.Inject;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigUpdateSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigUpdateSubCommand;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import picocli.CommandLine;
+
+/**
+ * Base class for testing error handling in CLI commands that use REST.
+ */
+@MicronautTest
+public class CliCommandsErrorTest {
+ private static final String NODE_URL = "http://localhost:10300";
+
+ @Inject
+ private ApplicationContext context;
+
+ private CommandLine cmd;
+ private StringWriter sout;
+ private StringWriter serr;
+ private int exitCode = Integer.MIN_VALUE;
+
+ private void setUp(Class<?> cmdClass) {
+ cmd = new CommandLine(cmdClass, new MicronautFactory(context));
+ sout = new StringWriter();
+ serr = new StringWriter();
+ cmd.setOut(new PrintWriter(sout));
+ cmd.setErr(new PrintWriter(serr));
+ }
+
+ private void execute(Class<?> cmdClass, String urlOptionName, String urlOptionValue, List<String> additionalOptions) {
+ setUp(cmdClass);
+ List<String> options = new ArrayList<>();
+ options.add("--" + urlOptionName + "=" + urlOptionValue);
+ options.addAll(additionalOptions);
+ exitCode = cmd.execute(options.toArray(new String[0]));
+ }
+
+ static List<Arguments> cmdClassAndOptionsProvider() {
+ return List.of(
+ Arguments.arguments(NodeConfigShowSubCommand.class, "node-url", List.of()),
Review Comment:
> Connect and status commands currently rely on the local installation so they should be tested in the integration tests. ,
I can pass `--cluster-url` option to them, right? Why is local installation required?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896912724
##########
modules/cli/src/main/java/org/apache/ignite/cli/call/status/StatusReplCall.java:
##########
@@ -40,9 +39,7 @@
public class StatusReplCall implements Call<EmptyCallInput, Status> {
private final NodeManager nodeManager;
-
private final CliPathsConfigLoader cliPathsCfgLdr;
Review Comment:
The whole Ignite 3 source code uses line breaks. I believe it is better not to break the convention even if something looks more readable.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896888600
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/CliCommandsErrorTest.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertAll;
+
+import io.micronaut.configuration.picocli.MicronautFactory;
+import io.micronaut.context.ApplicationContext;
+import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
+import jakarta.inject.Inject;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigUpdateSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigUpdateSubCommand;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import picocli.CommandLine;
+
+/**
+ * Base class for testing error handling in CLI commands that use REST.
+ */
+@MicronautTest
+public class CliCommandsErrorTest {
Review Comment:
I would name this test 'UrlOptionsNegativeTest'. Then it is clear what is the purpuse of this test and when a developer has to add more cases here.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896888841
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/CliCommandsErrorTest.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertAll;
+
+import io.micronaut.configuration.picocli.MicronautFactory;
+import io.micronaut.context.ApplicationContext;
+import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
+import jakarta.inject.Inject;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigUpdateSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigUpdateSubCommand;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import picocli.CommandLine;
+
+/**
+ * Base class for testing error handling in CLI commands that use REST.
Review Comment:
It is not a base class.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] kgusakov commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
kgusakov commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r903692486
##########
modules/cli/src/main/java/org/apache/ignite/cli/call/status/StatusCall.java:
##########
@@ -41,7 +40,6 @@
public class StatusCall implements Call<StatusCallInput, Status> {
private final NodeManager nodeManager;
-
Review Comment:
It was correct before the change :)
##########
modules/cli/src/main/java/org/apache/ignite/cli/call/status/StatusReplCall.java:
##########
@@ -40,9 +39,7 @@
public class StatusReplCall implements Call<EmptyCallInput, Status> {
Review Comment:
Deprecated TODO?
##########
modules/cli/src/main/java/org/apache/ignite/cli/call/status/StatusCall.java:
##########
@@ -41,7 +40,6 @@
public class StatusCall implements Call<StatusCallInput, Status> {
Review Comment:
I suppose TODO should be removed, doesn't it?
##########
modules/cli/src/main/java/org/apache/ignite/cli/commands/sql/SqlCommand.java:
##########
@@ -17,109 +17,70 @@
package org.apache.ignite.cli.commands.sql;
-import jakarta.inject.Inject;
import java.io.File;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.sql.SQLException;
+import java.util.concurrent.Callable;
+import org.apache.ignite.cli.call.sql.SqlQueryCall;
import org.apache.ignite.cli.commands.BaseCommand;
import org.apache.ignite.cli.commands.decorators.SqlQueryResultDecorator;
-import org.apache.ignite.cli.core.CallExecutionPipelineProvider;
import org.apache.ignite.cli.core.call.CallExecutionPipeline;
import org.apache.ignite.cli.core.call.StringCallInput;
-import org.apache.ignite.cli.core.exception.CommandExecutionException;
-import org.apache.ignite.cli.core.exception.ExceptionHandlers;
import org.apache.ignite.cli.core.exception.ExceptionWriter;
-import org.apache.ignite.cli.core.exception.handler.DefaultExceptionHandlers;
import org.apache.ignite.cli.core.exception.handler.SqlExceptionHandler;
-import org.apache.ignite.cli.core.repl.Repl;
-import org.apache.ignite.cli.core.repl.executor.RegistryCommandExecutor;
-import org.apache.ignite.cli.core.repl.executor.ReplExecutorProvider;
-import org.apache.ignite.cli.core.repl.executor.SqlQueryCall;
+import org.apache.ignite.cli.deprecated.IgniteCliException;
import org.apache.ignite.cli.sql.SqlManager;
-import org.apache.ignite.cli.sql.SqlSchemaProvider;
-import org.jetbrains.annotations.NotNull;
+import picocli.CommandLine.ArgGroup;
import picocli.CommandLine.Command;
import picocli.CommandLine.Option;
/**
* Command for sql execution.
*/
@Command(name = "sql", description = "Executes SQL query.")
-public class SqlCommand extends BaseCommand {
- private static final String INTERNAL_COMMAND_PREFIX = "!";
+public class SqlCommand extends BaseCommand implements Callable<Integer> {
@Option(names = {"-u", "--jdbc-url"}, required = true,
descriptionKey = "ignite.jdbc-url", description = "JDBC url to ignite cluster")
private String jdbc;
- @Option(names = {"-e", "--execute", "--exec"}) //todo: can be passed as parameter, not option (see IEP-88)
- private String command;
+ @ArgGroup(multiplicity = "1")
+ private ExecOptions execOptions;
- @Option(names = {"-f", "--script-file"})
- private File file;
+ private static class ExecOptions {
+ @Option(names = {"-e", "--execute", "--exec"}) //todo: can be passed as parameter, not option (see IEP-88)
Review Comment:
TODO must contain the JIRA issue number.
##########
modules/cli/src/main/java/org/apache/ignite/cli/commands/configuration/cluster/ClusterConfigUpdateReplSubCommand.java:
##########
@@ -57,7 +57,6 @@ public class ClusterConfigUpdateReplSubCommand extends BaseCommand {
private Session session;
/** {@inheritDoc} */
- @Override
Review Comment:
Removed by mistake?
##########
modules/cli/src/main/java/org/apache/ignite/cli/core/exception/handler/TimeoutExceptionHandler.java:
##########
@@ -29,9 +29,10 @@ public class TimeoutExceptionHandler implements ExceptionHandler<TimeoutExceptio
private static final IgniteLogger log = IgniteLogger.forClass(TimeoutExceptionHandler.class);
@Override
- public void handle(ExceptionWriter err, TimeoutException e) {
+ public int handle(ExceptionWriter err, TimeoutException e) {
log.error("Timeout exception ", e);
err.write("Command failed with timeout.");
+ return 1;
Review Comment:
Please, add empty lines according to style guide.
##########
modules/cli/src/main/java/org/apache/ignite/cli/core/exception/handler/IgniteCliApiExceptionHandler.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.core.exception.handler;
+
+import java.net.ConnectException;
+import java.net.UnknownHostException;
+import org.apache.ignite.cli.core.exception.ExceptionHandler;
+import org.apache.ignite.cli.core.exception.ExceptionWriter;
+import org.apache.ignite.cli.core.exception.IgniteCliApiException;
+import org.apache.ignite.lang.IgniteLogger;
+import org.apache.ignite.rest.client.invoker.ApiException;
+
+/**
+ * Exception handler for {@link IgniteCliApiException}.
+ */
+public class IgniteCliApiExceptionHandler implements ExceptionHandler<IgniteCliApiException> {
+ private static final IgniteLogger log = IgniteLogger.forClass(IgniteCliApiExceptionHandler.class);
+
+ @Override
+ public int handle(ExceptionWriter err, IgniteCliApiException e) {
+ String message;
+ if (e.getCause() instanceof ApiException) {
+ ApiException cause = (ApiException) e.getCause();
+ Throwable apiCause = cause.getCause();
+ if (apiCause instanceof UnknownHostException) {
+ message = "Could not determine IP address when connecting to URL: " + e.getUrl();
+ } else if (apiCause instanceof ConnectException) {
+ message = "Could not connect to URL: " + e.getUrl();
+ } else if (apiCause != null) {
+ message = apiCause.getMessage();
+ } else {
+ message = "An error occurred, error code: " + cause.getCode() + ", response: " + cause.getResponseBody();
+ }
+ } else {
+ message = e.getCause() != e ? e.getCause().getMessage() : e.getMessage();
+ }
+ log.error(message, e);
Review Comment:
```suggestion
log.error(message, e);
```
```suggestion
log.error(message, e);
```
##########
modules/cli/src/main/java/org/apache/ignite/cli/core/exception/handler/IgniteCliApiExceptionHandler.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.core.exception.handler;
+
+import java.net.ConnectException;
+import java.net.UnknownHostException;
+import org.apache.ignite.cli.core.exception.ExceptionHandler;
+import org.apache.ignite.cli.core.exception.ExceptionWriter;
+import org.apache.ignite.cli.core.exception.IgniteCliApiException;
+import org.apache.ignite.lang.IgniteLogger;
+import org.apache.ignite.rest.client.invoker.ApiException;
+
+/**
+ * Exception handler for {@link IgniteCliApiException}.
+ */
+public class IgniteCliApiExceptionHandler implements ExceptionHandler<IgniteCliApiException> {
+ private static final IgniteLogger log = IgniteLogger.forClass(IgniteCliApiExceptionHandler.class);
+
+ @Override
+ public int handle(ExceptionWriter err, IgniteCliApiException e) {
+ String message;
+ if (e.getCause() instanceof ApiException) {
+ ApiException cause = (ApiException) e.getCause();
+ Throwable apiCause = cause.getCause();
+ if (apiCause instanceof UnknownHostException) {
+ message = "Could not determine IP address when connecting to URL: " + e.getUrl();
+ } else if (apiCause instanceof ConnectException) {
+ message = "Could not connect to URL: " + e.getUrl();
+ } else if (apiCause != null) {
+ message = apiCause.getMessage();
+ } else {
+ message = "An error occurred, error code: " + cause.getCode() + ", response: " + cause.getResponseBody();
+ }
+ } else {
+ message = e.getCause() != e ? e.getCause().getMessage() : e.getMessage();
+ }
+ log.error(message, e);
+ err.write(message);
Review Comment:
```suggestion
err.write(message);
```
```suggestion
err.write(message);
```
##########
modules/cli/src/main/java/org/apache/ignite/cli/core/exception/handler/IgniteCliApiExceptionHandler.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.core.exception.handler;
+
+import java.net.ConnectException;
+import java.net.UnknownHostException;
+import org.apache.ignite.cli.core.exception.ExceptionHandler;
+import org.apache.ignite.cli.core.exception.ExceptionWriter;
+import org.apache.ignite.cli.core.exception.IgniteCliApiException;
+import org.apache.ignite.lang.IgniteLogger;
+import org.apache.ignite.rest.client.invoker.ApiException;
+
+/**
+ * Exception handler for {@link IgniteCliApiException}.
+ */
+public class IgniteCliApiExceptionHandler implements ExceptionHandler<IgniteCliApiException> {
+ private static final IgniteLogger log = IgniteLogger.forClass(IgniteCliApiExceptionHandler.class);
+
+ @Override
+ public int handle(ExceptionWriter err, IgniteCliApiException e) {
+ String message;
+ if (e.getCause() instanceof ApiException) {
+ ApiException cause = (ApiException) e.getCause();
+ Throwable apiCause = cause.getCause();
+ if (apiCause instanceof UnknownHostException) {
+ message = "Could not determine IP address when connecting to URL: " + e.getUrl();
+ } else if (apiCause instanceof ConnectException) {
+ message = "Could not connect to URL: " + e.getUrl();
+ } else if (apiCause != null) {
+ message = apiCause.getMessage();
+ } else {
+ message = "An error occurred, error code: " + cause.getCode() + ", response: " + cause.getResponseBody();
+ }
+ } else {
+ message = e.getCause() != e ? e.getCause().getMessage() : e.getMessage();
+ }
Review Comment:
```suggestion
}
```
```suggestion
}
```
##########
modules/cli/src/main/java/org/apache/ignite/cli/deprecated/spec/BootstrapIgniteCommandSpec.java:
##########
@@ -44,7 +45,8 @@ public class BootstrapIgniteCommandSpec extends BaseCommand implements IgniteCom
/** {@inheritDoc} */
@Override
- public void run() {
+ public Integer call() {
cmd.init(urls, spec.commandLine().getOut(), spec.commandLine().getColorScheme());
+ return 0;
Review Comment:
Empty line before, pls.
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/CliCommandTestBase.java:
##########
@@ -17,39 +17,100 @@
package org.apache.ignite.cli.commands;
+import static org.assertj.core.api.Assertions.assertThat;
+
import io.micronaut.configuration.picocli.MicronautFactory;
import io.micronaut.context.ApplicationContext;
import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
import jakarta.inject.Inject;
import java.io.PrintWriter;
import java.io.StringWriter;
+import org.junit.jupiter.api.BeforeEach;
import picocli.CommandLine;
/**
* Base class for testing CLI commands.
*/
@MicronautTest
-public class CliCommandTestBase {
+public abstract class CliCommandTestBase {
@Inject
private ApplicationContext context;
- private CommandLine commandLine;
+ private CommandLine cmd;
Review Comment:
Empty lines here and below pls
##########
modules/cli/src/main/java/org/apache/ignite/cli/core/exception/handler/IgniteCliApiExceptionHandler.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.core.exception.handler;
+
+import java.net.ConnectException;
+import java.net.UnknownHostException;
+import org.apache.ignite.cli.core.exception.ExceptionHandler;
+import org.apache.ignite.cli.core.exception.ExceptionWriter;
+import org.apache.ignite.cli.core.exception.IgniteCliApiException;
+import org.apache.ignite.lang.IgniteLogger;
+import org.apache.ignite.rest.client.invoker.ApiException;
+
+/**
+ * Exception handler for {@link IgniteCliApiException}.
+ */
+public class IgniteCliApiExceptionHandler implements ExceptionHandler<IgniteCliApiException> {
+ private static final IgniteLogger log = IgniteLogger.forClass(IgniteCliApiExceptionHandler.class);
+
+ @Override
+ public int handle(ExceptionWriter err, IgniteCliApiException e) {
+ String message;
Review Comment:
```suggestion
String message;
```
```suggestion
String message;
```
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/UrlOptionsNegativeTest.java:
##########
@@ -0,0 +1,232 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertAll;
+
+import io.micronaut.configuration.picocli.MicronautFactory;
+import io.micronaut.context.ApplicationContext;
+import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
+import jakarta.inject.Inject;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigShowReplSubCommand;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigUpdateReplSubCommand;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigUpdateSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigShowReplSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigUpdateReplSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigUpdateSubCommand;
+import org.apache.ignite.cli.commands.connect.ConnectCommand;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import picocli.CommandLine;
+
+/**
+ * Tests error handling with various invalid URLs in CLI commands that use REST.
+ */
+@MicronautTest
+public class UrlOptionsNegativeTest {
+ private static final String NODE_URL = "http://localhost:10300";
+
+ @Inject
+ private ApplicationContext context;
+
+ private CommandLine cmd;
Review Comment:
Empty lines
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] valepakh commented on pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
valepakh commented on PR #876:
URL: https://github.com/apache/ignite-3/pull/876#issuecomment-1163062414
> Shouldn't TODOs with the 17093 issue number be removed (or maybe recreated with another number) or fixed in this PR?
Yes, I'll remove TODOs in the Status commands - they were for the IgniteExceptions possible thrown by cliPathsCfgLdr and change them to 17090 in the SqlSchemaLoader were they will be fied.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] valepakh commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
valepakh commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896888549
##########
modules/cli/src/main/java/org/apache/ignite/cli/core/call/CallExecutionPipeline.java:
##########
@@ -90,24 +90,25 @@ public static <I extends CallInput, T> CallExecutionPipelineBuilder<I, T> builde
return new CallExecutionPipelineBuilder<>(call);
}
- /** {@inheritDoc} */
- public void runPipeline() {
+ /**
+ * Runs the pipeline.
+ *
+ * @return exit code.
+ */
+ public int runPipeline() {
I callInput = inputProvider.get();
CallOutput<T> callOutput = call.execute(callInput);
if (callOutput.hasError()) {
- exceptionHandlers.handleException(ExceptionWriter.fromPrintWriter(errOutput), callOutput.errorCause());
- return;
+ return exceptionHandlers.handleException(ExceptionWriter.fromPrintWriter(errOutput), callOutput.errorCause());
}
- if (callOutput.isEmpty()) {
- return;
+ if (!callOutput.isEmpty()) {
Review Comment:
Because otherwize two return statements will be needed and this would be less readable than negative condition IMO.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] valepakh commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
valepakh commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896886506
##########
modules/cli/src/main/java/org/apache/ignite/cli/core/exception/handler/UnknownCommandExceptionHandler.java:
##########
@@ -28,8 +28,9 @@
public class UnknownCommandExceptionHandler implements ExceptionHandler<UnknownCommandException> {
@Override
- public void handle(ExceptionWriter err, UnknownCommandException e) {
+ public int handle(ExceptionWriter err, UnknownCommandException e) {
err.write(e.getMessage());
+ return 1;
Review Comment:
This is only for REPL mode so this it irrelevant, I'll change to 2 and add a comment.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896904338
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/CliCommandsErrorTest.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertAll;
+
+import io.micronaut.configuration.picocli.MicronautFactory;
+import io.micronaut.context.ApplicationContext;
+import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
+import jakarta.inject.Inject;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigUpdateSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigUpdateSubCommand;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import picocli.CommandLine;
+
+/**
+ * Base class for testing error handling in CLI commands that use REST.
+ */
+@MicronautTest
+public class CliCommandsErrorTest {
+ private static final String NODE_URL = "http://localhost:10300";
+
+ @Inject
+ private ApplicationContext context;
+
+ private CommandLine cmd;
+ private StringWriter sout;
+ private StringWriter serr;
+ private int exitCode = Integer.MIN_VALUE;
+
+ private void setUp(Class<?> cmdClass) {
+ cmd = new CommandLine(cmdClass, new MicronautFactory(context));
+ sout = new StringWriter();
+ serr = new StringWriter();
+ cmd.setOut(new PrintWriter(sout));
+ cmd.setErr(new PrintWriter(serr));
+ }
+
+ private void execute(Class<?> cmdClass, String urlOptionName, String urlOptionValue, List<String> additionalOptions) {
+ setUp(cmdClass);
+ List<String> options = new ArrayList<>();
+ options.add("--" + urlOptionName + "=" + urlOptionValue);
+ options.addAll(additionalOptions);
+ exitCode = cmd.execute(options.toArray(new String[0]));
+ }
+
+ static List<Arguments> cmdClassAndOptionsProvider() {
+ return List.of(
+ Arguments.arguments(NodeConfigShowSubCommand.class, "node-url", List.of()),
Review Comment:
Agreed, disabled test with the link to jira would be a good solution.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896895635
##########
modules/cli/src/main/java/org/apache/ignite/cli/call/status/StatusCall.java:
##########
@@ -55,32 +53,27 @@ public StatusCall(NodeManager nodeManager, CliPathsConfigLoader cliPathsCfgLdr)
@Override
public CallOutput<Status> execute(StatusCallInput input) {
IgnitePaths paths = cliPathsCfgLdr.loadIgnitePathsOrThrowError();
- String connected = null;
try {
- connected = createNodeApi(input).getNodeConfiguration();
- } catch (ApiException e) {
- if (e.getCause() instanceof ConnectException) {
- return DefaultCallOutput.failure(new CommandExecutionException("status", "cannot connect to " + input.getClusterUrl()));
- } else {
- return DefaultCallOutput.failure(e);
- }
+ String connected = createNodeApi(input).getNodeConfiguration();
+ return DefaultCallOutput.success(
+ Status.builder()
+ .connected(connected != null)
+ .connectedNodeUrl(input.getClusterUrl())
+ .initialized(connected != null && canReadClusterConfig(input))
+ .nodeCount(nodeManager.getRunningNodes(paths.logDir, paths.cliPidsDir()).size())
+ .build()
+ );
+ } catch (ApiException | IllegalArgumentException e) {
+ return DefaultCallOutput.failure(new IgniteCliApiException(e, "get node configuration", input.getClusterUrl()));
Review Comment:
'get node configuration' -> 'status'
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] valepakh commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
valepakh commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896931034
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/CliCommandsErrorTest.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertAll;
+
+import io.micronaut.configuration.picocli.MicronautFactory;
+import io.micronaut.context.ApplicationContext;
+import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
+import jakarta.inject.Inject;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigUpdateSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigUpdateSubCommand;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import picocli.CommandLine;
+
+/**
+ * Base class for testing error handling in CLI commands that use REST.
+ */
+@MicronautTest
+public class CliCommandsErrorTest {
+ private static final String NODE_URL = "http://localhost:10300";
+
+ @Inject
+ private ApplicationContext context;
+
+ private CommandLine cmd;
+ private StringWriter sout;
+ private StringWriter serr;
+ private int exitCode = Integer.MIN_VALUE;
+
+ private void setUp(Class<?> cmdClass) {
+ cmd = new CommandLine(cmdClass, new MicronautFactory(context));
+ sout = new StringWriter();
+ serr = new StringWriter();
+ cmd.setOut(new PrintWriter(sout));
+ cmd.setErr(new PrintWriter(serr));
+ }
+
+ private void execute(Class<?> cmdClass, String urlOptionName, String urlOptionValue, List<String> additionalOptions) {
+ setUp(cmdClass);
+ List<String> options = new ArrayList<>();
+ options.add("--" + urlOptionName + "=" + urlOptionValue);
+ options.addAll(additionalOptions);
+ exitCode = cmd.execute(options.toArray(new String[0]));
+ }
+
+ static List<Arguments> cmdClassAndOptionsProvider() {
+ return List.of(
+ Arguments.arguments(NodeConfigShowSubCommand.class, "node-url", List.of()),
Review Comment:
Ah, sorry, connect command doesn't use the --cluster-url option so some changes in the tests are required to test it.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] valepakh commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
valepakh commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896863316
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/CliCommandsErrorTest.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertAll;
+
+import io.micronaut.configuration.picocli.MicronautFactory;
+import io.micronaut.context.ApplicationContext;
+import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
+import jakarta.inject.Inject;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigUpdateSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigUpdateSubCommand;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import picocli.CommandLine;
+
+/**
+ * Base class for testing error handling in CLI commands that use REST.
+ */
+@MicronautTest
+public class CliCommandsErrorTest {
+ private static final String NODE_URL = "http://localhost:10300";
+
+ @Inject
+ private ApplicationContext context;
+
+ private CommandLine cmd;
+ private StringWriter sout;
+ private StringWriter serr;
+ private int exitCode = Integer.MIN_VALUE;
+
+ private void setUp(Class<?> cmdClass) {
+ cmd = new CommandLine(cmdClass, new MicronautFactory(context));
+ sout = new StringWriter();
+ serr = new StringWriter();
+ cmd.setOut(new PrintWriter(sout));
+ cmd.setErr(new PrintWriter(serr));
+ }
+
+ private void execute(Class<?> cmdClass, String urlOptionName, String urlOptionValue, List<String> additionalOptions) {
+ setUp(cmdClass);
+ List<String> options = new ArrayList<>();
+ options.add("--" + urlOptionName + "=" + urlOptionValue);
+ options.addAll(additionalOptions);
+ exitCode = cmd.execute(options.toArray(new String[0]));
+ }
+
+ static List<Arguments> cmdClassAndOptionsProvider() {
+ return List.of(
+ Arguments.arguments(NodeConfigShowSubCommand.class, "node-url", List.of()),
Review Comment:
Connect and status commands currently rely on the local installation so they should be tested in the integration tests.
Cluster init have different semantic for the node-endpoint option.
I would probably add disabled test variants for these commands until the commands are implemented using REST API.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896830166
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/CliCommandsErrorTest.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertAll;
+
+import io.micronaut.configuration.picocli.MicronautFactory;
+import io.micronaut.context.ApplicationContext;
+import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
+import jakarta.inject.Inject;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigUpdateSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigUpdateSubCommand;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import picocli.CommandLine;
+
+/**
+ * Base class for testing error handling in CLI commands that use REST.
+ */
+@MicronautTest
+public class CliCommandsErrorTest {
+ private static final String NODE_URL = "http://localhost:10300";
+
+ @Inject
+ private ApplicationContext context;
+
+ private CommandLine cmd;
+ private StringWriter sout;
+ private StringWriter serr;
+ private int exitCode = Integer.MIN_VALUE;
+
+ private void setUp(Class<?> cmdClass) {
+ cmd = new CommandLine(cmdClass, new MicronautFactory(context));
+ sout = new StringWriter();
+ serr = new StringWriter();
+ cmd.setOut(new PrintWriter(sout));
+ cmd.setErr(new PrintWriter(serr));
+ }
+
+ private void execute(Class<?> cmdClass, String urlOptionName, String urlOptionValue, List<String> additionalOptions) {
+ setUp(cmdClass);
+ List<String> options = new ArrayList<>();
+ options.add("--" + urlOptionName + "=" + urlOptionValue);
+ options.addAll(additionalOptions);
+ exitCode = cmd.execute(options.toArray(new String[0]));
+ }
+
+ static List<Arguments> cmdClassAndOptionsProvider() {
+ return List.of(
+ Arguments.arguments(NodeConfigShowSubCommand.class, "node-url", List.of()),
Review Comment:
connect, status, cluster init also have --cluster-url or --node-url options. Shall we add tests for them as well?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] valepakh commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
valepakh commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896898847
##########
modules/cli/src/main/java/org/apache/ignite/cli/call/status/StatusReplCall.java:
##########
@@ -40,9 +39,7 @@
public class StatusReplCall implements Call<EmptyCallInput, Status> {
private final NodeManager nodeManager;
-
private final CliPathsConfigLoader cliPathsCfgLdr;
Review Comment:
Code style states that it is optional and I think it's better to use libe breaks between groups of fields, rather than between each field.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] Pochatkin commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
Pochatkin commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r897276506
##########
modules/cli/src/main/java/org/apache/ignite/cli/Main.java:
##########
@@ -54,11 +54,12 @@ public class Main {
public static void main(String[] args) {
initJavaLoggerProps();
+ int exitCode = 0;
Review Comment:
I think this can be a problem. I mean starting with exit code 0. Lets imagine that AnsiConsole.systemInstall(); throw some exeption and in this case we will have failed to start CLI and will have 0(success) exit code.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896775582
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/sql/SqlCommandTest.java:
##########
@@ -0,0 +1,56 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.commands.sql;
+
+import static org.junit.jupiter.api.Assertions.assertAll;
+
+import org.apache.ignite.cli.commands.CliCommandTestBase;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.Test;
+
+class SqlCommandTest extends CliCommandTestBase {
+
+ @Override
+ protected Class<?> getCommandClass() {
+ return SqlCommand.class;
+ }
+
+ @Test
+ @DisplayName("Should throw error if executed without execute or script-file options")
Review Comment:
Should throw error if executed without --execute or --script-file
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r898956788
##########
modules/cli/src/main/java/org/apache/ignite/cli/commands/BaseCommand.java:
##########
@@ -17,24 +17,37 @@
package org.apache.ignite.cli.commands;
-import java.util.concurrent.Callable;
import picocli.CommandLine.Model.CommandSpec;
import picocli.CommandLine.Option;
import picocli.CommandLine.Spec;
/**
* Base class for commands.
*/
-public abstract class BaseCommand implements Callable<Integer> {
+public abstract class BaseCommand {
/** Help option specification. */
@Option(names = {"-h", "--help"}, usageHelp = true, description = "Show this help message and exit.")
protected boolean usageHelpRequested;
@Spec
protected CommandSpec spec;
- @Override
- public Integer call() {
- return 0;
+ /**
+ * Constructs current full command name from the {@code CommandSpec} including all parent commands.
+ *
+ * @return full command name.
+ */
+ protected String getCommandName() {
+ StringBuilder sb = new StringBuilder();
+ CommandSpec root = spec;
+ do {
+ sb.insert(0, root.name());
+ root = root.parent();
+ // Don't add the space after the empty top level repl command's name
Review Comment:
I would suggest to use `trim()` in the end just to make code easier. But it is up to you.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896848783
##########
modules/cli/src/main/java/org/apache/ignite/cli/core/exception/handler/UnknownCommandExceptionHandler.java:
##########
@@ -28,8 +28,9 @@
public class UnknownCommandExceptionHandler implements ExceptionHandler<UnknownCommandException> {
@Override
- public void handle(ExceptionWriter err, UnknownCommandException e) {
+ public int handle(ExceptionWriter err, UnknownCommandException e) {
err.write(e.getMessage());
+ return 1;
Review Comment:
Are we shure that it is not 2?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] valepakh commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
valepakh commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896898847
##########
modules/cli/src/main/java/org/apache/ignite/cli/call/status/StatusReplCall.java:
##########
@@ -40,9 +39,7 @@
public class StatusReplCall implements Call<EmptyCallInput, Status> {
private final NodeManager nodeManager;
-
private final CliPathsConfigLoader cliPathsCfgLdr;
Review Comment:
Code style states that it is optional and I think it's better to use libe breaks between groups of fields, rather than between each field. These line breaks were added after the review which referred to the outdated code style.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896768137
##########
modules/cli/src/integrationTest/java/org/apache/ignite/cli/deprecated/ItConfigCommandTest.java:
##########
@@ -141,10 +141,10 @@ public void setWithWrongData() {
"network.shutdownQuietPeriod=asd"
);
- //assertEquals(1, exitCode); // TODO
+ assertEquals(1, exitCode);
assertThat(
err.toString(UTF_8),
- both(containsString("Command node config update failed with reason: Got error while updating the node configuration."))
+ both(containsString("An error occurred when calling \"update node configuration\""))
Review Comment:
and here
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] PakhomovAlexander commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896844732
##########
modules/cli/src/main/java/org/apache/ignite/cli/core/exception/IgniteCliApiException.java:
##########
@@ -0,0 +1,58 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.core.exception;
+
+/**
+ * Top level runtime exception for throwing the error message from REST API to user.
+ */
+public class IgniteCliApiException extends RuntimeException {
+
+ private final String call;
+ private final String url;
+
+ /**
+ * Creates a new instance of {@code IgniteCliApiException}.
+ *
+ * @param cause the cause.
+ * @param call REST API call.
+ * @param url endpoint URL.
+ */
+ public IgniteCliApiException(Throwable cause, String call, String url) {
+ super(cause);
+ this.call = call;
+ this.url = url;
+ }
+
+ /**
+ * Gets the REST API call.
Review Comment:
Call name or what?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] valepakh commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
valepakh commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896931665
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/CliCommandsErrorTest.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertAll;
+
+import io.micronaut.configuration.picocli.MicronautFactory;
+import io.micronaut.context.ApplicationContext;
+import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
+import jakarta.inject.Inject;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigUpdateSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigUpdateSubCommand;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import picocli.CommandLine;
+
+/**
+ * Base class for testing error handling in CLI commands that use REST.
+ */
+@MicronautTest
+public class CliCommandsErrorTest {
+ private static final String NODE_URL = "http://localhost:10300";
+
+ @Inject
+ private ApplicationContext context;
+
+ private CommandLine cmd;
+ private StringWriter sout;
+ private StringWriter serr;
+ private int exitCode = Integer.MIN_VALUE;
+
+ private void setUp(Class<?> cmdClass) {
+ cmd = new CommandLine(cmdClass, new MicronautFactory(context));
+ sout = new StringWriter();
+ serr = new StringWriter();
+ cmd.setOut(new PrintWriter(sout));
+ cmd.setErr(new PrintWriter(serr));
+ }
+
+ private void execute(Class<?> cmdClass, String urlOptionName, String urlOptionValue, List<String> additionalOptions) {
+ setUp(cmdClass);
+ List<String> options = new ArrayList<>();
+ options.add("--" + urlOptionName + "=" + urlOptionValue);
+ options.addAll(additionalOptions);
+ exitCode = cmd.execute(options.toArray(new String[0]));
+ }
+
+ static List<Arguments> cmdClassAndOptionsProvider() {
+ return List.of(
+ Arguments.arguments(NodeConfigShowSubCommand.class, "node-url", List.of()),
Review Comment:
As to why the local installation is required for the status command - because the REST endpoint is not implemented yet?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] valepakh commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
valepakh commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r896931034
##########
modules/cli/src/test/java/org/apache/ignite/cli/commands/CliCommandsErrorTest.java:
##########
@@ -0,0 +1,152 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF 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.apache.ignite.cli.commands;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.junit.jupiter.api.Assertions.assertAll;
+
+import io.micronaut.configuration.picocli.MicronautFactory;
+import io.micronaut.context.ApplicationContext;
+import io.micronaut.test.extensions.junit5.annotation.MicronautTest;
+import jakarta.inject.Inject;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.util.ArrayList;
+import java.util.List;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.cluster.ClusterConfigUpdateSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigShowSubCommand;
+import org.apache.ignite.cli.commands.configuration.node.NodeConfigUpdateSubCommand;
+import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import picocli.CommandLine;
+
+/**
+ * Base class for testing error handling in CLI commands that use REST.
+ */
+@MicronautTest
+public class CliCommandsErrorTest {
+ private static final String NODE_URL = "http://localhost:10300";
+
+ @Inject
+ private ApplicationContext context;
+
+ private CommandLine cmd;
+ private StringWriter sout;
+ private StringWriter serr;
+ private int exitCode = Integer.MIN_VALUE;
+
+ private void setUp(Class<?> cmdClass) {
+ cmd = new CommandLine(cmdClass, new MicronautFactory(context));
+ sout = new StringWriter();
+ serr = new StringWriter();
+ cmd.setOut(new PrintWriter(sout));
+ cmd.setErr(new PrintWriter(serr));
+ }
+
+ private void execute(Class<?> cmdClass, String urlOptionName, String urlOptionValue, List<String> additionalOptions) {
+ setUp(cmdClass);
+ List<String> options = new ArrayList<>();
+ options.add("--" + urlOptionName + "=" + urlOptionValue);
+ options.addAll(additionalOptions);
+ exitCode = cmd.execute(options.toArray(new String[0]));
+ }
+
+ static List<Arguments> cmdClassAndOptionsProvider() {
+ return List.of(
+ Arguments.arguments(NodeConfigShowSubCommand.class, "node-url", List.of()),
Review Comment:
Ah, sorry, connect command simply doesn't use the --cluster-url option but rather the parameter.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] valepakh commented on a diff in pull request #876: IGNITE-17093 Map error codes for cli commands
Posted by GitBox <gi...@apache.org>.
valepakh commented on code in PR #876:
URL: https://github.com/apache/ignite-3/pull/876#discussion_r897655857
##########
modules/cli/src/main/java/org/apache/ignite/cli/Main.java:
##########
@@ -54,11 +54,12 @@ public class Main {
public static void main(String[] args) {
initJavaLoggerProps();
+ int exitCode = 0;
Review Comment:
Throwing exceptions from main method results in non-zero exit code.
Although I couldn't find where it's specified, the comment in the launcher code indicates that
> The launcher's exit code (in the absence of calls to System.exit) will be non-zero if main threw an exception.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org