From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <tarantool-patches-bounces@dev.tarantool.org>
Received: from [87.239.111.99] (localhost [127.0.0.1])
	by dev.tarantool.org (Postfix) with ESMTP id 2559B6EC56;
	Fri, 19 Mar 2021 15:39:16 +0300 (MSK)
DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 2559B6EC56
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev;
	t=1616157556; bh=2Qf4ep4797AkxSmXR9p6e/2wnWW2KNnYCz88sgUYV2E=;
	h=To:References:Date:In-Reply-To:Subject:List-Id:List-Unsubscribe:
	 List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:
	 From;
	b=L4TpNCdbGt5gk2qAD5e/PFc5ZQaPOwdcT+lvJAhOQh4GF4tDHnHc82fQTB1mBHh2i
	 7ZYmCQC/lm4Vy2VBDwKTi0bW0Qzc9xSOeKOt2OsWjlwF1PeBB4ES2CMq90H505rJBw
	 ivts9RYciOubL6z3cTaKGGCfz9HuVeW2EZnQTaWM=
Received: from smtp34.i.mail.ru (smtp34.i.mail.ru [94.100.177.94])
 (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
 (No client certificate requested)
 by dev.tarantool.org (Postfix) with ESMTPS id 36E966EC56
 for <tarantool-patches@dev.tarantool.org>;
 Fri, 19 Mar 2021 15:39:14 +0300 (MSK)
DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 36E966EC56
Received: by smtp34.i.mail.ru with esmtpa (envelope-from
 <lvasiliev@tarantool.org>)
 id 1lNEPJ-0005BA-CS; Fri, 19 Mar 2021 15:39:13 +0300
To: Roman Khabibov <roman.habibov@tarantool.org>,
 tarantool-patches@dev.tarantool.org
References: <20210314153457.85909-1-roman.habibov@tarantool.org>
Message-ID: <15c6bce9-c9e0-d043-7b02-e795dc4804eb@tarantool.org>
Date: Fri, 19 Mar 2021 15:38:10 +0300
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: <20210314153457.85909-1-roman.habibov@tarantool.org>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
X-7564579A: EEAE043A70213CC8
X-77F55803: 4F1203BC0FB41BD95D6E7CC48CB1F5F179C48A9DDACBFB6F5347129BC2C9341C182A05F5380850409169E6ED844AF14AF800D7F44DCF814705319A2325B8A3161D5F202D00DB4201
X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7B8498AC83B273C12EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F790063791F3A0CD9A153DC48638F802B75D45FF914D58D5BE9E6BC131B5C99E7648C95C81A23F326053F8275504CFCFE1C192B164579EBE29B53962A471835C12D1D9774AD6D5ED66289B5278DA827A17800CE77A825AB47F0FC8649FA2833FD35BB23D2EF20D2F80756B5F868A13BD56FB6657A471835C12D1D977725E5C173C3A84C37EF884183F8E4D67117882F4460429728AD0CFFFB425014E868A13BD56FB6657D81D268191BDAD3DC09775C1D3CA48CF9415BE9523AAFE76BA3038C0950A5D36C8A9BA7A39EFB766EC990983EF5C0329BA3038C0950A5D36D5E8D9A59859A8B666D89E201A891B4676E601842F6C81A1F004C90652538430CDED94BCBF13EF3B93EC92FD9297F6718AA50765F790063789CC3F4C0027555EA7F4EDE966BC389F395957E7521B51C24C7702A67D5C33162DBA43225CD8A89FD2A95C73FD1EFF45CE5475246E174218B5C8C57E37DE458B4C7702A67D5C3316FA3894348FB808DB48C21F01D89DB561574AF45C6390F7469DAA53EE0834AAEE
X-B7AD71C0: AC4F5C86D027EB782CDD5689AFBDA7A24A6D60772A99906F8E1CD14B953EB46DD78C9A0F7B376EC5355D89D7DBCDD132
X-C1DE0DAB: 0D63561A33F958A56A7F04CBA087A3E3D4874719118E570A1A4AF05A3F92F669D59269BC5F550898D99A6476B3ADF6B47008B74DF8BB9EF7333BD3B22AA88B938A852937E12ACA75F04B387B5D7535DE410CA545F18667F91A7EA1CDA0B5A7A0
X-C8649E89: 4E36BF7865823D7055A7F0CF078B5EC49A30900B95165D34980A6B448CFD1B8AA8B7C9D0FF2C308C2D837753649A87F98D28176E0C8F6F6BA5BA4DA5EB2E091D1D7E09C32AA3244C514FC2709AAE6819F4402954DC472E0CFE8DA44ABE2443F7729B2BEF169E0186
X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojyKyJYJ15DtJlg5EWq3NAtA==
X-Mailru-Sender: 48EFD2E18A91C559803250E574CC3D29745EC0B72C71AD26F800D7F44DCF8147E5E949923882BAADD35D514C7DAC97282D063C67CFD4E84987597EC79699C0E6F21BD4036121A32F7402F9BA4338D657ED14614B50AE0675
X-Mras: Ok
Subject: Re: [Tarantool-patches] [PATCH] box: set cfg via environment
 variables
X-BeenThere: tarantool-patches@dev.tarantool.org
X-Mailman-Version: 2.1.34
Precedence: list
List-Id: Tarantool development patches <tarantool-patches.dev.tarantool.org>
List-Unsubscribe: <https://lists.tarantool.org/mailman/options/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=unsubscribe>
List-Archive: <https://lists.tarantool.org/pipermail/tarantool-patches/>
List-Post: <mailto:tarantool-patches@dev.tarantool.org>
List-Help: <mailto:tarantool-patches-request@dev.tarantool.org?subject=help>
List-Subscribe: <https://lists.tarantool.org/mailman/listinfo/tarantool-patches>, 
 <mailto:tarantool-patches-request@dev.tarantool.org?subject=subscribe>
From: Leonid Vasiliev via Tarantool-patches
 <tarantool-patches@dev.tarantool.org>
Reply-To: Leonid Vasiliev <lvasiliev@tarantool.org>
Errors-To: tarantool-patches-bounces@dev.tarantool.org
Sender: "Tarantool-patches" <tarantool-patches-bounces@dev.tarantool.org>

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.

>