Tarantool development patches archive
 help / color / mirror / Atom feed
From: Cyrill Gorcunov via Tarantool-patches <tarantool-patches@dev.tarantool.org>
To: Leonid Vasiliev <lvasiliev@tarantool.org>,
	Roman Khabibov <roman.habibov@tarantool.org>
Cc: TML <tarantool-patches@dev.tarantool.org>
Subject: Re: [Tarantool-patches] [PATCH v2 2/2] box: set cfg via environment variables
Date: Tue, 13 Apr 2021 20:57:11 +0300	[thread overview]
Message-ID: <YHXbd/51QbTJN3Ih@grain> (raw)
In-Reply-To: <28e3e145-d86b-bf21-8a40-73da577bfbff@tarantool.org>

Guys, I must confess I don't like dropping 'module' from template_cfg,
the reason it has been introduced in first place is that modules might
be used _before_ the box.cfg{} even called. So that the modules should
carry own config and provide it back to the load_lua code.

Currently we have one such 'early' module -- it is logger. But we might
extend it in future. Moreover spreading types here and there won't make
code anyhow easier to read and modify.

Also the question raises -- the environment variables are considered
only at box.cfg{} call, but should not be they also handled by modules
which support early use? If yes -- then we need to add such support
into the logger code. If no -- then it should be pointed in documentation
that log module ignores env variables if set up before box.cfg{} call.
Anoter option -- defer such support to the next release.

Now back to types. Current Roman's code test for basic types from @template_cfg
which means later when these values are setting into real @cfg variable they
are passing same testing again

            local good_types = string.gsub(template_cfg[k], ' ', '');
            if (string.find(',' .. good_types .. ',', ',' .. type(v) .. ',') == nil) then
                box.error(box.error.CFG, readable_name, "should be one of types "..

so the question is why do we need to do this double work? As far as I understand
the only thing what we need to do is to *fetch* data from environment variables
and set them into @cfg variable. Then loader will process every entry and report
an error if something goes wrong. Though I think we might need to dequote variables
just like it is done in the patch.

Now back to types. If you really think that we still need the validation of types
on the stage when we fetch the values from env variables then please don't revert
the commit with 'module' keyword but rather provide the types separately so
we rework them if needed. Modules should be extendable not squashed into defaults.

Say for now we could use
From: Cyrill Gorcunov <gorcunov@gmail.com>
Date: Tue, 13 Apr 2021 20:50:14 +0300
Subject: [PATCH] cfg: provide types for logger options

This needed for fast type check when fetching
options from environment variable.

Part-of #5602

Signed-off-by: Cyrill Gorcunov <gorcunov@gmail.com>
 src/box/lua/load_cfg.lua | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/src/box/lua/load_cfg.lua b/src/box/lua/load_cfg.lua
index d31c9eb2c..7f57fcbb1 100644
--- a/src/box/lua/load_cfg.lua
+++ b/src/box/lua/load_cfg.lua
@@ -118,6 +118,18 @@ local module_cfg = {
     log_format          = log.box_api,
+-- cfg types for modules, probably better to
+-- provide some API with type enumeration or
+-- similar. Currently it has use for environment
+-- processing only.
+local module_cfg_type = {
+    -- logging
+    log                 = 'string',
+    log_nonblock        = 'boolean',
+    log_level           = 'number, string',
+    log_format          = 'string',
 -- types of available options
 -- could be comma separated lua types or 'any' if any type is allowed
 local template_cfg = {
@@ -686,6 +698,10 @@ end
 local function process_option_value(option, raw_value)
     local param_type = template_cfg[option]
+    if param_type == 'module' then
+        param_type = module_cfg_type[option]
+    end
     -- May be peculiar to "feedback*" parameters.
     if param_type == nil then
         return nil

      reply	other threads:[~2021-04-13 17:57 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-13 12:45 [Tarantool-patches] [PATCH v2 0/2] Set " Roman Khabibov via Tarantool-patches
2021-04-13 12:45 ` [Tarantool-patches] [PATCH v2 1/2] lua/log: remove 'module' option type Roman Khabibov via Tarantool-patches
2021-04-13 14:52   ` Leonid Vasiliev via Tarantool-patches
2021-04-13 15:03     ` Cyrill Gorcunov via Tarantool-patches
2021-04-13 15:42       ` Leonid Vasiliev via Tarantool-patches
2021-04-13 12:45 ` [Tarantool-patches] [PATCH v2 2/2] box: set cfg via environment variables Roman Khabibov via Tarantool-patches
2021-04-13 15:39   ` Leonid Vasiliev via Tarantool-patches
2021-04-13 17:57     ` Cyrill Gorcunov via Tarantool-patches [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YHXbd/51QbTJN3Ih@grain \
    --to=tarantool-patches@dev.tarantool.org \
    --cc=gorcunov@gmail.com \
    --cc=lvasiliev@tarantool.org \
    --cc=roman.habibov@tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 2/2] box: set cfg via environment variables' \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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