Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH] box: set cfg via environment variables
@ 2021-03-14 15:34 Roman Khabibov via Tarantool-patches
  2021-03-19 12:38 ` Leonid Vasiliev via Tarantool-patches
  2021-03-19 13:23 ` Leonid Vasiliev via Tarantool-patches
  0 siblings, 2 replies; 7+ messages in thread
From: Roman Khabibov via Tarantool-patches @ 2021-03-14 15:34 UTC (permalink / raw)
  To: tarantool-patches

Add ability to set box.cfg options via env variables. These
variables should have name "TT_<OPTION>". When Tarantool instance
is started under tarantoolctl utility env variables options have
more priority than instance file's cfg.

Closes #5602

@TarantoolBot document
Title: Set box.cfg options via env variables

Now, it is possible to set box.cfg options via environment
variables. The name of variable should correspond the followng
pattern: "TT_<NAME>", where <NAME> is an option name. For example:
"TT_LISTEN", "TT_READAHEAD".
If it is complex value, for example, "replication" can be set by a
table, the corresponding environment varibale value has the syntax
of lua array (without keys):

`export TT_REPLICATION="{'uri1', 'uri2'}"`

Note that every value in arary must be quoted.
---

@ChangeLog
* Add ability to set box.cfg options via environment variables (gh-5602).

Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-5602-environment-variables
Issue: https://github.com/tarantool/tarantool/issues/5602

 src/box/lua/load_cfg.lua                      | 74 +++++++++++++++++++
 test/app-tap/tarantoolctl.test.lua            | 33 ++++++++-
 .../gh-5602-environment-vars-cfg.result       | 42 +++++++++++
 .../gh-5602-environment-vars-cfg.test.lua     | 53 +++++++++++++
 test/box-tap/gh-5602_script.lua               | 70 ++++++++++++++++++
 5 files changed, 271 insertions(+), 1 deletion(-)
 create mode 100644 test/box-tap/gh-5602-environment-vars-cfg.result
 create mode 100755 test/box-tap/gh-5602-environment-vars-cfg.test.lua
 create mode 100755 test/box-tap/gh-5602_script.lua

diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index 574c8bef4..eace25d46 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -616,6 +616,79 @@ setmetatable(box, {
      end
 })
 
+local function process_option_value(option, raw_value)
+    local param_type = template_cfg[option]
+    if param_type == nil then
+        return nil
+    end
+    local value = nil
+    if param_type == 'number' or option == 'log_level' then
+        value = tonumber(raw_value)
+        if value == nil then
+            error("Environment variable "..'TT_'..string.upper(option).." has"..
+                  " incorrect value for option '"..option.."': should be "..
+                  "converted to number")
+        end
+    elseif param_type == 'boolean' or option == 'log_nonblock' then
+        if raw_value == 'true' then
+            value = true
+        elseif raw_value == 'false' then
+            value = false
+        else
+            error("Environment variable "..'TT_'..string.upper(option).." has"..
+                  " incorrect value for option '"..option.."': should be "..
+                  "'true' or 'false'")
+        end
+    elseif param_type == 'string, number' then
+        value = tonumber(raw_value)
+        if value == nil then
+            value = raw_value
+        end
+    elseif param_type == 'string, number, table' then
+        if string.sub(raw_value, 1, 1) == '{' and
+           string.sub(raw_value, -1) == '}' then
+            value = string.sub(raw_value, 2, -2)
+            value = require('csv').load(value)[1]
+            for i, str in pairs(value) do
+                if not (string.sub(str, 1, 1) == '\'' and
+                        string.sub(str, -1) == '\'') and
+                   not (string.sub(str, 1, 1) == '"' and
+                        string.sub(str, -1) == '"') then
+                    error("Environment variable "..'TT_'..string.upper(option)..
+                          " has incorrect value for option '"..option.."': "..
+                          "elements of array should be quoted strings")
+                end
+                str = string.sub(str, 2, -2)
+                value[i] = str
+            end
+        else
+            value = tonumber(raw_value)
+            if value == nil then
+                value = raw_value
+            end
+        end
+    else
+        value = raw_value
+    end
+
+    return value
+end
+
+local function collect_env_cfg(cfg)
+    if cfg == nil then
+        cfg = {}
+    end
+    for option, _ in pairs(template_cfg) do
+        if cfg[option] == nil or os.getenv('TARANTOOLCTL') == 'true' then
+            local env_var_name = 'TT_'..string.upper(option)
+            local raw_value = os.getenv(env_var_name)
+            if raw_value ~= nil then
+                cfg[option] = process_option_value(option, raw_value)
+            end
+        end
+    end
+end
+
 -- Whether box is loaded.
 --
 -- `false` when box is not configured or when the initialization
@@ -627,6 +700,7 @@ setmetatable(box, {
 local box_is_configured = false
 
 local function load_cfg(cfg)
+    collect_env_cfg(cfg)
     -- A user may save box.cfg (this function) before box loading
     -- and call it afterwards. We should reconfigure box in the
     -- case.
diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
index 3c3023d34..1f15fe103 100755
--- a/test/app-tap/tarantoolctl.test.lua
+++ b/test/app-tap/tarantoolctl.test.lua
@@ -160,7 +160,7 @@ local function merge(...)
 end
 
 local test = tap.test('tarantoolctl')
-test:plan(8)
+test:plan(9)
 
 -- basic start/stop test
 -- must be stopped afterwards
@@ -632,4 +632,35 @@ test:test('filter_xlog', function(test)
     end
 end)
 
+-- gh-5602: Check that environment cfg variables are more
+-- prioritized than tarantoolctl cfg.
+do
+    local dir = fio.tempdir()
+    local code = [[ box.cfg ]]
+    create_script(dir, 'show_cfg.lua',  code)
+    local code = [[ box.cfg{listen=3302, readahead=10001} ]]
+    create_script(dir, 'cfg_script.lua', code)
+
+    local status, err = pcall(function()
+        test:test("check answers in case of call", function(test_i)
+            test_i:plan(4)
+            os.setenv('TT_LISTEN', '3301')
+            os.setenv('TT_READAHEAD', '10000')
+            check_ok(test_i, dir, 'start', 'cfg_script', 0)
+            tctl_wait_start(dir, 'cfg_script')
+            check_ok(test_i, dir, 'eval',  'cfg_script show_cfg.lua', 0,
+                     '---\n- 3301\n- 10000\n...', nil)
+            check_ok(test_i, dir, 'stop', 'cfg_script', 0)
+        end)
+    end)
+
+    cleanup_instance(dir, 'cfg_script')
+    recursive_rmdir(dir)
+
+    if status == false then
+        print(("Error: %s"):format(err))
+        os.exit()
+    end
+end
+
 os.exit(test:check() == true and 0 or -1)
diff --git a/test/box-tap/gh-5602-environment-vars-cfg.result b/test/box-tap/gh-5602-environment-vars-cfg.result
new file mode 100644
index 000000000..495e02a73
--- /dev/null
+++ b/test/box-tap/gh-5602-environment-vars-cfg.result
@@ -0,0 +1,42 @@
+TAP version 13
+1..5
+ok - box.cfg is successful
+ok - listen
+ok - readahead
+ok - strip_core
+ok - log_format is not set
+TAP version 13
+1..7
+ok - box.cfg is successful
+ok - listen
+ok - replication is table
+ok - replication URI 1
+ok - replication URI 2
+ok - replication_connect_timeout
+ok - replication_synchro_quorum
+TAP version 13
+1..3
+ok - box.cfg is successful
+ok - box.cfg{} background value is prioritized
+ok - box.cfg{} vinyl_timeout value is prioritized
+TAP version 13
+1..2
+ok - box.cfg is not successful
+ok - bad sql_cache_size value
+TAP version 13
+1..2
+ok - box.cfg is not successful
+builtin/box/load_cfg.lua:19: builtin/box/load_cfg.lua:638: Environment variable TT_STRIP_CORE has incorrect value for option 'strip_core': should be 'true' or 'false'
+ok - bad strip_core value
+TAP version 13
+1..2
+ok - box.cfg is not successful
+builtin/box/load_cfg.lua:19: builtin/box/load_cfg.lua:657: Environment variable TT_REPLICATION has incorrect value for option 'replication': elements of array should be quoted strings
+ok - bad replication value
+TAP version 13
+ok - set 1
+ok - set 2
+ok - set 3
+ok - set 4
+ok - set 5
+ok - set 6
diff --git a/test/box-tap/gh-5602-environment-vars-cfg.test.lua b/test/box-tap/gh-5602-environment-vars-cfg.test.lua
new file mode 100755
index 000000000..84b49b7eb
--- /dev/null
+++ b/test/box-tap/gh-5602-environment-vars-cfg.test.lua
@@ -0,0 +1,53 @@
+#!/usr/bin/env tarantool
+
+local os = require('os')
+local fio = require('fio')
+local tap = require('tap')
+local test = tap.test('gh-5602')
+
+-- gh-5602: Check that environment cfg variables working.
+local TARANTOOL_PATH = arg[-1]
+local script_name = 'gh-5602_script.lua'
+local path_to_script = fio.pathjoin(
+        os.getenv('PWD'),
+        'box-tap',
+        script_name)
+local function shell_command(set, i)
+    string.gsub(set, 'TT_', '')
+    return ('%s %s %s %d'):format(
+        set,
+        TARANTOOL_PATH,
+        path_to_script,
+        i)
+end
+
+local set_1 = 'TT_LISTEN=3301 ' ..
+              'TT_READAHEAD=10000 ' ..
+              'TT_STRIP_CORE=false ' ..
+              'TT_LOG_FORMAT=json'
+local set_2 = 'TT_LISTEN=3301 ' ..
+              'TT_REPLICATION=\"{\'0.0.0.0:12345\', ' ..
+                              '\'1.1.1.1:12345\'}\" ' ..
+              'TT_REPLICATION_CONNECT_TIMEOUT=0.01 ' ..
+              'TT_REPLICATION_SYNCHRO_QUORUM=\'4 + 1\''
+local set_3 = 'TT_BACKGROUND=true ' ..
+              'TT_VINYL_TIMEOUT=60.1'
+local set_4 = 'TT_SQL_CACHE_SIZE=a'
+local set_5 = 'TT_STRIP_CORE=a'
+local set_6 = 'TT_REPLICATION=\"{0.0.0.0:12345\', ' ..
+                              '\'1.1.1.1:12345\'}\"'
+local sets = {}
+sets[1] = set_1
+sets[2] = set_2
+sets[3] = set_3
+sets[4] = set_4
+sets[5] = set_5
+sets[6] = set_6
+
+for i, set in ipairs(sets) do
+    local tmpdir = fio.tempdir()
+    local new_path = fio.pathjoin(tmpdir, script_name)
+    fio.copyfile(path_to_script, new_path)
+    test:is(os.execute(shell_command(set, i)), 0, 'set '..i)
+end
+os.exit(0)
diff --git a/test/box-tap/gh-5602_script.lua b/test/box-tap/gh-5602_script.lua
new file mode 100755
index 000000000..8ff34deb2
--- /dev/null
+++ b/test/box-tap/gh-5602_script.lua
@@ -0,0 +1,70 @@
+local tap = require('tap')
+local test = tap.test('gh-5602')
+
+local status, err = pcall(box.cfg, {background = false, vinyl_timeout = 70.1})
+
+-- Check that environment cfg values are set correctly.
+if arg[1] == '1' then
+    test:plan(5)
+    test:ok(status ~= false, 'box.cfg is successful')
+    test:is(box.cfg['listen'], '3301', 'listen')
+    test:is(box.cfg['readahead'], 10000, 'readahead')
+    test:is(box.cfg['strip_core'], false, 'strip_core')
+    test:is(box.cfg['log_format'], 'json', 'log_format is not set')
+end
+if arg[1] == '2' then
+    test:plan(7)
+    test:ok(status ~= false, 'box.cfg is successful')
+    test:is(box.cfg['listen'], '3301', 'listen')
+    local replication = box.cfg['replication']
+    test:is(type(replication), 'table', 'replication is table')
+    test:ok(replication[1] == '0.0.0.0:12345' or
+            replication[1] == '1.1.1.1:12345', 'replication URI 1')
+    test:ok(replication[2] == '0.0.0.0:12345' or
+            replication[2] == '1.1.1.1:12345', 'replication URI 2')
+    test:is(box.cfg['replication_connect_timeout'], 0.01,
+            'replication_connect_timeout')
+    test:is(box.cfg['replication_synchro_quorum'], '4 + 1',
+            'replication_synchro_quorum')
+end
+
+-- Check that box.cfg{} values are more prioritized than
+-- environment cfg values.
+if arg[1] == '3' then
+    test:plan(3)
+    test:ok(status ~= false, 'box.cfg is successful')
+    test:is(box.cfg['background'], false,
+            'box.cfg{} background value is prioritized')
+    test:is(box.cfg['vinyl_timeout'], 70.1,
+            'box.cfg{} vinyl_timeout value is prioritized')
+end
+
+-- Check bad environment cfg values.
+if arg[1] == '4' then
+    test:plan(2)
+    test:ok(status == false, 'box.cfg is not successful')
+    local index = string.find(err, 'Environment variable TT_SQL_CACHE_SIZE '..
+        'has incorrect value for option \'sql_cache_size\': should be '..
+        'converted to number')
+    test:ok(index ~= nil, 'bad sql_cache_size value')
+end
+if arg[1] == '5' then
+    test:plan(2)
+    test:ok(status == false, 'box.cfg is not successful')
+    print(err)
+    local index = string.find(err, 'Environment variable TT_STRIP_CORE has '..
+        'incorrect value for option \'strip_core\': should be \'true\' '..
+        'or \'false\'')
+    test:ok(index ~= nil, 'bad strip_core value')
+end
+if arg[1] == '6' then
+    test:plan(2)
+    test:ok(status == false, 'box.cfg is not successful')
+    print(err)
+    local index = string.find(err, 'Environment variable TT_REPLICATION has '..
+        'incorrect value for option \'replication\': elements of array should'..
+        ' be quoted strings')
+    test:ok(index ~= nil, 'bad replication value')
+end
+
+os.exit(test:check() and 0 or 1)
\ No newline at end of file
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH] box: set cfg via environment variables
  2021-03-14 15:34 [Tarantool-patches] [PATCH] box: set cfg via environment variables Roman Khabibov via Tarantool-patches
@ 2021-03-19 12:38 ` Leonid Vasiliev via Tarantool-patches
  2021-04-09  8:56   ` Roman Khabibov via Tarantool-patches
  2021-03-19 13:23 ` Leonid Vasiliev via Tarantool-patches
  1 sibling, 1 reply; 7+ messages in thread
From: Leonid Vasiliev via Tarantool-patches @ 2021-03-19 12:38 UTC (permalink / raw)
  To: Roman Khabibov, tarantool-patches

Hi! Thank you for the patch.
See comments bellow:

On 3/14/21 6:34 PM, Roman Khabibov wrote:
> Add ability to set box.cfg options via env variables. These
> variables should have name "TT_<OPTION>". When Tarantool instance
> is started under tarantoolctl utility env variables options have
> more priority than instance file's cfg.
> 
> Closes #5602
> 
> @TarantoolBot document
> Title: Set box.cfg options via env variables
> 
> Now, it is possible to set box.cfg options via environment
> variables. The name of variable should correspond the followng

1) followng -> following

> pattern: "TT_<NAME>", where <NAME> is an option name. For example:
> "TT_LISTEN", "TT_READAHEAD".
> If it is complex value, for example, "replication" can be set by a
> table, the corresponding environment varibale value has the syntax

2) varibale -> variable

> of lua array (without keys):
> 
> `export TT_REPLICATION="{'uri1', 'uri2'}"`
> 
> Note that every value in arary must be quoted.

1) arary -> array

> ---
> 
> @ChangeLog
> * Add ability to set box.cfg options via environment variables (gh-5602).
> 
> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-5602-environment-variables
> Issue: https://github.com/tarantool/tarantool/issues/5602
> 
>   src/box/lua/load_cfg.lua                      | 74 +++++++++++++++++++
>   test/app-tap/tarantoolctl.test.lua            | 33 ++++++++-
>   .../gh-5602-environment-vars-cfg.result       | 42 +++++++++++
>   .../gh-5602-environment-vars-cfg.test.lua     | 53 +++++++++++++
>   test/box-tap/gh-5602_script.lua               | 70 ++++++++++++++++++
>   5 files changed, 271 insertions(+), 1 deletion(-)
>   create mode 100644 test/box-tap/gh-5602-environment-vars-cfg.result
>   create mode 100755 test/box-tap/gh-5602-environment-vars-cfg.test.lua
>   create mode 100755 test/box-tap/gh-5602_script.lua
> 
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 574c8bef4..eace25d46 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -616,6 +616,79 @@ setmetatable(box, {
>        end
>   })
> 

4) Add descriptions to functions (must start with `---`).

> +local function process_option_value(option, raw_value)
> +    local param_type = template_cfg[option]
> +    if param_type == nil then
> +        return nil
> +    end
> +    local value = nil
> +    if param_type == 'number' or option == 'log_level' then
> +        value = tonumber(raw_value)
> +        if value == nil then
> +            error("Environment variable "..'TT_'..string.upper(option).." has"..
> +                  " incorrect value for option '"..option.."': should be "..
> +                  "converted to number")

5) Codestyle (apply to the whole patch):
     - Indetation.
         From https://dev.minetest.net/Lua_code_style_guidelines :
         "When strings don't fit into the line, you should add the string
         (changes) to the next line(s) indented by one tab." (4 spaces 
in our case).
     - String format.
         From 
https://www.tarantool.io/en/doc/latest/dev_guide/lua_style_guide/#indentation-and-formatting:
         "Avoid multiple concatenations in one statement, use 
string.format instead"

> +        end
> +    elseif param_type == 'boolean' or option == 'log_nonblock' then
> +        if raw_value == 'true' then
> +            value = true
> +        elseif raw_value == 'false' then
> +            value = false
> +        else
> +            error("Environment variable "..'TT_'..string.upper(option).." has"..
> +                  " incorrect value for option '"..option.."': should be "..
> +                  "'true' or 'false'")
> +        end
> +    elseif param_type == 'string, number' then
> +        value = tonumber(raw_value)
> +        if value == nil then
> +            value = raw_value
> +        end
> +    elseif param_type == 'string, number, table' then

6) OMG! We have to get the table from the env variable. Nothing good to 
expect.
     - What about a nested table?
     - Also, what about the cases "string, table" or "table, string, 
number" ...? Now we don't have such a `param_type`, but they will appear 
tomorrow.

> +        if string.sub(raw_value, 1, 1) == '{' and
> +           string.sub(raw_value, -1) == '}' then
> +            value = string.sub(raw_value, 2, -2)
> +            value = require('csv').load(value)[1]
> +            for i, str in pairs(value) do
> +                if not (string.sub(str, 1, 1) == '\'' and
> +                        string.sub(str, -1) == '\'') and
> +                   not (string.sub(str, 1, 1) == '"' and
> +                        string.sub(str, -1) == '"') then
> +                    error("Environment variable "..'TT_'..string.upper(option)..
> +                          " has incorrect value for option '"..option.."': "..
> +                          "elements of array should be quoted strings")
> +                end
> +                str = string.sub(str, 2, -2)

7) Shouldn't we be checking types inside a table? Or can there only be 
strings?

> +                value[i] = str
> +            end
> +        else
> +            value = tonumber(raw_value)
> +            if value == nil then
> +                value = raw_value
> +            end
> +        end
> +    else
> +        value = raw_value
> +    end
> +
> +    return value
> +end
> +
> +local function collect_env_cfg(cfg)
> +    if cfg == nil then
> +        cfg = {}
> +    end

8) Please, separate logical blocks of code from each other with empty 
lines. Write comments.(apply to the whole patch).

> +    for option, _ in pairs(template_cfg) do
> +        if cfg[option] == nil or os.getenv('TARANTOOLCTL') == 'true' then
> +            local env_var_name = 'TT_'..string.upper(option)
> +            local raw_value = os.getenv(env_var_name)
> +            if raw_value ~= nil then
> +                cfg[option] = process_option_value(option, raw_value)
> +            end
> +        end
> +    end
> +end
> +
>   -- Whether box is loaded.
>   --
>   -- `false` when box is not configured or when the initialization
> @@ -627,6 +700,7 @@ setmetatable(box, {
>   local box_is_configured = false
>   
>   local function load_cfg(cfg)
> +    collect_env_cfg(cfg)
>       -- A user may save box.cfg (this function) before box loading
>       -- and call it afterwards. We should reconfigure box in the
>       -- case.

9) It looks like in this case we can overload the value that was set in
the previous call with the value from the environment. Or not?

> diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
> index 3c3023d34..1f15fe103 100755
> --- a/test/app-tap/tarantoolctl.test.lua
> +++ b/test/app-tap/tarantoolctl.test.lua
> @@ -160,7 +160,7 @@ local function merge(...)
>   end
>   
>   local test = tap.test('tarantoolctl')
> -test:plan(8)
> +test:plan(9)
>   
>   -- basic start/stop test
>   -- must be stopped afterwards
> @@ -632,4 +632,35 @@ test:test('filter_xlog', function(test)
>       end
>   end)
>   
> +-- gh-5602: Check that environment cfg variables are more
> +-- prioritized than tarantoolctl cfg.
> +do

10) As for me, it is preferable to use `test:test()` instead of 
`do...end`. Up to you.

> +    local dir = fio.tempdir()
> +    local code = [[ box.cfg ]]

11) Maybe `[[ box.cfg{} ]]`

> +    create_script(dir, 'show_cfg.lua',  code)

12) Two spaces before `code`, but one is needed.

> +    local code = [[ box.cfg{listen=3302, readahead=10001} ]]
> +    create_script(dir, 'cfg_script.lua', code)
> +
> +    local status, err = pcall(function()
> +        test:test("check answers in case of call", function(test_i)
> +            test_i:plan(4)
> +            os.setenv('TT_LISTEN', '3301')

13) We will not have problems due to a specific port when many tests are
run in parallel (such as this port will be already occupied).

> +            os.setenv('TT_READAHEAD', '10000')
> +            check_ok(test_i, dir, 'start', 'cfg_script', 0)
> +            tctl_wait_start(dir, 'cfg_script')
> +            check_ok(test_i, dir, 'eval',  'cfg_script show_cfg.lua', 0,
> +                     '---\n- 3301\n- 10000\n...', nil)
> +            check_ok(test_i, dir, 'stop', 'cfg_script', 0)
> +        end)
> +    end)
> +
> +    cleanup_instance(dir, 'cfg_script')
> +    recursive_rmdir(dir)
> +
> +    if status == false then
> +        print(("Error: %s"):format(err))
> +        os.exit()
> +    end
> +end
> +
>   os.exit(test:check() == true and 0 or -1)
> diff --git a/test/box-tap/gh-5602-environment-vars-cfg.result b/test/box-tap/gh-5602-environment-vars-cfg.result
> new file mode 100644
> index 000000000..495e02a73
> --- /dev/null
> +++ b/test/box-tap/gh-5602-environment-vars-cfg.result
> @@ -0,0 +1,42 @@
> +TAP version 13
> +1..5
> +ok - box.cfg is successful
> +ok - listen
> +ok - readahead
> +ok - strip_core
> +ok - log_format is not set
> +TAP version 13
> +1..7
> +ok - box.cfg is successful
> +ok - listen
> +ok - replication is table
> +ok - replication URI 1
> +ok - replication URI 2
> +ok - replication_connect_timeout
> +ok - replication_synchro_quorum
> +TAP version 13
> +1..3
> +ok - box.cfg is successful
> +ok - box.cfg{} background value is prioritized
> +ok - box.cfg{} vinyl_timeout value is prioritized
> +TAP version 13
> +1..2
> +ok - box.cfg is not successful
> +ok - bad sql_cache_size value
> +TAP version 13
> +1..2
> +ok - box.cfg is not successful
> +builtin/box/load_cfg.lua:19: builtin/box/load_cfg.lua:638: Environment variable TT_STRIP_CORE has incorrect value for option 'strip_core': should be 'true' or 'false'
> +ok - bad strip_core value
> +TAP version 13
> +1..2
> +ok - box.cfg is not successful
> +builtin/box/load_cfg.lua:19: builtin/box/load_cfg.lua:657: Environment variable TT_REPLICATION has incorrect value for option 'replication': elements of array should be quoted strings
> +ok - bad replication value
> +TAP version 13
> +ok - set 1
> +ok - set 2
> +ok - set 3
> +ok - set 4
> +ok - set 5
> +ok - set 6
> diff --git a/test/box-tap/gh-5602-environment-vars-cfg.test.lua b/test/box-tap/gh-5602-environment-vars-cfg.test.lua
> new file mode 100755
> index 000000000..84b49b7eb
> --- /dev/null
> +++ b/test/box-tap/gh-5602-environment-vars-cfg.test.lua
> @@ -0,0 +1,53 @@
> +#!/usr/bin/env tarantool
> +
> +local os = require('os')
> +local fio = require('fio')
> +local tap = require('tap')

13) It's nice to separate the variable definition and the `requires` 
with empty line.(apply to the whole patch)

> +local test = tap.test('gh-5602')
> +
> +-- gh-5602: Check that environment cfg variables working.
> +local TARANTOOL_PATH = arg[-1]
> +local script_name = 'gh-5602_script.lua'
> +local path_to_script = fio.pathjoin(
> +        os.getenv('PWD'),
> +        'box-tap',
> +        script_name)

14) Please, separate the function definition with an empty line.

15) Describe the test mechanism using a comment.

> +local function shell_command(set, i)
> +    string.gsub(set, 'TT_', '')
> +    return ('%s %s %s %d'):format(
> +        set,
> +        TARANTOOL_PATH,
> +        path_to_script,
> +        i)
> +end
> +
> +local set_1 = 'TT_LISTEN=3301 ' ..
> +              'TT_READAHEAD=10000 ' ..
> +              'TT_STRIP_CORE=false ' ..
> +              'TT_LOG_FORMAT=json'
> +local set_2 = 'TT_LISTEN=3301 ' ..
> +              'TT_REPLICATION=\"{\'0.0.0.0:12345\', ' ..
> +                              '\'1.1.1.1:12345\'}\" ' ..
> +              'TT_REPLICATION_CONNECT_TIMEOUT=0.01 ' ..
> +              'TT_REPLICATION_SYNCHRO_QUORUM=\'4 + 1\''
> +local set_3 = 'TT_BACKGROUND=true ' ..
> +              'TT_VINYL_TIMEOUT=60.1'
> +local set_4 = 'TT_SQL_CACHE_SIZE=a'
> +local set_5 = 'TT_STRIP_CORE=a'
> +local set_6 = 'TT_REPLICATION=\"{0.0.0.0:12345\', ' ..
> +                              '\'1.1.1.1:12345\'}\"'
> +local sets = {}
> +sets[1] = set_1
> +sets[2] = set_2
> +sets[3] = set_3
> +sets[4] = set_4
> +sets[5] = set_5
> +sets[6] = set_6
> +
> +for i, set in ipairs(sets) do
> +    local tmpdir = fio.tempdir()
> +    local new_path = fio.pathjoin(tmpdir, script_name)
> +    fio.copyfile(path_to_script, new_path)
> +    test:is(os.execute(shell_command(set, i)), 0, 'set '..i)
> +end

16) Add `test:plan()` and `test:check()`.

> +os.exit(0)
> diff --git a/test/box-tap/gh-5602_script.lua b/test/box-tap/gh-5602_script.lua

17) Use a more self-documenting name.

> new file mode 100755
> index 000000000..8ff34deb2
> --- /dev/null
> +++ b/test/box-tap/gh-5602_script.lua
> @@ -0,0 +1,70 @@
> +local tap = require('tap')
> +local test = tap.test('gh-5602')
> +
> +local status, err = pcall(box.cfg, {background = false, vinyl_timeout = 70.1})
> +
> +-- Check that environment cfg values are set correctly.
> +if arg[1] == '1' then
> +    test:plan(5)
> +    test:ok(status ~= false, 'box.cfg is successful')
> +    test:is(box.cfg['listen'], '3301', 'listen')
> +    test:is(box.cfg['readahead'], 10000, 'readahead')
> +    test:is(box.cfg['strip_core'], false, 'strip_core')
> +    test:is(box.cfg['log_format'], 'json', 'log_format is not set')
> +end
> +if arg[1] == '2' then
> +    test:plan(7)
> +    test:ok(status ~= false, 'box.cfg is successful')
> +    test:is(box.cfg['listen'], '3301', 'listen')
> +    local replication = box.cfg['replication']
> +    test:is(type(replication), 'table', 'replication is table')
> +    test:ok(replication[1] == '0.0.0.0:12345' or
> +            replication[1] == '1.1.1.1:12345', 'replication URI 1')
> +    test:ok(replication[2] == '0.0.0.0:12345' or
> +            replication[2] == '1.1.1.1:12345', 'replication URI 2')
> +    test:is(box.cfg['replication_connect_timeout'], 0.01,
> +            'replication_connect_timeout')
> +    test:is(box.cfg['replication_synchro_quorum'], '4 + 1',
> +            'replication_synchro_quorum')
> +end
> +
> +-- Check that box.cfg{} values are more prioritized than
> +-- environment cfg values.
> +if arg[1] == '3' then
> +    test:plan(3)
> +    test:ok(status ~= false, 'box.cfg is successful')
> +    test:is(box.cfg['background'], false,
> +            'box.cfg{} background value is prioritized')
> +    test:is(box.cfg['vinyl_timeout'], 70.1,
> +            'box.cfg{} vinyl_timeout value is prioritized')
> +end
> +
> +-- Check bad environment cfg values.
> +if arg[1] == '4' then
> +    test:plan(2)
> +    test:ok(status == false, 'box.cfg is not successful')
> +    local index = string.find(err, 'Environment variable TT_SQL_CACHE_SIZE '..
> +        'has incorrect value for option \'sql_cache_size\': should be '..
> +        'converted to number')
> +    test:ok(index ~= nil, 'bad sql_cache_size value')
> +end
> +if arg[1] == '5' then
> +    test:plan(2)
> +    test:ok(status == false, 'box.cfg is not successful')
> +    print(err)
> +    local index = string.find(err, 'Environment variable TT_STRIP_CORE has '..
> +        'incorrect value for option \'strip_core\': should be \'true\' '..
> +        'or \'false\'')
> +    test:ok(index ~= nil, 'bad strip_core value')
> +end
> +if arg[1] == '6' then
> +    test:plan(2)
> +    test:ok(status == false, 'box.cfg is not successful')
> +    print(err)
> +    local index = string.find(err, 'Environment variable TT_REPLICATION has '..
> +        'incorrect value for option \'replication\': elements of array should'..
> +        ' be quoted strings')
> +    test:ok(index ~= nil, 'bad replication value')
> +end
> +
> +os.exit(test:check() and 0 or 1)
> \ No newline at end of file

18) Add newline to the end of file.

> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH] box: set cfg via environment variables
  2021-03-14 15:34 [Tarantool-patches] [PATCH] box: set cfg via environment variables Roman Khabibov via Tarantool-patches
  2021-03-19 12:38 ` Leonid Vasiliev via Tarantool-patches
@ 2021-03-19 13:23 ` Leonid Vasiliev via Tarantool-patches
  1 sibling, 0 replies; 7+ messages in thread
From: Leonid Vasiliev via Tarantool-patches @ 2021-03-19 13:23 UTC (permalink / raw)
  To: Roman Khabibov, tarantool-patches



On 3/14/21 6:34 PM, Roman Khabibov wrote:
> Add ability to set box.cfg options via env variables. These
> variables should have name "TT_<OPTION>". When Tarantool instance
> is started under tarantoolctl utility env variables options have
> more priority than instance file's cfg.
> 
> Closes #5602
> 
> @TarantoolBot document
> Title: Set box.cfg options via env variables
> 
> Now, it is possible to set box.cfg options via environment
> variables. The name of variable should correspond the followng
> pattern: "TT_<NAME>", where <NAME> is an option name. For example:
> "TT_LISTEN", "TT_READAHEAD".
> If it is complex value, for example, "replication" can be set by a
> table, the corresponding environment varibale value has the syntax
> of lua array (without keys):
> 
> `export TT_REPLICATION="{'uri1', 'uri2'}"`
> 
> Note that every value in arary must be quoted.
> ---
> 
> @ChangeLog
> * Add ability to set box.cfg options via environment variables (gh-5602).

@Changelog - deprecated. See tarantool/doc/changelogs.md

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH] box: set cfg via environment variables
  2021-03-19 12:38 ` Leonid Vasiliev via Tarantool-patches
@ 2021-04-09  8:56   ` Roman Khabibov via Tarantool-patches
  2021-04-12  8:37     ` Leonid Vasiliev via Tarantool-patches
  2021-04-12  9:52     ` Leonid Vasiliev via Tarantool-patches
  0 siblings, 2 replies; 7+ messages in thread
From: Roman Khabibov via Tarantool-patches @ 2021-04-09  8:56 UTC (permalink / raw)
  To: Leonid Vasiliev; +Cc: tarantool-patches

Hi! Thanks for review.

> On Mar 19, 2021, at 15:38, Leonid Vasiliev <lvasiliev@tarantool.org> wrote:
> 
> Hi! Thank you for the patch.
> See comments bellow:
> 
> On 3/14/21 6:34 PM, Roman Khabibov wrote:
>> Add ability to set box.cfg options via env variables. These
>> variables should have name "TT_<OPTION>". When Tarantool instance
>> is started under tarantoolctl utility env variables options have
>> more priority than instance file's cfg.
>> Closes #5602
>> @TarantoolBot document
>> Title: Set box.cfg options via env variables
>> Now, it is possible to set box.cfg options via environment
>> variables. The name of variable should correspond the followng
> 
> 1) followng -> following
> 
>> pattern: "TT_<NAME>", where <NAME> is an option name. For example:
>> "TT_LISTEN", "TT_READAHEAD".
>> If it is complex value, for example, "replication" can be set by a
>> table, the corresponding environment varibale value has the syntax
> 
> 2) varibale -> variable
> 
>> of lua array (without keys):
>> `export TT_REPLICATION="{'uri1', 'uri2'}"`
>> Note that every value in arary must be quoted.
> 
> 1) arary -> array
    box: set cfg via environment variables
    
    Add ability to set box.cfg options via env variables. These
    variables should have name "TT_<OPTION>". When Tarantool instance
    is started under tarantoolctl utility env variables options have
    more priority than instance file's cfg.
    
    Closes #5602
    
    @TarantoolBot document
    Title: Set box.cfg options via env variables
    
    Now, it is possible to set box.cfg options via environment
    variables. The name of variable should correspond the following
    pattern: "TT_<NAME>", where <NAME> is an option name. For example:
    "TT_LISTEN", "TT_READAHEAD".
    If it is complex value, for example, "replication" can be set by a
    table, the corresponding environment variable value has the syntax
    of lua array (without keys):
    
    `export TT_REPLICATION="{'uri1', 'uri2'}”`

>> ---
>> @ChangeLog
>> * Add ability to set box.cfg options via environment variables (gh-5602).
>> Branch: https://github.com/tarantool/tarantool/tree/romanhabibov/gh-5602-environment-variables
>> Issue: https://github.com/tarantool/tarantool/issues/5602
>>  src/box/lua/load_cfg.lua                      | 74 +++++++++++++++++++
>>  test/app-tap/tarantoolctl.test.lua            | 33 ++++++++-
>>  .../gh-5602-environment-vars-cfg.result       | 42 +++++++++++
>>  .../gh-5602-environment-vars-cfg.test.lua     | 53 +++++++++++++
>>  test/box-tap/gh-5602_script.lua               | 70 ++++++++++++++++++
>>  5 files changed, 271 insertions(+), 1 deletion(-)
>>  create mode 100644 test/box-tap/gh-5602-environment-vars-cfg.result
>>  create mode 100755 test/box-tap/gh-5602-environment-vars-cfg.test.lua
>>  create mode 100755 test/box-tap/gh-5602_script.lua
>> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
>> index 574c8bef4..eace25d46 100644
>> --- a/src/box/lua/load_cfg.lua
>> +++ b/src/box/lua/load_cfg.lua
>> @@ -616,6 +616,79 @@ setmetatable(box, {
>>       end
>>  })
> 
> 4) Add descriptions to functions (must start with `---`).
Done.

>> +local function process_option_value(option, raw_value)
>> +    local param_type = template_cfg[option]
>> +    if param_type == nil then
>> +        return nil
>> +    end
>> +    local value = nil
>> +    if param_type == 'number' or option == 'log_level' then
>> +        value = tonumber(raw_value)
>> +        if value == nil then
>> +            error("Environment variable "..'TT_'..string.upper(option).." has"..
>> +                  " incorrect value for option '"..option.."': should be "..
>> +                  "converted to number")
> 
> 5) Codestyle (apply to the whole patch):
>    - Indetation.
>        From https://dev.minetest.net/Lua_code_style_guidelines :
>        "When strings don't fit into the line, you should add the string
>        (changes) to the next line(s) indented by one tab." (4 spaces in our case).
>    - String format.
>        From https://www.tarantool.io/en/doc/latest/dev_guide/lua_style_guide/#indentation-and-formatting:
>        "Avoid multiple concatenations in one statement, use string.format instead”
Done.

>> +        end
>> +    elseif param_type == 'boolean' or option == 'log_nonblock' then
>> +        if raw_value == 'true' then
>> +            value = true
>> +        elseif raw_value == 'false' then
>> +            value = false
>> +        else
>> +            error("Environment variable "..'TT_'..string.upper(option).." has"..
>> +                  " incorrect value for option '"..option.."': should be "..
>> +                  "'true' or 'false'")
>> +        end
>> +    elseif param_type == 'string, number' then
>> +        value = tonumber(raw_value)
>> +        if value == nil then
>> +            value = raw_value
>> +        end
>> +    elseif param_type == 'string, number, table' then
> 
> 6) OMG! We have to get the table from the env variable. Nothing good to expect.
>    - What about a nested table?
>    - Also, what about the cases "string, table" or "table, string, number" ...? Now we don't have such a `param_type`, but they will appear tomorrow.
Now, it is 'string, number, table’ only.

>> +        if string.sub(raw_value, 1, 1) == '{' and
>> +           string.sub(raw_value, -1) == '}' then
>> +            value = string.sub(raw_value, 2, -2)
>> +            value = require('csv').load(value)[1]
>> +            for i, str in pairs(value) do
>> +                if not (string.sub(str, 1, 1) == '\'' and
>> +                        string.sub(str, -1) == '\'') and
>> +                   not (string.sub(str, 1, 1) == '"' and
>> +                        string.sub(str, -1) == '"') then
>> +                    error("Environment variable "..'TT_'..string.upper(option)..
>> +                          " has incorrect value for option '"..option.."': "..
>> +                          "elements of array should be quoted strings")
>> +                end
>> +                str = string.sub(str, 2, -2)
> 
> 7) Shouldn't we be checking types inside a table? Or can there only be strings?
> 
>> +                value[i] = str
>> +            end
>> +        else
>> +            value = tonumber(raw_value)
>> +            if value == nil then
>> +                value = raw_value
>> +            end
>> +        end
>> +    else
>> +        value = raw_value
>> +    end
>> +
>> +    return value
>> +end
>> +
>> +local function collect_env_cfg(cfg)
>> +    if cfg == nil then
>> +        cfg = {}
>> +    end
> 
> 8) Please, separate logical blocks of code from each other with empty lines. Write comments.(apply to the whole patch).
Done.

>> +    for option, _ in pairs(template_cfg) do
>> +        if cfg[option] == nil or os.getenv('TARANTOOLCTL') == 'true' then
>> +            local env_var_name = 'TT_'..string.upper(option)
>> +            local raw_value = os.getenv(env_var_name)
>> +            if raw_value ~= nil then
>> +                cfg[option] = process_option_value(option, raw_value)
>> +            end
>> +        end
>> +    end
>> +end
>> +
>>  -- Whether box is loaded.
>>  --
>>  -- `false` when box is not configured or when the initialization
>> @@ -627,6 +700,7 @@ setmetatable(box, {
>>  local box_is_configured = false
>>    local function load_cfg(cfg)
>> +    collect_env_cfg(cfg)
>>      -- A user may save box.cfg (this function) before box loading
>>      -- and call it afterwards. We should reconfigure box in the
>>      -- case.
> 
> 9) It looks like in this case we can overload the value that was set in
> the previous call with the value from the environment. Or not?
Discussed.

>> diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
>> index 3c3023d34..1f15fe103 100755
>> --- a/test/app-tap/tarantoolctl.test.lua
>> +++ b/test/app-tap/tarantoolctl.test.lua
>> @@ -160,7 +160,7 @@ local function merge(...)
>>  end
>>    local test = tap.test('tarantoolctl')
>> -test:plan(8)
>> +test:plan(9)
>>    -- basic start/stop test
>>  -- must be stopped afterwards
>> @@ -632,4 +632,35 @@ test:test('filter_xlog', function(test)
>>      end
>>  end)
>>  +-- gh-5602: Check that environment cfg variables are more
>> +-- prioritized than tarantoolctl cfg.
>> +do
> 
> 10) As for me, it is preferable to use `test:test()` instead of `do...end`. Up to you.
> 
>> +    local dir = fio.tempdir()
>> +    local code = [[ box.cfg ]]
> 
> 11) Maybe `[[ box.cfg{} ]]`
No, box.cfg. It prints cfg.

>> +    create_script(dir, 'show_cfg.lua',  code)
> 
> 12) Two spaces before `code`, but one is needed.
Done.

>> +    local code = [[ box.cfg{listen=3302, readahead=10001} ]]
>> +    create_script(dir, 'cfg_script.lua', code)
>> +
>> +    local status, err = pcall(function()
>> +        test:test("check answers in case of call", function(test_i)
>> +            test_i:plan(4)
>> +            os.setenv('TT_LISTEN', '3301')
> 
> 13) We will not have problems due to a specific port when many tests are
> run in parallel (such as this port will be already occupied).
Changed to checkpoint_interval.

>> +            os.setenv('TT_READAHEAD', '10000')
>> +            check_ok(test_i, dir, 'start', 'cfg_script', 0)
>> +            tctl_wait_start(dir, 'cfg_script')
>> +            check_ok(test_i, dir, 'eval',  'cfg_script show_cfg.lua', 0,
>> +                     '---\n- 3301\n- 10000\n...', nil)
>> +            check_ok(test_i, dir, 'stop', 'cfg_script', 0)
>> +        end)
>> +    end)
>> +
>> +    cleanup_instance(dir, 'cfg_script')
>> +    recursive_rmdir(dir)
>> +
>> +    if status == false then
>> +        print(("Error: %s"):format(err))
>> +        os.exit()
>> +    end
>> +end
>> +
>>  os.exit(test:check() == true and 0 or -1)
>> diff --git a/test/box-tap/gh-5602-environment-vars-cfg.result b/test/box-tap/gh-5602-environment-vars-cfg.result
>> new file mode 100644
>> index 000000000..495e02a73
>> --- /dev/null
>> +++ b/test/box-tap/gh-5602-environment-vars-cfg.result
>> @@ -0,0 +1,42 @@
>> +TAP version 13
>> +1..5
>> +ok - box.cfg is successful
>> +ok - listen
>> +ok - readahead
>> +ok - strip_core
>> +ok - log_format is not set
>> +TAP version 13
>> +1..7
>> +ok - box.cfg is successful
>> +ok - listen
>> +ok - replication is table
>> +ok - replication URI 1
>> +ok - replication URI 2
>> +ok - replication_connect_timeout
>> +ok - replication_synchro_quorum
>> +TAP version 13
>> +1..3
>> +ok - box.cfg is successful
>> +ok - box.cfg{} background value is prioritized
>> +ok - box.cfg{} vinyl_timeout value is prioritized
>> +TAP version 13
>> +1..2
>> +ok - box.cfg is not successful
>> +ok - bad sql_cache_size value
>> +TAP version 13
>> +1..2
>> +ok - box.cfg is not successful
>> +builtin/box/load_cfg.lua:19: builtin/box/load_cfg.lua:638: Environment variable TT_STRIP_CORE has incorrect value for option 'strip_core': should be 'true' or 'false'
>> +ok - bad strip_core value
>> +TAP version 13
>> +1..2
>> +ok - box.cfg is not successful
>> +builtin/box/load_cfg.lua:19: builtin/box/load_cfg.lua:657: Environment variable TT_REPLICATION has incorrect value for option 'replication': elements of array should be quoted strings
>> +ok - bad replication value
>> +TAP version 13
>> +ok - set 1
>> +ok - set 2
>> +ok - set 3
>> +ok - set 4
>> +ok - set 5
>> +ok - set 6
>> diff --git a/test/box-tap/gh-5602-environment-vars-cfg.test.lua b/test/box-tap/gh-5602-environment-vars-cfg.test.lua
>> new file mode 100755
>> index 000000000..84b49b7eb
>> --- /dev/null
>> +++ b/test/box-tap/gh-5602-environment-vars-cfg.test.lua
>> @@ -0,0 +1,53 @@
>> +#!/usr/bin/env tarantool
>> +
>> +local os = require('os')
>> +local fio = require('fio')
>> +local tap = require('tap')
> 
> 13) It's nice to separate the variable definition and the `requires` with empty line.(apply to the whole patch)
Done.

>> +local test = tap.test('gh-5602')
>> +
>> +-- gh-5602: Check that environment cfg variables working.
>> +local TARANTOOL_PATH = arg[-1]
>> +local script_name = 'gh-5602_script.lua'
>> +local path_to_script = fio.pathjoin(
>> +        os.getenv('PWD'),
>> +        'box-tap',
>> +        script_name)
> 
> 14) Please, separate the function definition with an empty line.
Done.
> 15) Describe the test mechanism using a comment.
Done.

>> +local function shell_command(set, i)
>> +    string.gsub(set, 'TT_', '')
>> +    return ('%s %s %s %d'):format(
>> +        set,
>> +        TARANTOOL_PATH,
>> +        path_to_script,
>> +        i)
>> +end
>> +
>> +local set_1 = 'TT_LISTEN=3301 ' ..
>> +              'TT_READAHEAD=10000 ' ..
>> +              'TT_STRIP_CORE=false ' ..
>> +              'TT_LOG_FORMAT=json'
>> +local set_2 = 'TT_LISTEN=3301 ' ..
>> +              'TT_REPLICATION=\"{\'0.0.0.0:12345\', ' ..
>> +                              '\'1.1.1.1:12345\'}\" ' ..
>> +              'TT_REPLICATION_CONNECT_TIMEOUT=0.01 ' ..
>> +              'TT_REPLICATION_SYNCHRO_QUORUM=\'4 + 1\''
>> +local set_3 = 'TT_BACKGROUND=true ' ..
>> +              'TT_VINYL_TIMEOUT=60.1'
>> +local set_4 = 'TT_SQL_CACHE_SIZE=a'
>> +local set_5 = 'TT_STRIP_CORE=a'
>> +local set_6 = 'TT_REPLICATION=\"{0.0.0.0:12345\', ' ..
>> +                              '\'1.1.1.1:12345\'}\"'
>> +local sets = {}
>> +sets[1] = set_1
>> +sets[2] = set_2
>> +sets[3] = set_3
>> +sets[4] = set_4
>> +sets[5] = set_5
>> +sets[6] = set_6
>> +
>> +for i, set in ipairs(sets) do
>> +    local tmpdir = fio.tempdir()
>> +    local new_path = fio.pathjoin(tmpdir, script_name)
>> +    fio.copyfile(path_to_script, new_path)
>> +    test:is(os.execute(shell_command(set, i)), 0, 'set '..i)
>> +end
> 
> 16) Add `test:plan()` and `test:check()`.
done.

>> +os.exit(0)
>> diff --git a/test/box-tap/gh-5602_script.lua b/test/box-tap/gh-5602_script.lua
> 
> 17) Use a more self-documenting name.
Done.

>> new file mode 100755
>> index 000000000..8ff34deb2
>> --- /dev/null
>> +++ b/test/box-tap/gh-5602_script.lua
>> @@ -0,0 +1,70 @@
>> +local tap = require('tap')
>> +local test = tap.test('gh-5602')
>> +
>> +local status, err = pcall(box.cfg, {background = false, vinyl_timeout = 70.1})
>> +
>> +-- Check that environment cfg values are set correctly.
>> +if arg[1] == '1' then
>> +    test:plan(5)
>> +    test:ok(status ~= false, 'box.cfg is successful')
>> +    test:is(box.cfg['listen'], '3301', 'listen')
>> +    test:is(box.cfg['readahead'], 10000, 'readahead')
>> +    test:is(box.cfg['strip_core'], false, 'strip_core')
>> +    test:is(box.cfg['log_format'], 'json', 'log_format is not set')
>> +end
>> +if arg[1] == '2' then
>> +    test:plan(7)
>> +    test:ok(status ~= false, 'box.cfg is successful')
>> +    test:is(box.cfg['listen'], '3301', 'listen')
>> +    local replication = box.cfg['replication']
>> +    test:is(type(replication), 'table', 'replication is table')
>> +    test:ok(replication[1] == '0.0.0.0:12345' or
>> +            replication[1] == '1.1.1.1:12345', 'replication URI 1')
>> +    test:ok(replication[2] == '0.0.0.0:12345' or
>> +            replication[2] == '1.1.1.1:12345', 'replication URI 2')
>> +    test:is(box.cfg['replication_connect_timeout'], 0.01,
>> +            'replication_connect_timeout')
>> +    test:is(box.cfg['replication_synchro_quorum'], '4 + 1',
>> +            'replication_synchro_quorum')
>> +end
>> +
>> +-- Check that box.cfg{} values are more prioritized than
>> +-- environment cfg values.
>> +if arg[1] == '3' then
>> +    test:plan(3)
>> +    test:ok(status ~= false, 'box.cfg is successful')
>> +    test:is(box.cfg['background'], false,
>> +            'box.cfg{} background value is prioritized')
>> +    test:is(box.cfg['vinyl_timeout'], 70.1,
>> +            'box.cfg{} vinyl_timeout value is prioritized')
>> +end
>> +
>> +-- Check bad environment cfg values.
>> +if arg[1] == '4' then
>> +    test:plan(2)
>> +    test:ok(status == false, 'box.cfg is not successful')
>> +    local index = string.find(err, 'Environment variable TT_SQL_CACHE_SIZE '..
>> +        'has incorrect value for option \'sql_cache_size\': should be '..
>> +        'converted to number')
>> +    test:ok(index ~= nil, 'bad sql_cache_size value')
>> +end
>> +if arg[1] == '5' then
>> +    test:plan(2)
>> +    test:ok(status == false, 'box.cfg is not successful')
>> +    print(err)
>> +    local index = string.find(err, 'Environment variable TT_STRIP_CORE has '..
>> +        'incorrect value for option \'strip_core\': should be \'true\' '..
>> +        'or \'false\'')
>> +    test:ok(index ~= nil, 'bad strip_core value')
>> +end
>> +if arg[1] == '6' then
>> +    test:plan(2)
>> +    test:ok(status == false, 'box.cfg is not successful')
>> +    print(err)
>> +    local index = string.find(err, 'Environment variable TT_REPLICATION has '..
>> +        'incorrect value for option \'replication\': elements of array should'..
>> +        ' be quoted strings')
>> +    test:ok(index ~= nil, 'bad replication value')
>> +end
>> +
>> +os.exit(test:check() and 0 or 1)
>> \ No newline at end of file
> 
> 18) Add newline to the end of file.

commit ee6525f843a347c1df8a92a015414f4cbcceafb9
Author: Roman Khabibov <roman.habibov@tarantool.org>
Date:   Tue Mar 9 16:47:11 2021 +0300

    box: set cfg via environment variables
    
    Add ability to set box.cfg options via env variables. These
    variables should have name "TT_<OPTION>". When Tarantool instance
    is started under tarantoolctl utility env variables options have
    more priority than instance file's cfg.
    
    Closes #5602
    
    @TarantoolBot document
    Title: Set box.cfg options via env variables
    
    Now, it is possible to set box.cfg options via environment
    variables. The name of variable should correspond the following
    pattern: "TT_<NAME>", where <NAME> is an option name. For example:
    "TT_LISTEN", "TT_READAHEAD".
    If it is complex value, for example, "replication" can be set by a
    table, the corresponding environment variable value has the syntax
    of lua array (without keys):
    
    `export TT_REPLICATION="{'uri1', 'uri2'}"`

diff --git a/changelogs/unreleased/environment-cfg.md b/changelogs/unreleased/environment-cfg.md
new file mode 100755
index 000000000..c5d5005ac
--- /dev/null
+++ b/changelogs/unreleased/environment-cfg.md
@@ -0,0 +1,5 @@
+## feature/core
+
+* Now, it is possibly to set box.cfg options with environment variables.
+The priority of sources of configuration options is the following (from low
+to high): default, tarantoolctl, environment, box.cfg{}. (gh-5602).
\ No newline at end of file
diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index 574c8bef4..a1439ae46 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -416,6 +416,20 @@ local translate_cfg = {
     end},
 }
 
+-- Configuration options from environment is taken into account
+-- only at the first box.cfg{} call. When box.cfg is reload, they
+-- are ignored.
+--
+-- The priority of sources of configuration options is the
+-- following (from low to high): default, tarantoolctl,
+-- environment, box.cfg{}.
+local env_cfg = {}
+-- This is auxiliary table and it needed to keep values which
+-- passed or typed to box.cfg{} at the first call. Then we
+-- distinguish values (it was from environment or not) and decide
+-- ignore it or not during reload.
+local user_cfg = {}
+
 -- Upgrade old config
 local function upgrade_cfg(cfg, translate_cfg)
     if cfg == nil then
@@ -515,6 +529,22 @@ local function prepare_cfg(cfg, default_cfg, template_cfg,
     return new_cfg
 end
 
+---
+-- Transfer options from env_cfg to cfg.
+--
+local function apply_env_cfg(cfg, env_cfg)
+    if env_cfg == nil then
+        return
+    end
+    for k, v in pairs(env_cfg) do
+        if cfg[k] == nil or os.getenv('TARANTOOLCTL') == 'true' then
+            cfg[k] = v
+        elseif type(v) == 'table' then
+            apply_env_cfg(cfg[k], v)
+        end
+    end
+end
+
 local function apply_default_cfg(cfg, default_cfg, module_cfg)
     for k,v in pairs(default_cfg) do
         if cfg[k] == nil then
@@ -549,7 +579,19 @@ local function compare_cfg(cfg1, cfg2)
     return true
 end
 
+---
+-- Nullify option values from environment in old configuration.
+--
+local function reset_env_options(oldcfg)
+    for k, v in pairs(env_cfg) do
+        if user_cfg[k] == nil then
+            oldcfg[k] = nil
+        end
+    end
+end
+
 local function reload_cfg(oldcfg, cfg)
+    reset_env_options(oldcfg)
     cfg = upgrade_cfg(cfg, translate_cfg)
     local newcfg = prepare_cfg(cfg, default_cfg, template_cfg,
                                module_cfg, modify_cfg)
@@ -616,6 +658,104 @@ setmetatable(box, {
      end
 })
 
+---
+-- Erase quotes at the begin and the end of string.
+--
+local function dequote(str)
+    if string.sub(str, 1, 1) == '\'' and string.sub(str, -1) == '\'' or
+       string.sub(str, 1, 1) == '"' and string.sub(str, -1) == '"' then
+        return string.sub(str, 2, -2)
+    end
+    return str
+end
+
+---
+-- Convert raw option value becoming from the corresponding
+-- environment variable represented as string to string, number or
+-- boolean (depending on option).
+--
+local function process_option_value(option, raw_value)
+    local param_type = template_cfg[option]
+
+    -- May be peculiar to "feedback*" parameters.
+    if param_type == nil then
+        return nil
+    end
+
+    local value = nil
+    local err_msg_fmt = 'Environment variable TT_%s has '..
+        'incorrect value for option \'%s\': should be %s'
+
+    -- "log_level" option should be a number.
+    if param_type == 'number' or option == 'log_level' then
+        value = tonumber(raw_value)
+        if value == nil then
+            error(err_msg_fmt:format(string.upper(option), option,
+                    "convertible to number"))
+        end
+    -- "log_nonblock" option should be boolean.
+    elseif param_type == 'boolean' or option == 'log_nonblock' then
+        if raw_value == 'true' then
+            value = true
+        elseif raw_value == 'false' then
+            value = false
+        else
+            error(err_msg_fmt:format(string.upper(option), option,
+                    "'true' or 'false'"))
+        end
+    elseif param_type == 'string, number' then
+        -- Firstly, try to convert to number.
+        value = tonumber(raw_value)
+        if value == nil then
+            value = raw_value
+        end
+    elseif param_type == 'string, number, table' then
+        if string.sub(raw_value, 1, 1) == '{' and
+           string.sub(raw_value, -1) == '}' then
+            value = string.sub(raw_value, 2, -2)
+            value = require('csv').load(value)[1]
+            for i, item in pairs(value) do
+                value[i] = dequote(item)
+            end
+        else
+            value = tonumber(raw_value)
+            if value == nil then
+                value = raw_value
+            end
+        end
+    else
+        value = raw_value
+    end
+
+    return value
+end
+
+-- local function collect_env_cfg()
+--     local env_cfg = {}
+--     for option, _ in pairs(template_cfg) do
+--         if cfg[option] == nil or os.getenv('TARANTOOLCTL') == 'true' then
+--             local env_var_name = 'TT_'..string.upper(option)
+--             local raw_value = os.getenv(env_var_name)
+--             if raw_value ~= nil then
+--                 cfg[option] = process_option_value(option, raw_value)
+--             end
+--         end
+--     end
+-- end
+
+---
+-- Pull and process configuration values from environment.
+--
+local function collect_env_cfg()
+    for option, _ in pairs(template_cfg) do
+        local env_var_name = 'TT_'..string.upper(option)
+        local raw_value = os.getenv(env_var_name)
+        if raw_value ~= nil then
+            env_cfg[option] = process_option_value(option, raw_value)
+        end
+    end
+end
+
 -- Whether box is loaded.
 --
 -- `false` when box is not configured or when the initialization
@@ -627,6 +767,7 @@ setmetatable(box, {
 local box_is_configured = false
 
 local function load_cfg(cfg)
+    user_cfg = cfg
     -- A user may save box.cfg (this function) before box loading
     -- and call it afterwards. We should reconfigure box in the
     -- case.
@@ -638,6 +779,8 @@ local function load_cfg(cfg)
     cfg = upgrade_cfg(cfg, translate_cfg)
     cfg = prepare_cfg(cfg, default_cfg, template_cfg,
                       module_cfg, modify_cfg)
+    collect_env_cfg()
+    apply_env_cfg(cfg, env_cfg)
     apply_default_cfg(cfg, default_cfg, module_cfg);
     -- Save new box.cfg
     box.cfg = cfg
diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
index 3c3023d34..060316714 100755
--- a/test/app-tap/tarantoolctl.test.lua
+++ b/test/app-tap/tarantoolctl.test.lua
@@ -160,7 +160,7 @@ local function merge(...)
 end
 
 local test = tap.test('tarantoolctl')
-test:plan(8)
+test:plan(9)
 
 -- basic start/stop test
 -- must be stopped afterwards
@@ -632,4 +632,35 @@ test:test('filter_xlog', function(test)
     end
 end)
 
+-- gh-5602: Check that environment cfg variables are more
+-- prioritized than tarantoolctl cfg.
+do
+    local dir = fio.tempdir()
+    local code = [[ box.cfg ]]
+    create_script(dir, 'show_cfg.lua', code)
+    local code = [[ box.cfg{checkpoint_interval=3302, readahead=10001} ]]
+    create_script(dir, 'cfg_script.lua', code)
+
+    local status, err = pcall(function()
+        test:test("check answers in case of call", function(test_i)
+            test_i:plan(4)
+            os.setenv('TT_CHECKPOINT_INTERVAL', '3301')
+            os.setenv('TT_READAHEAD', '10000')
+            check_ok(test_i, dir, 'start', 'cfg_script', 0)
+            tctl_wait_start(dir, 'cfg_script')
+            check_ok(test_i, dir, 'eval',  'cfg_script show_cfg.lua', 0,
+                     '---\n- 3301\n- 10000\n...', nil)
+            check_ok(test_i, dir, 'stop', 'cfg_script', 0)
+        end)
+    end)
+
+    cleanup_instance(dir, 'cfg_script')
+    recursive_rmdir(dir)
+
+    if status == false then
+        print(("Error: %s"):format(err))
+        os.exit()
+    end
+end
+
 os.exit(test:check() == true and 0 or -1)
diff --git a/test/box-tap/gh-5602-environment-vars-cfg.result b/test/box-tap/gh-5602-environment-vars-cfg.result
new file mode 100644
index 000000000..c90dd301b
--- /dev/null
+++ b/test/box-tap/gh-5602-environment-vars-cfg.result
@@ -0,0 +1,41 @@
+TAP version 13
+1..5
+ok - box.cfg is successful
+ok - listen
+ok - readahead
+ok - strip_core
+ok - log_format is not set
+TAP version 13
+1..7
+ok - box.cfg is successful
+ok - listen
+ok - replication is table
+ok - replication URI 1
+ok - replication URI 2
+ok - replication_connect_timeout
+ok - replication_synchro_quorum
+TAP version 13
+1..3
+ok - box.cfg is successful
+ok - box.cfg{} background value is prioritized
+ok - box.cfg{} vinyl_timeout value is prioritized
+TAP version 13
+1..2
+ok - box.cfg is not successful
+ok - bad sql_cache_size value
+TAP version 13
+1..2
+ok - box.cfg is not successful
+ok - bad strip_core value
+TAP version 13
+1..2
+ok - box.cfg is not successful
+ok - bad replication value
+TAP version 13
+1..6
+ok - set 1
+ok - set 2
+ok - set 3
+ok - set 4
+ok - set 5
+ok - set 6
diff --git a/test/box-tap/gh-5602-environment-vars-cfg.test.lua b/test/box-tap/gh-5602-environment-vars-cfg.test.lua
new file mode 100755
index 000000000..3d19fdc90
--- /dev/null
+++ b/test/box-tap/gh-5602-environment-vars-cfg.test.lua
@@ -0,0 +1,57 @@
+#!/usr/bin/env tarantool
+
+local os = require('os')
+local fio = require('fio')
+local tap = require('tap')
+
+local test = tap.test('gh-5602')
+
+-- gh-5602: Check that environment cfg variables working.
+local TARANTOOL_PATH = arg[-1]
+local script_name = 'gh-5602-test-cases-script.lua'
+local path_to_script = fio.pathjoin(
+        os.getenv('PWD'),
+        'box-tap',
+        script_name)
+
+---
+-- Generate shell command where set is sequence of environment
+-- variables, then path to tarantool, the first arg is path to
+-- test script and the second one is number of set.
+--
+local function shell_command(set, i)
+    return ('%s %s %s %d'):format(
+        set,
+        TARANTOOL_PATH,
+        path_to_script,
+        i)
+end
+
+local set_1 = ('%s %s %s %s'):format('TT_LISTEN=3301', 'TT_READAHEAD=10000',
+    'TT_STRIP_CORE=false', 'TT_LOG_FORMAT=json')
+local set_2 = ('%s %s %s %s'):format('TT_LISTEN=3301',
+    'TT_REPLICATION=\"{\'0.0.0.0:12345\', \'1.1.1.1:12345\'}\"',
+    'TT_REPLICATION_CONNECT_TIMEOUT=0.01',
+    'TT_REPLICATION_SYNCHRO_QUORUM=\'4 + 1\'')
+local set_3 = 'TT_BACKGROUND=true TT_VINYL_TIMEOUT=60.1'
+local set_4 = 'TT_SQL_CACHE_SIZE=a'
+local set_5 = 'TT_STRIP_CORE=a'
+local set_6 = 'TT_REPLICATION=\"{0.0.0.0:12345\', \'1.1.1.1:12345\'}\"'
+
+local sets = {}
+sets[1] = set_1
+sets[2] = set_2
+sets[3] = set_3
+sets[4] = set_4
+sets[5] = set_5
+sets[6] = set_6
+
+test:plan(#sets)
+for i, set in ipairs(sets) do
+    local tmpdir = fio.tempdir()
+    local new_path = fio.pathjoin(tmpdir, script_name)
+    fio.copyfile(path_to_script, new_path)
+    test:is(os.execute(shell_command(set, i)), 0, 'set '..i)
+end
+
+os.exit(test:check() and 0 or 1)
diff --git a/test/box-tap/gh-5602-test-cases-script.lua b/test/box-tap/gh-5602-test-cases-script.lua
new file mode 100755
index 000000000..a8b2fc987
--- /dev/null
+++ b/test/box-tap/gh-5602-test-cases-script.lua
@@ -0,0 +1,69 @@
+local tap = require('tap')
+
+local test = tap.test('gh-5602')
+
+local status, err = pcall(box.cfg, {background = false, vinyl_timeout = 70.1})
+
+-- Check that environment cfg values are set correctly.
+if arg[1] == '1' then
+    test:plan(5)
+    test:ok(status ~= false, 'box.cfg is successful')
+    test:is(box.cfg['listen'], 3301, 'listen')
+    test:is(box.cfg['readahead'], 10000, 'readahead')
+    test:is(box.cfg['strip_core'], false, 'strip_core')
+    test:is(box.cfg['log_format'], 'json', 'log_format is not set')
+end
+if arg[1] == '2' then
+    test:plan(7)
+    test:ok(status ~= false, 'box.cfg is successful')
+    test:is(box.cfg['listen'], 3301, 'listen')
+    local replication = box.cfg['replication']
+    test:is(type(replication), 'table', 'replication is table')
+    test:ok(replication[1] == '0.0.0.0:12345' or
+            replication[1] == '1.1.1.1:12345', 'replication URI 1')
+    test:ok(replication[2] == '0.0.0.0:12345' or
+            replication[2] == '1.1.1.1:12345', 'replication URI 2')
+    test:is(box.cfg['replication_connect_timeout'], 0.01,
+            'replication_connect_timeout')
+    test:is(box.cfg['replication_synchro_quorum'], '4 + 1',
+            'replication_synchro_quorum')
+end
+
+-- Check that box.cfg{} values are more prioritized than
+-- environment cfg values.
+if arg[1] == '3' then
+    test:plan(3)
+    test:ok(status ~= false, 'box.cfg is successful')
+    test:is(box.cfg['background'], false,
+            'box.cfg{} background value is prioritized')
+    test:is(box.cfg['vinyl_timeout'], 70.1,
+            'box.cfg{} vinyl_timeout value is prioritized')
+end
+
+local err_msg_fmt = 'Environment variable TT_%s has '..
+    'incorrect value for option \'%s\': should be %s'
+
+-- Check bad environment cfg values.
+if arg[1] == '4' then
+    test:plan(2)
+    test:ok(status == false, 'box.cfg is not successful')
+    local index = string.find(err, err_msg_fmt:format('SQL_CACHE_SIZE',
+        'sql_cache_size', 'convertible to number'))
+    test:ok(index ~= nil, 'bad sql_cache_size value')
+end
+if arg[1] == '5' then
+    test:plan(2)
+    test:ok(status == false, 'box.cfg is not successful')
+    local index = string.find(err, err_msg_fmt:format('STRIP_CORE',
+        'strip_core', '\'true\' or \'false\''))
+    test:ok(index ~= nil, 'bad strip_core value')
+end
+if arg[1] == '6' then
+    test:plan(2)
+    test:ok(status == false, 'box.cfg is not successful')
+    local index = string.find(tostring(err), 'Incorrect value for option '..
+        '\'replication\': expected host:service or /unix.socket')
+    test:ok(index ~= nil, 'bad replication value')
+end
+
+os.exit(test:check() and 0 or 1)


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH] box: set cfg via environment variables
  2021-04-09  8:56   ` Roman Khabibov via Tarantool-patches
@ 2021-04-12  8:37     ` Leonid Vasiliev via Tarantool-patches
  2021-04-13 12:47       ` Roman Khabibov via Tarantool-patches
  2021-04-12  9:52     ` Leonid Vasiliev via Tarantool-patches
  1 sibling, 1 reply; 7+ messages in thread
From: Leonid Vasiliev via Tarantool-patches @ 2021-04-12  8:37 UTC (permalink / raw)
  To: Roman Khabibov; +Cc: tarantool-patches

Hi! Thank you for the patch.
See 9 comments below:

> 
> commit ee6525f843a347c1df8a92a015414f4cbcceafb9
> Author: Roman Khabibov <roman.habibov@tarantool.org>
> Date:   Tue Mar 9 16:47:11 2021 +0300
> 
>      box: set cfg via environment variables
>      
>      Add ability to set box.cfg options via env variables. These
>      variables should have name "TT_<OPTION>". When Tarantool instance
>      is started under tarantoolctl utility env variables options have
>      more priority than instance file's cfg.
>      
>      Closes #5602
>      
>      @TarantoolBot document
>      Title: Set box.cfg options via env variables
>      
>      Now, it is possible to set box.cfg options via environment
>      variables. The name of variable should correspond the following
>      pattern: "TT_<NAME>", where <NAME> is an option name. For example:
>      "TT_LISTEN", "TT_READAHEAD".
>      If it is complex value, for example, "replication" can be set by a
>      table, the corresponding environment variable value has the syntax
>      of lua array (without keys):
>      
>      `export TT_REPLICATION="{'uri1', 'uri2'}"`
> 
> diff --git a/changelogs/unreleased/environment-cfg.md b/changelogs/unreleased/environment-cfg.md
> new file mode 100755
> index 000000000..c5d5005ac
> --- /dev/null
> +++ b/changelogs/unreleased/environment-cfg.md
> @@ -0,0 +1,5 @@
> +## feature/core
> +
> +* Now, it is possibly to set box.cfg options with environment variables.
> +The priority of sources of configuration options is the following (from low
> +to high): default, tarantoolctl, environment, box.cfg{}. (gh-5602).
> \ No newline at end of file
> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
> index 574c8bef4..a1439ae46 100644
> --- a/src/box/lua/load_cfg.lua
> +++ b/src/box/lua/load_cfg.lua
> @@ -416,6 +416,20 @@ local translate_cfg = {
>       end},
>   }
>   
> +-- Configuration options from environment is taken into account
> +-- only at the first box.cfg{} call. When box.cfg is reload, they
> +-- are ignored.
> +--
> +-- The priority of sources of configuration options is the
> +-- following (from low to high): default, tarantoolctl,
> +-- environment, box.cfg{}.
> +local env_cfg = {}
> +-- This is auxiliary table and it needed to keep values which
> +-- passed or typed to box.cfg{} at the first call. Then we
> +-- distinguish values (it was from environment or not) and decide
> +-- ignore it or not during reload.
> +local user_cfg = {}

1) AFAIU we decided what it's not needed. In my mind, on reload, we have
no difference between the options that were set using environment and any
other way.

> +
>   -- Upgrade old config
>   local function upgrade_cfg(cfg, translate_cfg)
>       if cfg == nil then
> @@ -515,6 +529,22 @@ local function prepare_cfg(cfg, default_cfg, template_cfg,
>       return new_cfg
>   end
>   
> +---
> +-- Transfer options from env_cfg to cfg.
> +--
> +local function apply_env_cfg(cfg, env_cfg)
> +    if env_cfg == nil then
> +        return
> +    end
> +    for k, v in pairs(env_cfg) do
> +        if cfg[k] == nil or os.getenv('TARANTOOLCTL') == 'true' then

2) Add a comment describing the condition `os.getenv('TARANTOOLCTL') == 
'true'`.

> +            cfg[k] = v
> +        elseif type(v) == 'table' then
> +            apply_env_cfg(cfg[k], v)
> +        end
> +    end
> +end
> +
>   local function apply_default_cfg(cfg, default_cfg, module_cfg)
>       for k,v in pairs(default_cfg) do
>           if cfg[k] == nil then
> @@ -549,7 +579,19 @@ local function compare_cfg(cfg1, cfg2)
>       return true
>   end
>   
> +---
> +-- Nullify option values from environment in old configuration.
> +--
> +local function reset_env_options(oldcfg)

3) Why should we nullify option values from environment in old
configuration?

> +    for k, v in pairs(env_cfg) do
> +        if user_cfg[k] == nil then
> +            oldcfg[k] = nil
> +        end
> +    end
> +end
> +
>   local function reload_cfg(oldcfg, cfg)
> +    reset_env_options(oldcfg)
>       cfg = upgrade_cfg(cfg, translate_cfg)
>       local newcfg = prepare_cfg(cfg, default_cfg, template_cfg,
>                                  module_cfg, modify_cfg)
> @@ -616,6 +658,104 @@ setmetatable(box, {
>        end
>   })
>   
> +---
> +-- Erase quotes at the begin and the end of string.
> +--
> +local function dequote(str)
> +    if string.sub(str, 1, 1) == '\'' and string.sub(str, -1) == '\'' or
> +       string.sub(str, 1, 1) == '"' and string.sub(str, -1) == '"' then
> +        return string.sub(str, 2, -2)
> +    end
> +    return str
> +end
> +
> +---
> +-- Convert raw option value becoming from the corresponding
> +-- environment variable represented as string to string, number or
> +-- boolean (depending on option).
> +--
> +local function process_option_value(option, raw_value)
> +    local param_type = template_cfg[option]
> +
> +    -- May be peculiar to "feedback*" parameters.
> +    if param_type == nil then
> +        return nil
> +    end
> +
> +    local value = nil
> +    local err_msg_fmt = 'Environment variable TT_%s has '..

4) Add a space before `..`.

> +        'incorrect value for option \'%s\': should be %s'
> +
> +    -- "log_level" option should be a number.
> +    if param_type == 'number' or option == 'log_level' then

5) As for me it's strange, that in 
a94a9b3fd629b5abbc99a23ba527cd91e5c6ebf5 Cyrill Gorcunov
moves the type check to a module, but you (in fact) return it back
(determine the type by the parameter name). I think you need to discuss
this question with Cyrill and check types either inside the module or in 
the `load_cfg.lua`.

> +        value = tonumber(raw_value)
> +        if value == nil then
> +            error(err_msg_fmt:format(string.upper(option), option,
> +                    "convertible to number"))
> +        end
> +    -- "log_nonblock" option should be boolean.
> +    elseif param_type == 'boolean' or option == 'log_nonblock' then
> +        if raw_value == 'true' then
> +            value = true
> +        elseif raw_value == 'false' then
> +            value = false
> +        else
> +            error(err_msg_fmt:format(string.upper(option), option,
> +                    "'true' or 'false'"))
> +        end
> +    elseif param_type == 'string, number' then

6) Since some options can have several types, I think it's more
comfortable for such options to use a table listing all possible types.
But such refactoring doesn't seem to be mandatory for the patch.

> +        -- Firstly, try to convert to number.
> +        value = tonumber(raw_value)
> +        if value == nil then
> +            value = raw_value
> +        end
> +    elseif param_type == 'string, number, table' then
> +        if string.sub(raw_value, 1, 1) == '{' and
> +           string.sub(raw_value, -1) == '}' then
> +            value = string.sub(raw_value, 2, -2)
> +            value = require('csv').load(value)[1]
> +            for i, item in pairs(value) do
> +                value[i] = dequote(item)

7) In this case, the number inside the table will be parsed as a
"string". We don't want that.

> +            end
> +        else
> +            value = tonumber(raw_value)
> +            if value == nil then
> +                value = raw_value
> +            end
> +        end
> +    else
> +        value = raw_value
> +    end
> +
> +    return value
> +end
> +
> +-- local function collect_env_cfg()
> +--     local env_cfg = {}
> +--     for option, _ in pairs(template_cfg) do
> +--         if cfg[option] == nil or os.getenv('TARANTOOLCTL') == 'true' then
> +--             local env_var_name = 'TT_'..string.upper(option) > +--             local raw_value = os.getenv(env_var_name)
> +--             if raw_value ~= nil then
> +--                 cfg[option] = process_option_value(option, raw_value)
> +--             end
> +--         end
> +--     end
> +-- end

8) Don't leave commented code after work.

> +
> +---
> +-- Pull and process configuration values from environment.
> +--
> +local function collect_env_cfg()
> +    for option, _ in pairs(template_cfg) do
> +        local env_var_name = 'TT_'..string.upper(option)

9) Add spaces around the `..`

> +        local raw_value = os.getenv(env_var_name)
> +        if raw_value ~= nil then
> +            env_cfg[option] = process_option_value(option, raw_value)
> +        end
> +    end
> +end
> +
>   -- Whether box is loaded.
>   --
>   -- `false` when box is not configured or when the initialization
> @@ -627,6 +767,7 @@ setmetatable(box, {
>   local box_is_configured = false
>   
>   local function load_cfg(cfg)
> +    user_cfg = cfg
>       -- A user may save box.cfg (this function) before box loading
>       -- and call it afterwards. We should reconfigure box in the
>       -- case.
> @@ -638,6 +779,8 @@ local function load_cfg(cfg)
>       cfg = upgrade_cfg(cfg, translate_cfg)
>       cfg = prepare_cfg(cfg, default_cfg, template_cfg,
>                         module_cfg, modify_cfg)
> +    collect_env_cfg()
> +    apply_env_cfg(cfg, env_cfg)
>       apply_default_cfg(cfg, default_cfg, module_cfg);
>       -- Save new box.cfg
>       box.cfg = cfg
> diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
> index 3c3023d34..060316714 100755
> --- a/test/app-tap/tarantoolctl.test.lua
> +++ b/test/app-tap/tarantoolctl.test.lua
> @@ -160,7 +160,7 @@ local function merge(...)
>   end
>   
>   local test = tap.test('tarantoolctl')
> -test:plan(8)
> +test:plan(9)
>   
>   -- basic start/stop test
>   -- must be stopped afterwards
> @@ -632,4 +632,35 @@ test:test('filter_xlog', function(test)
>       end
>   end)
>   
> +-- gh-5602: Check that environment cfg variables are more
> +-- prioritized than tarantoolctl cfg.
> +do
> +    local dir = fio.tempdir()
> +    local code = [[ box.cfg ]]
> +    create_script(dir, 'show_cfg.lua', code)
> +    local code = [[ box.cfg{checkpoint_interval=3302, readahead=10001} ]]
> +    create_script(dir, 'cfg_script.lua', code)
> +
> +    local status, err = pcall(function()
> +        test:test("check answers in case of call", function(test_i)
> +            test_i:plan(4)
> +            os.setenv('TT_CHECKPOINT_INTERVAL', '3301')
> +            os.setenv('TT_READAHEAD', '10000')
> +            check_ok(test_i, dir, 'start', 'cfg_script', 0)
> +            tctl_wait_start(dir, 'cfg_script')
> +            check_ok(test_i, dir, 'eval',  'cfg_script show_cfg.lua', 0,
> +                     '---\n- 3301\n- 10000\n...', nil)
> +            check_ok(test_i, dir, 'stop', 'cfg_script', 0)
> +        end)
> +    end)
> +
> +    cleanup_instance(dir, 'cfg_script')
> +    recursive_rmdir(dir)
> +
> +    if status == false then
> +        print(("Error: %s"):format(err))
> +        os.exit()
> +    end
> +end
> +
>   os.exit(test:check() == true and 0 or -1)
> diff --git a/test/box-tap/gh-5602-environment-vars-cfg.result b/test/box-tap/gh-5602-environment-vars-cfg.result
> new file mode 100644
> index 000000000..c90dd301b
> --- /dev/null
> +++ b/test/box-tap/gh-5602-environment-vars-cfg.result
> @@ -0,0 +1,41 @@
> +TAP version 13
> +1..5
> +ok - box.cfg is successful
> +ok - listen
> +ok - readahead
> +ok - strip_core
> +ok - log_format is not set
> +TAP version 13
> +1..7
> +ok - box.cfg is successful
> +ok - listen
> +ok - replication is table
> +ok - replication URI 1
> +ok - replication URI 2
> +ok - replication_connect_timeout
> +ok - replication_synchro_quorum
> +TAP version 13
> +1..3
> +ok - box.cfg is successful
> +ok - box.cfg{} background value is prioritized
> +ok - box.cfg{} vinyl_timeout value is prioritized
> +TAP version 13
> +1..2
> +ok - box.cfg is not successful
> +ok - bad sql_cache_size value
> +TAP version 13
> +1..2
> +ok - box.cfg is not successful
> +ok - bad strip_core value
> +TAP version 13
> +1..2
> +ok - box.cfg is not successful
> +ok - bad replication value
> +TAP version 13
> +1..6
> +ok - set 1
> +ok - set 2
> +ok - set 3
> +ok - set 4
> +ok - set 5
> +ok - set 6
> diff --git a/test/box-tap/gh-5602-environment-vars-cfg.test.lua b/test/box-tap/gh-5602-environment-vars-cfg.test.lua
> new file mode 100755
> index 000000000..3d19fdc90
> --- /dev/null
> +++ b/test/box-tap/gh-5602-environment-vars-cfg.test.lua
> @@ -0,0 +1,57 @@
> +#!/usr/bin/env tarantool
> +
> +local os = require('os')
> +local fio = require('fio')
> +local tap = require('tap')
> +
> +local test = tap.test('gh-5602')
> +
> +-- gh-5602: Check that environment cfg variables working.
> +local TARANTOOL_PATH = arg[-1]
> +local script_name = 'gh-5602-test-cases-script.lua'
> +local path_to_script = fio.pathjoin(
> +        os.getenv('PWD'),
> +        'box-tap',
> +        script_name)
> +
> +---
> +-- Generate shell command where set is sequence of environment
> +-- variables, then path to tarantool, the first arg is path to
> +-- test script and the second one is number of set.
> +--
> +local function shell_command(set, i)
> +    return ('%s %s %s %d'):format(
> +        set,
> +        TARANTOOL_PATH,
> +        path_to_script,
> +        i)
> +end
> +
> +local set_1 = ('%s %s %s %s'):format('TT_LISTEN=3301', 'TT_READAHEAD=10000',
> +    'TT_STRIP_CORE=false', 'TT_LOG_FORMAT=json')
> +local set_2 = ('%s %s %s %s'):format('TT_LISTEN=3301',
> +    'TT_REPLICATION=\"{\'0.0.0.0:12345\', \'1.1.1.1:12345\'}\"',
> +    'TT_REPLICATION_CONNECT_TIMEOUT=0.01',
> +    'TT_REPLICATION_SYNCHRO_QUORUM=\'4 + 1\'')
> +local set_3 = 'TT_BACKGROUND=true TT_VINYL_TIMEOUT=60.1'
> +local set_4 = 'TT_SQL_CACHE_SIZE=a'
> +local set_5 = 'TT_STRIP_CORE=a'
> +local set_6 = 'TT_REPLICATION=\"{0.0.0.0:12345\', \'1.1.1.1:12345\'}\"'
> +
> +local sets = {}
> +sets[1] = set_1
> +sets[2] = set_2
> +sets[3] = set_3
> +sets[4] = set_4
> +sets[5] = set_5
> +sets[6] = set_6
> +
> +test:plan(#sets)
> +for i, set in ipairs(sets) do
> +    local tmpdir = fio.tempdir()
> +    local new_path = fio.pathjoin(tmpdir, script_name)
> +    fio.copyfile(path_to_script, new_path)
> +    test:is(os.execute(shell_command(set, i)), 0, 'set '..i)
> +end
> +
> +os.exit(test:check() and 0 or 1)
> diff --git a/test/box-tap/gh-5602-test-cases-script.lua b/test/box-tap/gh-5602-test-cases-script.lua
> new file mode 100755
> index 000000000..a8b2fc987
> --- /dev/null
> +++ b/test/box-tap/gh-5602-test-cases-script.lua
> @@ -0,0 +1,69 @@
> +local tap = require('tap')
> +
> +local test = tap.test('gh-5602')
> +
> +local status, err = pcall(box.cfg, {background = false, vinyl_timeout = 70.1})
> +
> +-- Check that environment cfg values are set correctly.
> +if arg[1] == '1' then
> +    test:plan(5)
> +    test:ok(status ~= false, 'box.cfg is successful')
> +    test:is(box.cfg['listen'], 3301, 'listen')
> +    test:is(box.cfg['readahead'], 10000, 'readahead')
> +    test:is(box.cfg['strip_core'], false, 'strip_core')
> +    test:is(box.cfg['log_format'], 'json', 'log_format is not set')
> +end
> +if arg[1] == '2' then
> +    test:plan(7)
> +    test:ok(status ~= false, 'box.cfg is successful')
> +    test:is(box.cfg['listen'], 3301, 'listen')
> +    local replication = box.cfg['replication']
> +    test:is(type(replication), 'table', 'replication is table')
> +    test:ok(replication[1] == '0.0.0.0:12345' or
> +            replication[1] == '1.1.1.1:12345', 'replication URI 1')
> +    test:ok(replication[2] == '0.0.0.0:12345' or
> +            replication[2] == '1.1.1.1:12345', 'replication URI 2')
> +    test:is(box.cfg['replication_connect_timeout'], 0.01,
> +            'replication_connect_timeout')
> +    test:is(box.cfg['replication_synchro_quorum'], '4 + 1',
> +            'replication_synchro_quorum')
> +end
> +
> +-- Check that box.cfg{} values are more prioritized than
> +-- environment cfg values.
> +if arg[1] == '3' then
> +    test:plan(3)
> +    test:ok(status ~= false, 'box.cfg is successful')
> +    test:is(box.cfg['background'], false,
> +            'box.cfg{} background value is prioritized')
> +    test:is(box.cfg['vinyl_timeout'], 70.1,
> +            'box.cfg{} vinyl_timeout value is prioritized')
> +end
> +
> +local err_msg_fmt = 'Environment variable TT_%s has '..
> +    'incorrect value for option \'%s\': should be %s'
> +
> +-- Check bad environment cfg values.
> +if arg[1] == '4' then
> +    test:plan(2)
> +    test:ok(status == false, 'box.cfg is not successful')
> +    local index = string.find(err, err_msg_fmt:format('SQL_CACHE_SIZE',
> +        'sql_cache_size', 'convertible to number'))
> +    test:ok(index ~= nil, 'bad sql_cache_size value')
> +end
> +if arg[1] == '5' then
> +    test:plan(2)
> +    test:ok(status == false, 'box.cfg is not successful')
> +    local index = string.find(err, err_msg_fmt:format('STRIP_CORE',
> +        'strip_core', '\'true\' or \'false\''))
> +    test:ok(index ~= nil, 'bad strip_core value')
> +end
> +if arg[1] == '6' then
> +    test:plan(2)
> +    test:ok(status == false, 'box.cfg is not successful')
> +    local index = string.find(tostring(err), 'Incorrect value for option '..
> +        '\'replication\': expected host:service or /unix.socket')
> +    test:ok(index ~= nil, 'bad replication value')
> +end
> +
> +os.exit(test:check() and 0 or 1)
> 

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH] box: set cfg via environment variables
  2021-04-09  8:56   ` Roman Khabibov via Tarantool-patches
  2021-04-12  8:37     ` Leonid Vasiliev via Tarantool-patches
@ 2021-04-12  9:52     ` Leonid Vasiliev via Tarantool-patches
  1 sibling, 0 replies; 7+ messages in thread
From: Leonid Vasiliev via Tarantool-patches @ 2021-04-12  9:52 UTC (permalink / raw)
  To: Roman Khabibov; +Cc: tarantool-patches

The "luachek" test failed.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [Tarantool-patches] [PATCH] box: set cfg via environment variables
  2021-04-12  8:37     ` Leonid Vasiliev via Tarantool-patches
@ 2021-04-13 12:47       ` Roman Khabibov via Tarantool-patches
  0 siblings, 0 replies; 7+ messages in thread
From: Roman Khabibov via Tarantool-patches @ 2021-04-13 12:47 UTC (permalink / raw)
  To: Leonid Vasiliev; +Cc: tarantool-patches

Hi! Thanks for the review.

> On Apr 12, 2021, at 11:37, Leonid Vasiliev <lvasiliev@tarantool.org> wrote:
> 
> Hi! Thank you for the patch.
> See 9 comments below:
> 
>> commit ee6525f843a347c1df8a92a015414f4cbcceafb9
>> Author: Roman Khabibov <roman.habibov@tarantool.org>
>> Date:   Tue Mar 9 16:47:11 2021 +0300
>>     box: set cfg via environment variables
>>          Add ability to set box.cfg options via env variables. These
>>     variables should have name "TT_<OPTION>". When Tarantool instance
>>     is started under tarantoolctl utility env variables options have
>>     more priority than instance file's cfg.
>>          Closes #5602
>>          @TarantoolBot document
>>     Title: Set box.cfg options via env variables
>>          Now, it is possible to set box.cfg options via environment
>>     variables. The name of variable should correspond the following
>>     pattern: "TT_<NAME>", where <NAME> is an option name. For example:
>>     "TT_LISTEN", "TT_READAHEAD".
>>     If it is complex value, for example, "replication" can be set by a
>>     table, the corresponding environment variable value has the syntax
>>     of lua array (without keys):
>>          `export TT_REPLICATION="{'uri1', 'uri2'}"`
>> diff --git a/changelogs/unreleased/environment-cfg.md b/changelogs/unreleased/environment-cfg.md
>> new file mode 100755
>> index 000000000..c5d5005ac
>> --- /dev/null
>> +++ b/changelogs/unreleased/environment-cfg.md
>> @@ -0,0 +1,5 @@
>> +## feature/core
>> +
>> +* Now, it is possibly to set box.cfg options with environment variables.
>> +The priority of sources of configuration options is the following (from low
>> +to high): default, tarantoolctl, environment, box.cfg{}. (gh-5602).
>> \ No newline at end of file
>> diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
>> index 574c8bef4..a1439ae46 100644
>> --- a/src/box/lua/load_cfg.lua
>> +++ b/src/box/lua/load_cfg.lua
>> @@ -416,6 +416,20 @@ local translate_cfg = {
>>      end},
>>  }
>>  +-- Configuration options from environment is taken into account
>> +-- only at the first box.cfg{} call. When box.cfg is reload, they
>> +-- are ignored.
>> +--
>> +-- The priority of sources of configuration options is the
>> +-- following (from low to high): default, tarantoolctl,
>> +-- environment, box.cfg{}.
>> +local env_cfg = {}
>> +-- This is auxiliary table and it needed to keep values which
>> +-- passed or typed to box.cfg{} at the first call. Then we
>> +-- distinguish values (it was from environment or not) and decide
>> +-- ignore it or not during reload.
>> +local user_cfg = {}
> 
> 1) AFAIU we decided what it's not needed. In my mind, on reload, we have
> no difference between the options that were set using environment and any
> other way.
Removed.

>> +
>>  -- Upgrade old config
>>  local function upgrade_cfg(cfg, translate_cfg)
>>      if cfg == nil then
>> @@ -515,6 +529,22 @@ local function prepare_cfg(cfg, default_cfg, template_cfg,
>>      return new_cfg
>>  end
>>  +---
>> +-- Transfer options from env_cfg to cfg.
>> +--
>> +local function apply_env_cfg(cfg, env_cfg)
>> +    if env_cfg == nil then
>> +        return
>> +    end
>> +    for k, v in pairs(env_cfg) do
>> +        if cfg[k] == nil or os.getenv('TARANTOOLCTL') == 'true' then
> 
> 2) Add a comment describing the condition `os.getenv('TARANTOOLCTL') == 'true'`.
+---
+-- Transfer options from env_cfg to cfg.
+--
+local function apply_env_cfg(cfg, env_cfg)
+    if env_cfg == nil then
+        return
+    end
+
+    for k, v in pairs(env_cfg) do
+        -- If Tarantool started with tarantoolctl then we have to
+        -- ignore configuration from tarantoolctl, because they
+        -- has priority lower than environment variables.
+        if cfg[k] == nil or os.getenv('TARANTOOLCTL') == 'true' then
+            cfg[k] = v
+        elseif type(v) == 'table' then
+            apply_env_cfg(cfg[k], v)
+        end
+    end
+end
+

>> +            cfg[k] = v
>> +        elseif type(v) == 'table' then
>> +            apply_env_cfg(cfg[k], v)
>> +        end
>> +    end
>> +end
>> +
>>  local function apply_default_cfg(cfg, default_cfg, module_cfg)
>>      for k,v in pairs(default_cfg) do
>>          if cfg[k] == nil then
>> @@ -549,7 +579,19 @@ local function compare_cfg(cfg1, cfg2)
>>      return true
>>  end
>>  +---
>> +-- Nullify option values from environment in old configuration.
>> +--
>> +local function reset_env_options(oldcfg)
> 
> 3) Why should we nullify option values from environment in old
> configuration?
> 
>> +    for k, v in pairs(env_cfg) do
>> +        if user_cfg[k] == nil then
>> +            oldcfg[k] = nil
>> +        end
>> +    end
>> +end
>> +
>>  local function reload_cfg(oldcfg, cfg)
>> +    reset_env_options(oldcfg)
>>      cfg = upgrade_cfg(cfg, translate_cfg)
>>      local newcfg = prepare_cfg(cfg, default_cfg, template_cfg,
>>                                 module_cfg, modify_cfg)
>> @@ -616,6 +658,104 @@ setmetatable(box, {
>>       end
>>  })
>>  +---
>> +-- Erase quotes at the begin and the end of string.
>> +--
>> +local function dequote(str)
>> +    if string.sub(str, 1, 1) == '\'' and string.sub(str, -1) == '\'' or
>> +       string.sub(str, 1, 1) == '"' and string.sub(str, -1) == '"' then
>> +        return string.sub(str, 2, -2)
>> +    end
>> +    return str
>> +end
>> +
>> +---
>> +-- Convert raw option value becoming from the corresponding
>> +-- environment variable represented as string to string, number or
>> +-- boolean (depending on option).
>> +--
>> +local function process_option_value(option, raw_value)
>> +    local param_type = template_cfg[option]
>> +
>> +    -- May be peculiar to "feedback*" parameters.
>> +    if param_type == nil then
>> +        return nil
>> +    end
>> +
>> +    local value = nil
>> +    local err_msg_fmt = 'Environment variable TT_%s has '..
> 
> 4) Add a space before `..`.
Done.

>> +        'incorrect value for option \'%s\': should be %s'
>> +
>> +    -- "log_level" option should be a number.
>> +    if param_type == 'number' or option == 'log_level' then
> 
> 5) As for me it's strange, that in a94a9b3fd629b5abbc99a23ba527cd91e5c6ebf5 Cyrill Gorcunov
> moves the type check to a module, but you (in fact) return it back
> (determine the type by the parameter name). I think you need to discuss
> this question with Cyrill and check types either inside the module or in the `load_cfg.lua`.
Done. See the first patch.

>> +        value = tonumber(raw_value)
>> +        if value == nil then
>> +            error(err_msg_fmt:format(string.upper(option), option,
>> +                    "convertible to number"))
>> +        end
>> +    -- "log_nonblock" option should be boolean.
>> +    elseif param_type == 'boolean' or option == 'log_nonblock' then
>> +        if raw_value == 'true' then
>> +            value = true
>> +        elseif raw_value == 'false' then
>> +            value = false
>> +        else
>> +            error(err_msg_fmt:format(string.upper(option), option,
>> +                    "'true' or 'false'"))
>> +        end
>> +    elseif param_type == 'string, number' then
> 
> 6) Since some options can have several types, I think it's more
> comfortable for such options to use a table listing all possible types.
> But such refactoring doesn't seem to be mandatory for the patch.
> 
>> +        -- Firstly, try to convert to number.
>> +        value = tonumber(raw_value)
>> +        if value == nil then
>> +            value = raw_value
>> +        end
>> +    elseif param_type == 'string, number, table' then
>> +        if string.sub(raw_value, 1, 1) == '{' and
>> +           string.sub(raw_value, -1) == '}' then
>> +            value = string.sub(raw_value, 2, -2)
>> +            value = require('csv').load(value)[1]
>> +            for i, item in pairs(value) do
>> +                value[i] = dequote(item)
> 
> 7) In this case, the number inside the table will be parsed as a
> "string". We don't want that.
+---
+-- Convert raw option value becoming from the corresponding
+-- environment variable represented as string to string, number or
+-- boolean (depending on option).
+--
+local function process_option_value(option, raw_value)
+    local param_type = template_cfg[option]
+
+    -- May be peculiar to "feedback*" parameters.
+    if param_type == nil then
+        return nil
+    end
+
+    local value = nil
+    local err_msg_fmt = 'Environment variable TT_%s has ' ..
+        'incorrect value for option \'%s\': should be %s'
+
+    -- "log_level" option should be a number.
+    if param_type == 'number' then
+        value = tonumber(raw_value)
+        if value == nil then
+            error(err_msg_fmt:format(string.upper(option), option,
+                    "convertible to number"))
+        end
+    -- "log_nonblock" option should be boolean.
+    elseif param_type == 'boolean' then
+        if raw_value == 'true' then
+            value = true
+        elseif raw_value == 'false' then
+            value = false
+        else
+            error(err_msg_fmt:format(string.upper(option), option,
+                    "'true' or 'false'"))
+        end
+    elseif param_type == 'string, number' then
+        -- Firstly, try to convert to number.
+        value = tonumber(raw_value)
+        if value == nil then
+            value = raw_value
+        end
+    elseif param_type == 'string, number, table' then
+        if string.sub(raw_value, 1, 1) == '{' and
+           string.sub(raw_value, -1) == '}' then
+            value = string.sub(raw_value, 2, -2)
+            value = require('csv').load(value)[1]
+            for i, item in pairs(value) do
+                item = dequote(item)
+                value[i] = tonumber(item)
+                if value[i] == nil then
+                    value[i] = item
+                end
+            end
+        else
+            value = tonumber(raw_value)
+            if value == nil then
+                value = raw_value
+            end
+        end
+    else
+        value = raw_value
+    end
+
+    return value
+end
+

>> +            end
>> +        else
>> +            value = tonumber(raw_value)
>> +            if value == nil then
>> +                value = raw_value
>> +            end
>> +        end
>> +    else
>> +        value = raw_value
>> +    end
>> +
>> +    return value
>> +end
>> +
>> +-- local function collect_env_cfg()
>> +--     local env_cfg = {}
>> +--     for option, _ in pairs(template_cfg) do
>> +--         if cfg[option] == nil or os.getenv('TARANTOOLCTL') == 'true' then
>> +--             local env_var_name = 'TT_'..string.upper(option) > +--             local raw_value = os.getenv(env_var_name)
>> +--             if raw_value ~= nil then
>> +--                 cfg[option] = process_option_value(option, raw_value)
>> +--             end
>> +--         end
>> +--     end
>> +-- end
> 
> 8) Don't leave commented code after work.
Removed.

>> +
>> +---
>> +-- Pull and process configuration values from environment.
>> +--
>> +local function collect_env_cfg()
>> +    for option, _ in pairs(template_cfg) do
>> +        local env_var_name = 'TT_'..string.upper(option)
> 
> 9) Add spaces around the `..`
Done.

>> +        local raw_value = os.getenv(env_var_name)
>> +        if raw_value ~= nil then
>> +            env_cfg[option] = process_option_value(option, raw_value)
>> +        end
>> +    end
>> +end
>> +
>>  -- Whether box is loaded.
>>  --
>>  -- `false` when box is not configured or when the initialization
>> @@ -627,6 +767,7 @@ setmetatable(box, {
>>  local box_is_configured = false
>>    local function load_cfg(cfg)
>> +    user_cfg = cfg
>>      -- A user may save box.cfg (this function) before box loading
>>      -- and call it afterwards. We should reconfigure box in the
>>      -- case.
>> @@ -638,6 +779,8 @@ local function load_cfg(cfg)
>>      cfg = upgrade_cfg(cfg, translate_cfg)
>>      cfg = prepare_cfg(cfg, default_cfg, template_cfg,
>>                        module_cfg, modify_cfg)
>> +    collect_env_cfg()
>> +    apply_env_cfg(cfg, env_cfg)
>>      apply_default_cfg(cfg, default_cfg, module_cfg);
>>      -- Save new box.cfg
>>      box.cfg = cfg
>> diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
>> index 3c3023d34..060316714 100755
>> --- a/test/app-tap/tarantoolctl.test.lua
>> +++ b/test/app-tap/tarantoolctl.test.lua
>> @@ -160,7 +160,7 @@ local function merge(...)
>>  end
>>    local test = tap.test('tarantoolctl')
>> -test:plan(8)
>> +test:plan(9)
>>    -- basic start/stop test
>>  -- must be stopped afterwards
>> @@ -632,4 +632,35 @@ test:test('filter_xlog', function(test)
>>      end
>>  end)
>>  +-- gh-5602: Check that environment cfg variables are more
>> +-- prioritized than tarantoolctl cfg.
>> +do
>> +    local dir = fio.tempdir()
>> +    local code = [[ box.cfg ]]
>> +    create_script(dir, 'show_cfg.lua', code)
>> +    local code = [[ box.cfg{checkpoint_interval=3302, readahead=10001} ]]
>> +    create_script(dir, 'cfg_script.lua', code)
>> +
>> +    local status, err = pcall(function()
>> +        test:test("check answers in case of call", function(test_i)
>> +            test_i:plan(4)
>> +            os.setenv('TT_CHECKPOINT_INTERVAL', '3301')
>> +            os.setenv('TT_READAHEAD', '10000')
>> +            check_ok(test_i, dir, 'start', 'cfg_script', 0)
>> +            tctl_wait_start(dir, 'cfg_script')
>> +            check_ok(test_i, dir, 'eval',  'cfg_script show_cfg.lua', 0,
>> +                     '---\n- 3301\n- 10000\n...', nil)
>> +            check_ok(test_i, dir, 'stop', 'cfg_script', 0)
>> +        end)
>> +    end)
>> +
>> +    cleanup_instance(dir, 'cfg_script')
>> +    recursive_rmdir(dir)
>> +
>> +    if status == false then
>> +        print(("Error: %s"):format(err))
>> +        os.exit()
>> +    end
>> +end
>> +
>>  os.exit(test:check() == true and 0 or -1)
>> diff --git a/test/box-tap/gh-5602-environment-vars-cfg.result b/test/box-tap/gh-5602-environment-vars-cfg.result
>> new file mode 100644
>> index 000000000..c90dd301b
>> --- /dev/null
>> +++ b/test/box-tap/gh-5602-environment-vars-cfg.result
>> @@ -0,0 +1,41 @@
>> +TAP version 13
>> +1..5
>> +ok - box.cfg is successful
>> +ok - listen
>> +ok - readahead
>> +ok - strip_core
>> +ok - log_format is not set
>> +TAP version 13
>> +1..7
>> +ok - box.cfg is successful
>> +ok - listen
>> +ok - replication is table
>> +ok - replication URI 1
>> +ok - replication URI 2
>> +ok - replication_connect_timeout
>> +ok - replication_synchro_quorum
>> +TAP version 13
>> +1..3
>> +ok - box.cfg is successful
>> +ok - box.cfg{} background value is prioritized
>> +ok - box.cfg{} vinyl_timeout value is prioritized
>> +TAP version 13
>> +1..2
>> +ok - box.cfg is not successful
>> +ok - bad sql_cache_size value
>> +TAP version 13
>> +1..2
>> +ok - box.cfg is not successful
>> +ok - bad strip_core value
>> +TAP version 13
>> +1..2
>> +ok - box.cfg is not successful
>> +ok - bad replication value
>> +TAP version 13
>> +1..6
>> +ok - set 1
>> +ok - set 2
>> +ok - set 3
>> +ok - set 4
>> +ok - set 5
>> +ok - set 6
>> diff --git a/test/box-tap/gh-5602-environment-vars-cfg.test.lua b/test/box-tap/gh-5602-environment-vars-cfg.test.lua
>> new file mode 100755
>> index 000000000..3d19fdc90
>> --- /dev/null
>> +++ b/test/box-tap/gh-5602-environment-vars-cfg.test.lua
>> @@ -0,0 +1,57 @@
>> +#!/usr/bin/env tarantool
>> +
>> +local os = require('os')
>> +local fio = require('fio')
>> +local tap = require('tap')
>> +
>> +local test = tap.test('gh-5602')
>> +
>> +-- gh-5602: Check that environment cfg variables working.
>> +local TARANTOOL_PATH = arg[-1]
>> +local script_name = 'gh-5602-test-cases-script.lua'
>> +local path_to_script = fio.pathjoin(
>> +        os.getenv('PWD'),
>> +        'box-tap',
>> +        script_name)
>> +
>> +---
>> +-- Generate shell command where set is sequence of environment
>> +-- variables, then path to tarantool, the first arg is path to
>> +-- test script and the second one is number of set.
>> +--
>> +local function shell_command(set, i)
>> +    return ('%s %s %s %d'):format(
>> +        set,
>> +        TARANTOOL_PATH,
>> +        path_to_script,
>> +        i)
>> +end
>> +
>> +local set_1 = ('%s %s %s %s'):format('TT_LISTEN=3301', 'TT_READAHEAD=10000',
>> +    'TT_STRIP_CORE=false', 'TT_LOG_FORMAT=json')
>> +local set_2 = ('%s %s %s %s'):format('TT_LISTEN=3301',
>> +    'TT_REPLICATION=\"{\'0.0.0.0:12345\', \'1.1.1.1:12345\'}\"',
>> +    'TT_REPLICATION_CONNECT_TIMEOUT=0.01',
>> +    'TT_REPLICATION_SYNCHRO_QUORUM=\'4 + 1\'')
>> +local set_3 = 'TT_BACKGROUND=true TT_VINYL_TIMEOUT=60.1'
>> +local set_4 = 'TT_SQL_CACHE_SIZE=a'
>> +local set_5 = 'TT_STRIP_CORE=a'
>> +local set_6 = 'TT_REPLICATION=\"{0.0.0.0:12345\', \'1.1.1.1:12345\'}\"'
>> +
>> +local sets = {}
>> +sets[1] = set_1
>> +sets[2] = set_2
>> +sets[3] = set_3
>> +sets[4] = set_4
>> +sets[5] = set_5
>> +sets[6] = set_6
>> +
>> +test:plan(#sets)
>> +for i, set in ipairs(sets) do
>> +    local tmpdir = fio.tempdir()
>> +    local new_path = fio.pathjoin(tmpdir, script_name)
>> +    fio.copyfile(path_to_script, new_path)
>> +    test:is(os.execute(shell_command(set, i)), 0, 'set '..i)
>> +end
>> +
>> +os.exit(test:check() and 0 or 1)
>> diff --git a/test/box-tap/gh-5602-test-cases-script.lua b/test/box-tap/gh-5602-test-cases-script.lua
>> new file mode 100755
>> index 000000000..a8b2fc987
>> --- /dev/null
>> +++ b/test/box-tap/gh-5602-test-cases-script.lua
>> @@ -0,0 +1,69 @@
>> +local tap = require('tap')
>> +
>> +local test = tap.test('gh-5602')
>> +
>> +local status, err = pcall(box.cfg, {background = false, vinyl_timeout = 70.1})
>> +
>> +-- Check that environment cfg values are set correctly.
>> +if arg[1] == '1' then
>> +    test:plan(5)
>> +    test:ok(status ~= false, 'box.cfg is successful')
>> +    test:is(box.cfg['listen'], 3301, 'listen')
>> +    test:is(box.cfg['readahead'], 10000, 'readahead')
>> +    test:is(box.cfg['strip_core'], false, 'strip_core')
>> +    test:is(box.cfg['log_format'], 'json', 'log_format is not set')
>> +end
>> +if arg[1] == '2' then
>> +    test:plan(7)
>> +    test:ok(status ~= false, 'box.cfg is successful')
>> +    test:is(box.cfg['listen'], 3301, 'listen')
>> +    local replication = box.cfg['replication']
>> +    test:is(type(replication), 'table', 'replication is table')
>> +    test:ok(replication[1] == '0.0.0.0:12345' or
>> +            replication[1] == '1.1.1.1:12345', 'replication URI 1')
>> +    test:ok(replication[2] == '0.0.0.0:12345' or
>> +            replication[2] == '1.1.1.1:12345', 'replication URI 2')
>> +    test:is(box.cfg['replication_connect_timeout'], 0.01,
>> +            'replication_connect_timeout')
>> +    test:is(box.cfg['replication_synchro_quorum'], '4 + 1',
>> +            'replication_synchro_quorum')
>> +end
>> +
>> +-- Check that box.cfg{} values are more prioritized than
>> +-- environment cfg values.
>> +if arg[1] == '3' then
>> +    test:plan(3)
>> +    test:ok(status ~= false, 'box.cfg is successful')
>> +    test:is(box.cfg['background'], false,
>> +            'box.cfg{} background value is prioritized')
>> +    test:is(box.cfg['vinyl_timeout'], 70.1,
>> +            'box.cfg{} vinyl_timeout value is prioritized')
>> +end
>> +
>> +local err_msg_fmt = 'Environment variable TT_%s has '..
>> +    'incorrect value for option \'%s\': should be %s'
>> +
>> +-- Check bad environment cfg values.
>> +if arg[1] == '4' then
>> +    test:plan(2)
>> +    test:ok(status == false, 'box.cfg is not successful')
>> +    local index = string.find(err, err_msg_fmt:format('SQL_CACHE_SIZE',
>> +        'sql_cache_size', 'convertible to number'))
>> +    test:ok(index ~= nil, 'bad sql_cache_size value')
>> +end
>> +if arg[1] == '5' then
>> +    test:plan(2)
>> +    test:ok(status == false, 'box.cfg is not successful')
>> +    local index = string.find(err, err_msg_fmt:format('STRIP_CORE',
>> +        'strip_core', '\'true\' or \'false\''))
>> +    test:ok(index ~= nil, 'bad strip_core value')
>> +end
>> +if arg[1] == '6' then
>> +    test:plan(2)
>> +    test:ok(status == false, 'box.cfg is not successful')
>> +    local index = string.find(tostring(err), 'Incorrect value for option '..
>> +        '\'replication\': expected host:service or /unix.socket')
>> +    test:ok(index ~= nil, 'bad replication value')
>> +end
>> +
>> +os.exit(test:check() and 0 or 1)

box: set cfg via environment variables

Add ability to set box.cfg options via env variables. These
variables should have name "TT_<OPTION>". When Tarantool instance
is started under tarantoolctl utility env variables options have
more priority than instance file's cfg.

Closes #5602

@TarantoolBot document
Title: Set box.cfg options via env variables

Now, it is possible to set box.cfg options via environment
variables. The name of variable should correspond the following
pattern: "TT_<NAME>", where <NAME> is an option name. For example:
"TT_LISTEN", "TT_READAHEAD".
If it is complex value, for example, "replication" can be set by a
table, the corresponding environment variable value has the syntax
of lua array (without keys):

`export TT_REPLICATION="{'uri1', 'uri2'}"`
---
 changelogs/unreleased/environment-cfg.md      |   5 +
 src/box/lua/load_cfg.lua                      | 114 ++++++++++++++++++
 test/app-tap/tarantoolctl.test.lua            |  33 ++++-
 .../gh-5602-environment-vars-cfg.result       |  41 +++++++
 .../gh-5602-environment-vars-cfg.test.lua     |  57 +++++++++
 test/box-tap/gh-5602-test-cases-script.lua    |  69 +++++++++++
 6 files changed, 318 insertions(+), 1 deletion(-)
 create mode 100755 changelogs/unreleased/environment-cfg.md
 create mode 100644 test/box-tap/gh-5602-environment-vars-cfg.result
 create mode 100755 test/box-tap/gh-5602-environment-vars-cfg.test.lua
 create mode 100755 test/box-tap/gh-5602-test-cases-script.lua

diff --git a/changelogs/unreleased/environment-cfg.md b/changelogs/unreleased/environment-cfg.md
new file mode 100755
index 000000000..894c40bf6
--- /dev/null
+++ b/changelogs/unreleased/environment-cfg.md
@@ -0,0 +1,5 @@
+## feature/core
+
+* Now, it is possibly to set box.cfg options with environment variables.
+The priority of sources of configuration options is the following (from low
+to high): default, tarantoolctl, environment, box.cfg{}. (gh-5602).
diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index f90ba8a9a..df6a52e94 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -525,6 +525,26 @@ local function prepare_cfg(cfg, default_cfg, template_cfg,
     return new_cfg
 end
 
+---
+-- Transfer options from env_cfg to cfg.
+--
+local function apply_env_cfg(cfg, env_cfg)
+    if env_cfg == nil then
+        return
+    end
+
+    for k, v in pairs(env_cfg) do
+        -- If Tarantool started with tarantoolctl then we have to
+        -- ignore configuration from tarantoolctl, because they
+        -- has priority lower than environment variables.
+        if cfg[k] == nil or os.getenv('TARANTOOLCTL') == 'true' then
+            cfg[k] = v
+        elseif type(v) == 'table' then
+            apply_env_cfg(cfg[k], v)
+        end
+    end
+end
+
 local function apply_default_cfg(cfg, default_cfg, module_cfg)
     for k,v in pairs(default_cfg) do
         if cfg[k] == nil then
@@ -647,6 +667,98 @@ local function on_initial_config()
     end
 end
 
+---
+-- Erase quotes at the begin and the end of string.
+--
+local function dequote(str)
+    if string.sub(str, 1, 1) == '\'' and string.sub(str, -1) == '\'' or
+       string.sub(str, 1, 1) == '"' and string.sub(str, -1) == '"' then
+        return string.sub(str, 2, -2)
+    end
+    return str
+end
+
+---
+-- Convert raw option value becoming from the corresponding
+-- environment variable represented as string to string, number or
+-- boolean (depending on option).
+--
+local function process_option_value(option, raw_value)
+    local param_type = template_cfg[option]
+
+    -- May be peculiar to "feedback*" parameters.
+    if param_type == nil then
+        return nil
+    end
+
+    local value = nil
+    local err_msg_fmt = 'Environment variable TT_%s has ' ..
+        'incorrect value for option \'%s\': should be %s'
+
+    -- "log_level" option should be a number.
+    if param_type == 'number' then
+        value = tonumber(raw_value)
+        if value == nil then
+            error(err_msg_fmt:format(string.upper(option), option,
+                    "convertible to number"))
+        end
+    -- "log_nonblock" option should be boolean.
+    elseif param_type == 'boolean' then
+        if raw_value == 'true' then
+            value = true
+        elseif raw_value == 'false' then
+            value = false
+        else
+            error(err_msg_fmt:format(string.upper(option), option,
+                    "'true' or 'false'"))
+        end
+    elseif param_type == 'string, number' then
+        -- Firstly, try to convert to number.
+        value = tonumber(raw_value)
+        if value == nil then
+            value = raw_value
+        end
+    elseif param_type == 'string, number, table' then
+        if string.sub(raw_value, 1, 1) == '{' and
+           string.sub(raw_value, -1) == '}' then
+            value = string.sub(raw_value, 2, -2)
+            value = require('csv').load(value)[1]
+            for i, item in pairs(value) do
+                item = dequote(item)
+                value[i] = tonumber(item)
+                if value[i] == nil then
+                    value[i] = item
+                end
+            end
+        else
+            value = tonumber(raw_value)
+            if value == nil then
+                value = raw_value
+            end
+        end
+    else
+        value = raw_value
+    end
+
+    return value
+end
+
+---
+-- Pull and process configuration values from environment.
+--
+local function collect_env_cfg()
+    local env_cfg = {}
+    for option, _ in pairs(template_cfg) do
+        local env_var_name = 'TT_' .. string.upper(option)
+        local raw_value = os.getenv(env_var_name)
+        if raw_value ~= nil then
+            env_cfg[option] = process_option_value(option, raw_value)
+        end
+    end
+
+    return env_cfg
+end
+
 -- Whether box is loaded.
 --
 -- `false` when box is not configured or when the initialization
@@ -669,6 +781,8 @@ local function load_cfg(cfg)
     cfg = upgrade_cfg(cfg, translate_cfg)
     cfg = prepare_cfg(cfg, default_cfg, template_cfg,
                       module_cfg, modify_cfg)
+    local env_cfg = collect_env_cfg()
+    apply_env_cfg(cfg, env_cfg)
     apply_default_cfg(cfg, default_cfg, module_cfg);
     -- Save new box.cfg
     box.cfg = cfg
diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua
index 3c3023d34..060316714 100755
--- a/test/app-tap/tarantoolctl.test.lua
+++ b/test/app-tap/tarantoolctl.test.lua
@@ -160,7 +160,7 @@ local function merge(...)
 end
 
 local test = tap.test('tarantoolctl')
-test:plan(8)
+test:plan(9)
 
 -- basic start/stop test
 -- must be stopped afterwards
@@ -632,4 +632,35 @@ test:test('filter_xlog', function(test)
     end
 end)
 
+-- gh-5602: Check that environment cfg variables are more
+-- prioritized than tarantoolctl cfg.
+do
+    local dir = fio.tempdir()
+    local code = [[ box.cfg ]]
+    create_script(dir, 'show_cfg.lua', code)
+    local code = [[ box.cfg{checkpoint_interval=3302, readahead=10001} ]]
+    create_script(dir, 'cfg_script.lua', code)
+
+    local status, err = pcall(function()
+        test:test("check answers in case of call", function(test_i)
+            test_i:plan(4)
+            os.setenv('TT_CHECKPOINT_INTERVAL', '3301')
+            os.setenv('TT_READAHEAD', '10000')
+            check_ok(test_i, dir, 'start', 'cfg_script', 0)
+            tctl_wait_start(dir, 'cfg_script')
+            check_ok(test_i, dir, 'eval',  'cfg_script show_cfg.lua', 0,
+                     '---\n- 3301\n- 10000\n...', nil)
+            check_ok(test_i, dir, 'stop', 'cfg_script', 0)
+        end)
+    end)
+
+    cleanup_instance(dir, 'cfg_script')
+    recursive_rmdir(dir)
+
+    if status == false then
+        print(("Error: %s"):format(err))
+        os.exit()
+    end
+end
+
 os.exit(test:check() == true and 0 or -1)
diff --git a/test/box-tap/gh-5602-environment-vars-cfg.result b/test/box-tap/gh-5602-environment-vars-cfg.result
new file mode 100644
index 000000000..c90dd301b
--- /dev/null
+++ b/test/box-tap/gh-5602-environment-vars-cfg.result
@@ -0,0 +1,41 @@
+TAP version 13
+1..5
+ok - box.cfg is successful
+ok - listen
+ok - readahead
+ok - strip_core
+ok - log_format is not set
+TAP version 13
+1..7
+ok - box.cfg is successful
+ok - listen
+ok - replication is table
+ok - replication URI 1
+ok - replication URI 2
+ok - replication_connect_timeout
+ok - replication_synchro_quorum
+TAP version 13
+1..3
+ok - box.cfg is successful
+ok - box.cfg{} background value is prioritized
+ok - box.cfg{} vinyl_timeout value is prioritized
+TAP version 13
+1..2
+ok - box.cfg is not successful
+ok - bad sql_cache_size value
+TAP version 13
+1..2
+ok - box.cfg is not successful
+ok - bad strip_core value
+TAP version 13
+1..2
+ok - box.cfg is not successful
+ok - bad replication value
+TAP version 13
+1..6
+ok - set 1
+ok - set 2
+ok - set 3
+ok - set 4
+ok - set 5
+ok - set 6
diff --git a/test/box-tap/gh-5602-environment-vars-cfg.test.lua b/test/box-tap/gh-5602-environment-vars-cfg.test.lua
new file mode 100755
index 000000000..3d19fdc90
--- /dev/null
+++ b/test/box-tap/gh-5602-environment-vars-cfg.test.lua
@@ -0,0 +1,57 @@
+#!/usr/bin/env tarantool
+
+local os = require('os')
+local fio = require('fio')
+local tap = require('tap')
+
+local test = tap.test('gh-5602')
+
+-- gh-5602: Check that environment cfg variables working.
+local TARANTOOL_PATH = arg[-1]
+local script_name = 'gh-5602-test-cases-script.lua'
+local path_to_script = fio.pathjoin(
+        os.getenv('PWD'),
+        'box-tap',
+        script_name)
+
+---
+-- Generate shell command where set is sequence of environment
+-- variables, then path to tarantool, the first arg is path to
+-- test script and the second one is number of set.
+--
+local function shell_command(set, i)
+    return ('%s %s %s %d'):format(
+        set,
+        TARANTOOL_PATH,
+        path_to_script,
+        i)
+end
+
+local set_1 = ('%s %s %s %s'):format('TT_LISTEN=3301', 'TT_READAHEAD=10000',
+    'TT_STRIP_CORE=false', 'TT_LOG_FORMAT=json')
+local set_2 = ('%s %s %s %s'):format('TT_LISTEN=3301',
+    'TT_REPLICATION=\"{\'0.0.0.0:12345\', \'1.1.1.1:12345\'}\"',
+    'TT_REPLICATION_CONNECT_TIMEOUT=0.01',
+    'TT_REPLICATION_SYNCHRO_QUORUM=\'4 + 1\'')
+local set_3 = 'TT_BACKGROUND=true TT_VINYL_TIMEOUT=60.1'
+local set_4 = 'TT_SQL_CACHE_SIZE=a'
+local set_5 = 'TT_STRIP_CORE=a'
+local set_6 = 'TT_REPLICATION=\"{0.0.0.0:12345\', \'1.1.1.1:12345\'}\"'
+
+local sets = {}
+sets[1] = set_1
+sets[2] = set_2
+sets[3] = set_3
+sets[4] = set_4
+sets[5] = set_5
+sets[6] = set_6
+
+test:plan(#sets)
+for i, set in ipairs(sets) do
+    local tmpdir = fio.tempdir()
+    local new_path = fio.pathjoin(tmpdir, script_name)
+    fio.copyfile(path_to_script, new_path)
+    test:is(os.execute(shell_command(set, i)), 0, 'set '..i)
+end
+
+os.exit(test:check() and 0 or 1)
diff --git a/test/box-tap/gh-5602-test-cases-script.lua b/test/box-tap/gh-5602-test-cases-script.lua
new file mode 100755
index 000000000..a8b2fc987
--- /dev/null
+++ b/test/box-tap/gh-5602-test-cases-script.lua
@@ -0,0 +1,69 @@
+local tap = require('tap')
+
+local test = tap.test('gh-5602')
+
+local status, err = pcall(box.cfg, {background = false, vinyl_timeout = 70.1})
+
+-- Check that environment cfg values are set correctly.
+if arg[1] == '1' then
+    test:plan(5)
+    test:ok(status ~= false, 'box.cfg is successful')
+    test:is(box.cfg['listen'], 3301, 'listen')
+    test:is(box.cfg['readahead'], 10000, 'readahead')
+    test:is(box.cfg['strip_core'], false, 'strip_core')
+    test:is(box.cfg['log_format'], 'json', 'log_format is not set')
+end
+if arg[1] == '2' then
+    test:plan(7)
+    test:ok(status ~= false, 'box.cfg is successful')
+    test:is(box.cfg['listen'], 3301, 'listen')
+    local replication = box.cfg['replication']
+    test:is(type(replication), 'table', 'replication is table')
+    test:ok(replication[1] == '0.0.0.0:12345' or
+            replication[1] == '1.1.1.1:12345', 'replication URI 1')
+    test:ok(replication[2] == '0.0.0.0:12345' or
+            replication[2] == '1.1.1.1:12345', 'replication URI 2')
+    test:is(box.cfg['replication_connect_timeout'], 0.01,
+            'replication_connect_timeout')
+    test:is(box.cfg['replication_synchro_quorum'], '4 + 1',
+            'replication_synchro_quorum')
+end
+
+-- Check that box.cfg{} values are more prioritized than
+-- environment cfg values.
+if arg[1] == '3' then
+    test:plan(3)
+    test:ok(status ~= false, 'box.cfg is successful')
+    test:is(box.cfg['background'], false,
+            'box.cfg{} background value is prioritized')
+    test:is(box.cfg['vinyl_timeout'], 70.1,
+            'box.cfg{} vinyl_timeout value is prioritized')
+end
+
+local err_msg_fmt = 'Environment variable TT_%s has '..
+    'incorrect value for option \'%s\': should be %s'
+
+-- Check bad environment cfg values.
+if arg[1] == '4' then
+    test:plan(2)
+    test:ok(status == false, 'box.cfg is not successful')
+    local index = string.find(err, err_msg_fmt:format('SQL_CACHE_SIZE',
+        'sql_cache_size', 'convertible to number'))
+    test:ok(index ~= nil, 'bad sql_cache_size value')
+end
+if arg[1] == '5' then
+    test:plan(2)
+    test:ok(status == false, 'box.cfg is not successful')
+    local index = string.find(err, err_msg_fmt:format('STRIP_CORE',
+        'strip_core', '\'true\' or \'false\''))
+    test:ok(index ~= nil, 'bad strip_core value')
+end
+if arg[1] == '6' then
+    test:plan(2)
+    test:ok(status == false, 'box.cfg is not successful')
+    local index = string.find(tostring(err), 'Incorrect value for option '..
+        '\'replication\': expected host:service or /unix.socket')
+    test:ok(index ~= nil, 'bad replication value')
+end
+
+os.exit(test:check() and 0 or 1)
-- 
2.24.3 (Apple Git-128)


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-04-13 12:47 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-14 15:34 [Tarantool-patches] [PATCH] box: set cfg via environment variables Roman Khabibov via Tarantool-patches
2021-03-19 12:38 ` Leonid Vasiliev via Tarantool-patches
2021-04-09  8:56   ` Roman Khabibov via Tarantool-patches
2021-04-12  8:37     ` Leonid Vasiliev via Tarantool-patches
2021-04-13 12:47       ` Roman Khabibov via Tarantool-patches
2021-04-12  9:52     ` Leonid Vasiliev via Tarantool-patches
2021-03-19 13:23 ` Leonid Vasiliev via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox