[GitHub] [ignite-3] sk0x50 commented on a diff in pull request #961: IGNITE-17347 Add port parameters to Ignite3 CLI node start command

[GitBox <gi...@apache.org>]

sk0x50 commented on code in PR #961:
URL: https://github.com/apache/ignite-3/pull/961#discussion_r942653841


##########
modules/cli/src/main/java/org/apache/ignite/cli/deprecated/builtins/node/NodeManager.java:
##########
@@ -136,6 +137,9 @@ public RunningNode start(
             if (srvCfg != null) {
                 cmdArgs.add("--config");
                 cmdArgs.add(srvCfg.toAbsolutePath().toString());
+            } else if (srvCfgStr != null) {
+                cmdArgs.add("--configStr");

Review Comment:
   `--configStr` does not look descriptive to be honest :) By the way, do we use camel case for commands? Perhaps it should be named `config-str`, for instance see at `work-dir`.



##########
modules/cli/src/main/java/org/apache/ignite/cli/deprecated/spec/NodeCommandSpec.java:
##########
@@ -102,12 +130,32 @@ public Integer call() {
             out.println(tbl);
             return 0;
         }
+
+        private Path getConfigPath() {
+            return configOptions != null ? configOptions.configPath : null;
+        }
+
+        private String getConfigStr() {

Review Comment:
   Well, looks like these parameters (port, rest-port etc) cannot be provided with a configuration file. Right?
   It seems to me, that ability to start two nodes with the same configuration file and overridden value of `port` would be very convenient. I mean the following:
   `node start nodeName --config \path_to_configuration_file --port 12345`
   
   What do you think?



-- 
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