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/08/08 15:35:15 UTC
[GitHub] [ignite-3] PakhomovAlexander opened a new pull request, #986: IGNITE-17349 Add common UI components
PakhomovAlexander opened a new pull request, #986:
URL: https://github.com/apache/ignite-3/pull/986
Now everything that we are going to print should go through
UI component. The list of UI components decide how to render
errors, tables and more.
ValidationProblem is removed because it is impossible to
understand from the http status code to what class body
should be deserialized: Problem or ValidationProblem (both
of them have 400 http code). So, now there is only one
Problem that in case of validation issue provides additional
information in `invalidParams` field.
And Problem now does not include the trace id and error code
into the `datail` field because it makes it hard to render
on client side. The detail field should contain useful
explanation of what happened and how to fix this but not
a debug information.
Also, removed all dots from commands output and description.
--
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 #986: IGNITE-17349 Add common UI components
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #986:
URL: https://github.com/apache/ignite-3/pull/986#discussion_r951345776
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/validation/ConfigurationValidationException.java:
##########
@@ -54,4 +55,9 @@ public ConfigurationValidationException(List<ValidationIssue> issues) {
public List<ValidationIssue> getIssues() {
return issues;
}
+
+ private static String createMessageFromIssues(List<ValidationIssue> issues) {
Review Comment:
Correct, the root cause of the problem will be explained in Problem#detail filed.
--
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 #986: IGNITE-17349 Add common UI components
Posted by GitBox <gi...@apache.org>.
valepakh commented on code in PR #986:
URL: https://github.com/apache/ignite-3/pull/986#discussion_r944373042
##########
modules/cli/src/main/java/org/apache/ignite/cli/core/style/component/MessageUiComponent.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.style.component;
+
+import static org.apache.ignite.cli.core.style.AnsiStringSupport.ansi;
+
+import org.apache.ignite.cli.core.style.element.UiElement;
+import org.apache.ignite.cli.core.style.element.UiString;
+
+/**
+ * UI component that represents a message.
+ */
+public class MessageUiComponent implements UiComponent {
+
+ private final String message;
+
+ private final UiElement[] messageUiElements;
+
+ private final String hint;
+
+ private final UiElement[] hintUiElements;
+
+ private MessageUiComponent(
+ String message,
+ UiElement[] messageUiElements,
+ String hint,
+ UiElement[] hintUiElements) {
+ this.message = message;
+ this.messageUiElements = messageUiElements;
+ this.hint = hint;
+ this.hintUiElements = hintUiElements;
+ }
+
+ @Override
+ public String render() {
+ return ansi(
+ UiString.format(message, messageUiElements)
+ + (UiString.format(hint, hintUiElements) == null ? ""
Review Comment:
Maybe it's worth to extract this to a variable? It's hard to follow the logic.
##########
modules/cli/src/main/java/org/apache/ignite/cli/deprecated/spec/NodeCommandSpec.java:
##########
@@ -60,45 +63,22 @@ public class NodeCommandSpec {
/**
* Starts Ignite node command.
*/
- @Command(name = "start", description = "Starts an Ignite node locally.")
+ @Command(name = "start", description = "Starts an Ignite node locally")
@Singleton
public static class StartNodeCommandSpec extends BaseCommand implements Callable<Integer> {
+ /** Consistent id, which will be used by new node. */
+ @Parameters(paramLabel = "name", description = "Name of the new node")
+ public String nodeName;
/** Loader for Ignite distributive paths. */
@Inject
private CliPathsConfigLoader cliPathsCfgLdr;
-
Review Comment:
Looks like some of the line breaks got removed during the merge and ConfigOptions class is again at the bottom.
--
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 #986: IGNITE-17349 Add common UI components
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #986:
URL: https://github.com/apache/ignite-3/pull/986#discussion_r943617744
##########
modules/cli/src/main/java/org/apache/ignite/cli/commands/cluster/status/ClusterStatusReplSubCommand.java:
##########
@@ -58,7 +60,7 @@ public void run() {
} else if (session.isConnectedToNode()) {
inputUrl = session.nodeUrl();
} else {
- spec.commandLine().getErr().println("You are not connected to node. Run 'connect' command or use '--cluster-url' option.");
+ spec.commandLine().getErr().println(CONNECT_OR_USE_CLUSTER_URL_MESSAGE.render());
Review Comment:
Agreed, but I think this is out of the scope of this ticket.
--
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] sk0x50 commented on a diff in pull request #986: IGNITE-17349 Add common UI components
Posted by GitBox <gi...@apache.org>.
sk0x50 commented on code in PR #986:
URL: https://github.com/apache/ignite-3/pull/986#discussion_r951085088
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/validation/ConfigurationValidationException.java:
##########
@@ -54,4 +55,9 @@ public ConfigurationValidationException(List<ValidationIssue> issues) {
public List<ValidationIssue> getIssues() {
return issues;
}
+
+ private static String createMessageFromIssues(List<ValidationIssue> issues) {
Review Comment:
I don't think that this change is fully correct. It just skips error message from a validation issue.
In case of catching a such exception you need to add additional code to get all information. I mean that the following code will not be enough:
```
try {
....
}
catch (Exception e) {
log.("unexpected exception", e);
```
The code above will print: ```Validation did not pass for keys: Key 1, Key 2``. The problem itself will be lost.
--
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] asfgit closed pull request #986: IGNITE-17349 Add common UI components
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #986: IGNITE-17349 Add common UI components
URL: https://github.com/apache/ignite-3/pull/986
--
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] sk0x50 commented on a diff in pull request #986: IGNITE-17349 Add common UI components
Posted by GitBox <gi...@apache.org>.
sk0x50 commented on code in PR #986:
URL: https://github.com/apache/ignite-3/pull/986#discussion_r951085088
##########
modules/configuration-api/src/main/java/org/apache/ignite/configuration/validation/ConfigurationValidationException.java:
##########
@@ -54,4 +55,9 @@ public ConfigurationValidationException(List<ValidationIssue> issues) {
public List<ValidationIssue> getIssues() {
return issues;
}
+
+ private static String createMessageFromIssues(List<ValidationIssue> issues) {
Review Comment:
I don't think that this change is fully correct. It just skips error message from a validation issue.
In case of catching a such exception you need to add additional code to get all information. I mean that the following code will not be enough:
```
try {
....
throw new ConfigurationValidationException(issues);
...
}
catch (Exception e) {
log.("unexpected exception", e);
}
```
The code above will print: ```Validation did not pass for keys: Key 1, Key 2``. The problem itself will be lost.
--
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 pull request #986: IGNITE-17349 Add common UI components
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on PR #986:
URL: https://github.com/apache/ignite-3/pull/986#issuecomment-1208721768
https://issues.apache.org/jira/browse/IGNITE-17349
--
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 #986: IGNITE-17349 Add common UI components
Posted by GitBox <gi...@apache.org>.
PakhomovAlexander commented on code in PR #986:
URL: https://github.com/apache/ignite-3/pull/986#discussion_r943630981
##########
modules/cli/src/main/java/org/apache/ignite/cli/deprecated/ui/ProgressBar.java:
##########
@@ -107,7 +107,7 @@ private String render() {
var completedPart = ((double) curr / (double) max);
- // Space reserved for '||Done!'
+ // Space reserved for '||Done'
var reservedSpace = 7;
Review Comment:
I set it to 6 first, and it broke ProgressBarTest. This number makes no differense on what the user sees.
--
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 #986: IGNITE-17349 Add common UI components
Posted by GitBox <gi...@apache.org>.
valepakh commented on code in PR #986:
URL: https://github.com/apache/ignite-3/pull/986#discussion_r942213534
##########
modules/cli/src/main/java/org/apache/ignite/cli/commands/sql/SqlCommand.java:
##########
@@ -50,19 +50,11 @@ public class SqlCommand extends BaseCommand implements Callable<Integer> {
@ArgGroup(multiplicity = "1")
private ExecOptions execOptions;
- private static class ExecOptions {
Review Comment:
Probably it's best to keep this class here so all the options are in the same place.
##########
modules/cli/src/main/java/org/apache/ignite/cli/commands/cluster/status/ClusterStatusReplSubCommand.java:
##########
@@ -58,7 +60,7 @@ public void run() {
} else if (session.isConnectedToNode()) {
inputUrl = session.nodeUrl();
} else {
- spec.commandLine().getErr().println("You are not connected to node. Run 'connect' command or use '--cluster-url' option.");
+ spec.commandLine().getErr().println(CONNECT_OR_USE_CLUSTER_URL_MESSAGE.render());
Review Comment:
`ConnectToClusterQuestion.askQuestionIfNotConnected` should be used in this and other new REPL commands.
##########
modules/cli/src/main/java/org/apache/ignite/cli/deprecated/ui/ProgressBar.java:
##########
@@ -107,7 +107,7 @@ private String render() {
var completedPart = ((double) curr / (double) max);
- // Space reserved for '||Done!'
+ // Space reserved for '||Done'
var reservedSpace = 7;
Review Comment:
Should the resevred space be 6 now that the exclamation mark is removed?
##########
modules/cli/src/main/java/org/apache/ignite/cli/core/exception/ExceptionHandler.java:
##########
@@ -32,7 +33,14 @@
@Override
public int handle(ExceptionWriter err, Throwable e) {
LOG.error("Unhandled exception", e);
- err.write("Internal error!");
+ err.write(
+ ErrorUiComponent.builder()
+ .header("Unknown error")
+ .details("Please, take a look at node log")
Review Comment:
Maybe it's worth writing exception's message in the details?
##########
modules/cli/src/main/java/org/apache/ignite/cli/core/style/component/QuestionUiComponent.java:
##########
@@ -0,0 +1,72 @@
+/*
+ * 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.style.component;
+
+import static org.apache.ignite.cli.core.style.AnsiStringSupport.ansi;
+
+import org.apache.ignite.cli.core.style.element.UiElement;
+import org.apache.ignite.cli.core.style.element.UiString;
+
+/**
+ * UI component that represent a question.
Review Comment:
```suggestion
* UI component that represents a question.
```
##########
modules/cli/src/main/java/org/apache/ignite/cli/core/exception/handler/TimeoutExceptionHandler.java:
##########
@@ -32,7 +33,14 @@ public class TimeoutExceptionHandler implements ExceptionHandler<TimeoutExceptio
@Override
public int handle(ExceptionWriter err, TimeoutException e) {
LOG.error("Timeout exception", e);
- err.write("Command failed with timeout.");
+ err.write(
+ ErrorUiComponent.builder()
+ .header("The command is running too long")
Review Comment:
```suggestion
.header("The command is running for too long")
```
##########
modules/cli/src/integrationTest/java/org/apache/ignite/cli/commands/connect/ItConnectCommandTest.java:
##########
@@ -78,7 +78,8 @@ void connectWithWrongUrl() {
// Then
assertAll(
- () -> assertErrOutputIs("Could not connect to URL [url=http://localhost:11111]" + System.lineSeparator())
+ () -> assertErrOutputIs("Node unavailable\n"
Review Comment:
```suggestion
() -> assertErrOutputIs("Node unavailable" + System.lineSeparator()
```
##########
modules/cli/src/main/java/org/apache/ignite/cli/core/style/component/MessageUiComponent.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.style.component;
+
+import static org.apache.ignite.cli.core.style.AnsiStringSupport.ansi;
+
+import org.apache.ignite.cli.core.style.element.UiElement;
+import org.apache.ignite.cli.core.style.element.UiString;
+
+/**
+ * UI component that represent a message.
Review Comment:
```suggestion
* UI component that represents a message.
```
--
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