[tarantool-patches] Re: [PATCH v1 1/1] Tarantoolctl "enter" with set language

  • From: Imeev Mergen <imeevma@xxxxxxxxxxxxx>
  • To: Vladislav Shpilevoy <v.shpilevoy@xxxxxxxxxxxxx>, tarantool-patches@xxxxxxxxxxxxx
  • Date: Thu, 26 Jul 2018 14:41:53 +0300



On 07/26/2018 12:56 AM, Vladislav Shpilevoy wrote:



On 25/07/2018 10:51, imeevma@xxxxxxxxxxxxx wrote:
This patch allow to use option "--language" with
"tarantoolctl enter" command. Also default value
for language were added in default configuration
file. Language is either SQL or Lua.

Closes #2385.
---
Branch: https://github.com/tarantool/tarantool/tree/imeevma/gh-2385-select-input-language-tarantoolctl
Issue: https://github.com/tarantool/tarantool/issues/2385

diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
index 2dd5d74..55711b8 100755
--- a/extra/dist/tarantoolctl.in
+++ b/extra/dist/tarantoolctl.in
@@ -155,6 +155,11 @@ local function load_default_file(default_file)
      d.vinyl_dir = fio.pathjoin(d.vinyl_dir, instance_name)
      d.log       = fio.pathjoin(d.log,       instance_name .. '.log')
  +    d.language  = (d.language or "lua"):lower()
+    if d.language ~= "lua" and d.language ~= "sql" then
+        d.language = "lua"

1. Please, do not ignore unknown language. Log this error like others
in the same function already do.
Done.

2. Why do you need language in default_cfg? It is box.cfg as I
understand and box.cfg has no language option. It is not? If you
need to parse a language from the file, then you can store it in
another table or even in a separate variable like others on lines
30-49.
Done.

+    end
+
      default_cfg = d
        if not usermode the> @@ -629,6 +635,17 @@ local function logrotate()
  end
    local function enter()
+    local options = keyword_arguments
+    local language = options.language
+    if language ~= nil then
+        language = language:lower()
+        if language ~= "lua" and language ~= "sql" then
+            log.warn("Language \"%s\" is not recognized. Default language" ..
+                     " \"%s\" is set.", language, default_cfg.language)
+            language = nil

3. For enter() function it is ok to fail on unknown language.
You should not ignore the error because a caller could use
something like this:

    cat my_script.sql | tarantoolctl enter --language sqlll

And if you ignore invalid language, this command will feed the
entire script on SQL into Lua console. It will produce huge
number of syntax errors if no worse.
Done.

+        end
+    end
+    language = language or default_cfg.language
      local console_sock_path = uri.parse(console_sock).service
      if fio.stat(console_sock_path) == nil then
          log.error("Can't connect to %s (%s)", console_sock_path, errno.strerror())
@@ -644,7 +661,9 @@ local function enter()
          console_sock, TIMEOUT_INFINITY
      )
  -    console.on_start(function(self) self:eval(cmd) end)
+    local cmd_language = string.format("\\set language %s", language)

4. Why not to merge cmd_language into cmd? It is created a line above.
Discussed, partly changed.

+
+    console.on_start(function(self) self:eval(cmd) self:eval(cmd_language) end)
      console.on_client_disconnect(function(self) self.running = false end)
      console.start()
      return 0
@@ -1284,6 +1303,7 @@ do
          { 'chdir',       'string'  },
          { 'only-server', 'string'  },
          { 'server',      'string'  },
+        { 'language',    'string'  },
      })
        local cmd_name


5. Lets add the same option to 'eval', 'connect' to make the commands
symmetric.
Discussed, decided to reject the idea.

6. Please, update the comments. For example, on line 1028 I
still see the comment about 'enter': "Enter an instance's interactive Lua console",
but actually it is not complete now - you can choose SQL.

Done.

commit 7088606bba3810ffc6db313cf31d7aa101d7175d
Author: Mergen Imeev <imeevma@xxxxxxxxx>
Date:   Fri Jul 13 19:25:32 2018 +0300

    Tarantoolctl "enter" with set language

    This patch allow to use option "--language" with
    "tarantoolctl enter" command. Also default value
    for language were added in default configuration
    file. Language is either SQL or Lua.

    Closes #2385.

diff --git a/extra/dist/CMakeLists.txt b/extra/dist/CMakeLists.txt
index 862689e..3045b5a 100644
--- a/extra/dist/CMakeLists.txt
+++ b/extra/dist/CMakeLists.txt
@@ -34,6 +34,7 @@ message (STATUS "tarantoolctl logdir: ${TARANTOOL_LOGDIR}")
 set(TARANTOOL_RUNDIR "${CMAKE_INSTALL_FULL_LOCALSTATEDIR}/run/tarantool")
 message (STATUS "tarantoolctl rundir: ${TARANTOOL_RUNDIR}")
 set(TARANTOOL_USER "tarantool")
+set(TARANTOOL_LANGUAGE "lua")
 set(SYSCONFIG_AVAILABLEDIR "tarantool/instances.available")
 set(SYSCONFIG_ENABLEDDIR "tarantool/instances.enabled")
 set(TARANTOOL_AVAILABLEDIR "${CMAKE_INSTALL_FULL_SYSCONFDIR}/${SYSCONFIG_AVAILABLEDIR}")
diff --git a/extra/dist/default/tarantool.in b/extra/dist/default/tarantool.in
index 9a8dad3..01e7144 100644
--- a/extra/dist/default/tarantool.in
+++ b/extra/dist/default/tarantool.in
@@ -20,6 +20,7 @@ default_cfg = {
     vinyl_dir  = "@TARANTOOL_DATADIR@", -- @TARANTOOL_DATADIR@/${INSTANCE}
     log        = "@TARANTOOL_LOGDIR@", -- @TARANTOOL_LOGDIR@/${INSTANCE}.log
     username   = "@TARANTOOL_USER@",
+    language   = "@TARANTOOL_LANGUAGE@",
 }

 -- instances.available - all available instances
diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
index 2dd5d74..ef9da2f 100755
--- a/extra/dist/tarantoolctl.in
+++ b/extra/dist/tarantoolctl.in
@@ -47,6 +47,7 @@ local default_cfg
 local positional_arguments
 local keyword_arguments
 local lua_arguments = arg
+local language

 -- function for printing usage reference
 local usage
@@ -155,6 +156,14 @@ local function load_default_file(default_file)
     d.vinyl_dir = fio.pathjoin(d.vinyl_dir, instance_name)
     d.log       = fio.pathjoin(d.log,       instance_name .. '.log')

+    language = (d.language or "lua"):lower()
+    d.language  = nil
+
+    if language ~= "lua" and language ~= "sql" then
+        log.error('Unknown language: %s', language)
+        os.exit(1)
+    end
+
     default_cfg = d

     if not usermode then
@@ -629,6 +638,12 @@ local function logrotate()
 end

 local function enter()
+    local options = keyword_arguments
+    language = (options.language or language):lower()
+    if language ~= "lua" and language ~= "sql" then
+        log.error('Unknown language: %s', options.language)
+        return 1
+    end
     local console_sock_path = uri.parse(console_sock).service
     if fio.stat(console_sock_path) == nil then
         log.error("Can't connect to %s (%s)", console_sock_path, errno.strerror())
@@ -644,7 +659,9 @@ local function enter()
         console_sock, TIMEOUT_INFINITY
     )

-    console.on_start(function(self) self:eval(cmd) end)
+    local cmd_connect = string.format("\\set language %s", language)
+
+    console.on_start(function(self) self:eval(cmd) self:eval(cmd_connect) end)
     console.on_client_disconnect(function(self) self.running = false end)
     console.start()
     return 0
@@ -1008,11 +1025,15 @@ local commands = setmetatable({
         }
     }, enter = {
         func = exit_wrapper(enter), process = process_local, help = {
-            header = "%s enter INSTANCE",
+            header = "%s enter INSTANCE [--language=language]",
             linkmode = "%s enter",
             description =
 [=[
-        Enter an instance's interactive Lua console.
+        Enter an instance's interactive Lua or SQL console.
+
+        Supported options:
+        * --language=language to set interactive console language.
+          May be either Lua or SQL.
 ]=],
             weight = 65,
             deprecated = false,
@@ -1284,6 +1305,7 @@ do
         { 'chdir',       'string'  },
         { 'only-server', 'string'  },
         { 'server',      'string'  },
+        { 'language',    'string'  },
     })

     local cmd_name

Other related posts: