You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "valepakh (via GitHub)" <gi...@apache.org> on 2023/10/27 13:24:52 UTC
[PR] IGNITE-20705 Switch user context when reconnecting [ignite-3]
valepakh opened a new pull request, #2762:
URL: https://github.com/apache/ignite-3/pull/2762
https://issues.apache.org/jira/browse/IGNITE-20705
Thank you for submitting the pull request.
To streamline the review process of the patch and ensure better code quality
we ask both an author and a reviewer to verify the following:
### The Review Checklist
- [ ] **Formal criteria:** TC status, codestyle, mandatory documentation. Also make sure to complete the following:
\- There is a single JIRA ticket related to the pull request.
\- The web-link to the pull request is attached to the JIRA ticket.
\- The JIRA ticket has the Patch Available state.
\- The description of the JIRA ticket explains WHAT was made, WHY and HOW.
\- The pull request title is treated as the final commit message. The following pattern must be used: IGNITE-XXXX Change summary where XXXX - number of JIRA issue.
- [ ] **Design:** new code conforms with the design principles of the components it is added to.
- [ ] **Patch quality:** patch cannot be split into smaller pieces, its size must be reasonable.
- [ ] **Code quality:** code is clean and readable, necessary developer documentation is added if needed.
- [ ] **Tests code quality:** test set covers positive/negative scenarios, happy/edge cases. Tests are effective in terms of execution time and resources.
### Notes
- [Apache Ignite Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Java+Code+Style+Guide)
--
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
Re: [PR] IGNITE-20705 Switch user context when reconnecting [ignite-3]
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander commented on PR #2762:
URL: https://github.com/apache/ignite-3/pull/2762#issuecomment-1787152768
The output differs for connect without args and with:
```bash
[a2:defaultNode]> connect
You are already connected to http://localhost:10300
[a2:defaultNode]> connect http://localhost:10300 --username a2 --password wrong_password
You are already connected to the http://localhost:10300 as org.apache.ignite.internal.cli.core.style.element.UiElements$$Lambda$697/0x000000013c433d08@108d55c4, do you want to connect as org.apache.ignite.internal.cli.core.style.element.UiElements$$Lambda$697/0x000000013c433d08@5432c277? [Y/n]
You are already connected to http://localhost:10300
[a2:defaultNode]>
--
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
Re: [PR] IGNITE-20705 Switch user context when reconnecting [ignite-3]
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander commented on PR #2762:
URL: https://github.com/apache/ignite-3/pull/2762#issuecomment-1787142183
@valepakh during manual check I've got this:
```bash
You are already connected to the http://localhost:10300 as org.apache.ignite.internal.cli.core.style.element.UiElements$$Lambda$684/0x000000013e43f220@7a55f148, do you want to connect as org.apache.ignite.internal.cli.core.style.element.UiElements$$Lambda$684/0x000000013e43f220@2e2f720? [Y/n]
--
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
Re: [PR] IGNITE-20705 Switch user context when reconnecting [ignite-3]
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander commented on code in PR #2762:
URL: https://github.com/apache/ignite-3/pull/2762#discussion_r1377504034
##########
modules/cli/src/main/java/org/apache/ignite/internal/cli/commands/questions/ConnectToClusterQuestion.java:
##########
@@ -97,22 +98,40 @@ private String sessionNodeUrl() {
}
/**
- * Ask if the user really wants to connect if we are already connected and the URL is different.
+ * Ask if the user really wants to connect if we are already connected and the URL or authentication parameters are different.
*
- * @param nodeUrl Node URL.
- * @return {@link FlowBuilder} instance which provides the node URL if we are not connected or connected to different URL or interrupts
- * if user's answer is negative.
+ * @param input Node URL and authentication parameters.
+ * @return {@link FlowBuilder} instance which provides the connect call input if we are not connected or user agreed to reconnect or
+ * interrupts if user's answer is negative.
*/
- public FlowBuilder<Void, String> askQuestionIfConnected(String nodeUrl) {
+ public FlowBuilder<Void, ConnectCallInput> askQuestionIfConnected(ConnectCallInput input) {
SessionInfo sessionInfo = session.info();
- if (sessionInfo != null && !Objects.equals(sessionInfo.nodeUrl(), nodeUrl)) {
- QuestionUiComponent question = fromYesNoQuestion(
- "You are already connected to the %s, do you want to connect to the %s?",
- UiElements.url(sessionInfo.nodeUrl()), UiElements.url(nodeUrl)
- );
- return Flows.acceptQuestion(question, () -> nodeUrl);
+ if (sessionInfo != null) {
+ if (!Objects.equals(sessionInfo.nodeUrl(), input.url())) {
+ return Flows.acceptQuestion(fromYesNoQuestion(
+ "You are already connected to the %s, do you want to connect to the %s?",
+ UiElements.url(sessionInfo.nodeUrl()), UiElements.url(input.url())
+ ), () -> input);
+ }
+
+ String oldUsername = sessionInfo.username();
+ String newUsername = input.username();
+
+ if (newUsername != null) {
+ if (oldUsername != null) {
+ return Flows.acceptQuestion(fromYesNoQuestion(
+ "You are already connected to the %s as %s, do you want to connect as %s?",
+ UiElements.url(sessionInfo.nodeUrl()), string(oldUsername), string(newUsername)
+ ), () -> input);
+ } else {
+ return Flows.acceptQuestion(fromYesNoQuestion(
+ "You are already connected to the %s, do you want to connect as %s?",
+ UiElements.url(sessionInfo.nodeUrl()), string(newUsername)
Review Comment:
I think it is better to introduce `username` UI element. In the future, we might change the way we display usernames.
##########
modules/cli/src/test/java/org/apache/ignite/internal/cli/core/repl/completer/DynamicCompleterRegistryTest.java:
##########
@@ -324,20 +324,20 @@ void positionalParameter() {
// Given completer for single positional parameter
registry.register(
CompleterConf.builder()
- .command("connect") // should be real commands
+ .command("cluster", "config", "show") // should be real commands
.singlePositionalParameter()
.build(),
words -> completer1
);
// When there is no positional argument typed
- List<DynamicCompleter> connectCompleter = registry.findCompleters(words("connect"));
+ List<DynamicCompleter> connectCompleter = registry.findCompleters(words("cluster", "config", "show"));
Review Comment:
configShowCompleter
##########
modules/cli/src/test/java/org/apache/ignite/internal/cli/core/repl/completer/DynamicCompletionInsiderTest.java:
##########
@@ -31,12 +31,6 @@ class DynamicCompletionInsiderTest {
private static Stream<Arguments> typedWordsSource() {
return Stream.of(
- // connect command needs a parameter
- Arguments.of(new String[]{"connect"}, false),
Review Comment:
I think these test cases should not be removed but replaced with another 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
Re: [PR] IGNITE-20705 Switch user context when reconnecting [ignite-3]
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander commented on PR #2762:
URL: https://github.com/apache/ignite-3/pull/2762#issuecomment-1787157988
Playing around and getting some inconsistency in cli configuration:
```bash
[a1:defaultNode]> cli config show
ignite.auth.basic.username=a1
ignite.auth.basic.password=a1
[a1:defaultNode]>
[a1:defaultNode]>
[a1:defaultNode]> connect
You are already connected to http://localhost:10300
[a1:defaultNode]> connect http://localhost:10300 --username a2 --password wrong_password
You are already connected to the http://localhost:10300 as org.apache.ignite.internal.cli.core.style.element.UiElements$$Lambda$705/0x0000000139439e80@2ca5f1ed, do you want to connect as org.apache.ignite.internal.cli.core.style.element.UiElements$$Lambda$705/0x0000000139439e80@6c03fb16? [Y/n]
Authentication error occurred while connecting to the node, it could be due to the wrong basic auth configuration. Do you want to configure them now? [Y/n]
Enter username: a2
Enter user password: ** <-- here is the correct password typed
Remember current credentials? [Y/n]
Config saved
Connected to http://localhost:10300
[a2:defaultNode]> cli config show
ignite.auth.basic.username=a2
ignite.auth.basic.password=wrong_password
[a2:defaultNode]>
--
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
Re: [PR] IGNITE-20705 Switch user context when reconnecting [ignite-3]
Posted by "valepakh (via GitHub)" <gi...@apache.org>.
valepakh commented on code in PR #2762:
URL: https://github.com/apache/ignite-3/pull/2762#discussion_r1377569013
##########
modules/cli/src/test/java/org/apache/ignite/internal/cli/core/repl/completer/DynamicCompletionInsiderTest.java:
##########
@@ -31,12 +31,6 @@ class DynamicCompletionInsiderTest {
private static Stream<Arguments> typedWordsSource() {
return Stream.of(
- // connect command needs a parameter
- Arguments.of(new String[]{"connect"}, false),
Review Comment:
> I think these test cases should not be removed but replaced with another command.
I don't think we have another command with required completable 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
Re: [PR] IGNITE-20705 Switch user context when reconnecting [ignite-3]
Posted by "PakhomovAlexander (via GitHub)" <gi...@apache.org>.
PakhomovAlexander merged PR #2762:
URL: https://github.com/apache/ignite-3/pull/2762
--
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