[tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton

[tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton

[Olga Arkhangelskaia]

Added test to check if the valid syslog configuration is ok.

Issue: #3502
---
https://github.com/tarantool/tarantool/issues/3502

Branch: OKriw/gh-3502-syslog-fix
---
https://github.com/tarantool/tarantool/tree/OKriw/gh-3502-syslog-fix
---
 test/box-tap/cfg.test.lua | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
index ffafdbe42..605623013 100755
--- a/test/box-tap/cfg.test.lua
+++ b/test/box-tap/cfg.test.lua
@@ -6,7 +6,7 @@ local socket = require('socket')
 local fio = require('fio')
 local uuid = require('uuid')
 local msgpack = require('msgpack')
-test:plan(95)
+test:plan(96)
 
 --------------------------------------------------------------------------------
 -- Invalid values
@@ -464,5 +464,11 @@ code = string.format(code_fmt, dir, instance_uuid, uuid.new())
 test:is(run_script(code), PANIC, "replicaset_uuid mismatch")
 fio.rmdir(dir)
 
+--
+-- Check syslog configuration
+--
+code = [[box.cfg{log = 'syslog:identity=tarantool'}]]
+test:is(run_script(code), 0, "valid log configuration is broken")
+
 test:check()
 os.exit(0)
-- 
2.14.3 (Apple Git-98)

[tarantool-patches] [PATCH 2/2] Fixes logging to syslog

[Olga Arkhangelskaia]

Mode logging to syslog was broken due to assertion in case
ov vaild configuration:
box.cfg{log = 'syslog:identity=tarantool'}

Isue: #3502
---
https://github.com/tarantool/tarantool/issues/3502

Branch: OKriw/gh-3502-syslog-fix
---
https://github.com/tarantool/tarantool/tree/OKriw/gh-3502-syslog-fix
---
 src/box/box.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/box/box.cc b/src/box/box.cc
index 7c6312d78..9252e82c1 100644
--- a/src/box/box.cc
+++ b/src/box/box.cc
@@ -375,9 +375,9 @@ box_check_say()
 				tnt_raise(ClientError, ER_CFG, "log",
 					  diag_last_error(diag_get())->errmsg);
 			}
+			diag_raise();
 		}
 		say_free_syslog_opts(&opts);
-		diag_raise();
 	}
 
 	const char *log_format = cfg_gets("log_format");
-- 
2.14.3 (Apple Git-98)

Re: [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton

[Vladimir Davydov]

As this is the second version of this patch, you should add v2 to the
subject line and write a brief change log.

On Tue, Jul 17, 2018 at 01:25:07PM +0300, Olga Arkhangelskaia wrote:
> Added test to check if the valid syslog configuration is ok.
> 
> Issue: #3502

Should be:

Closes #3502

> ---
> https://github.com/tarantool/tarantool/issues/3502
> 
> Branch: OKriw/gh-3502-syslog-fix
> ---
> https://github.com/tarantool/tarantool/tree/OKriw/gh-3502-syslog-fix

You misunderstood me. The links shouldn't go to the commit message.
You should open the patch file produced by git-format-patch in your
favorite text editor and add the links manually after --- generated by
git-format-patch. Here's a couple of examples of what a patch email is
supposed to look like:

With a cover letter:
https://www.freelists.org/post/tarantool-patches/PATCH-v2-02-alter-fix-WAL-error-handling

Without a cover letter:
https://www.freelists.org/post/tarantool-patches/PATCH-v3-Introduce-replica-local-spaces

Note, neither links nor change log is included in the commit message
once a patch is committed:
https://github.com/tarantool/tarantool/commit/f64f46199e19542fa60eede939d62cd115abb83a

Please use them as sample.

> ---
>  test/box-tap/cfg.test.lua | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

Please squash the test into the patch with the fix (`git rebase -i`)

> 
> diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
> index ffafdbe42..605623013 100755
> --- a/test/box-tap/cfg.test.lua
> +++ b/test/box-tap/cfg.test.lua
> @@ -6,7 +6,7 @@ local socket = require('socket')
>  local fio = require('fio')
>  local uuid = require('uuid')
>  local msgpack = require('msgpack')
> -test:plan(95)
> +test:plan(96)
>  
>  --------------------------------------------------------------------------------
>  -- Invalid values
> @@ -464,5 +464,11 @@ code = string.format(code_fmt, dir, instance_uuid, uuid.new())
>  test:is(run_script(code), PANIC, "replicaset_uuid mismatch")
>  fio.rmdir(dir)
>  
> +--
> +-- Check syslog configuration
> +--
> +code = [[box.cfg{log = 'syslog:identity=tarantool'}]]
> +test:is(run_script(code), 0, "valid log configuration is broken")
> +
>  test:check()
>  os.exit(0)

Re: [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton

[Vladimir Davydov]

On Tue, Jul 17, 2018 at 04:13:49PM +0300, Vladimir Davydov wrote:
> >  test/box-tap/cfg.test.lua | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> Please squash the test into the patch with the fix (`git rebase -i`)

Hmm, the test doesn't pass on certain distros (see travis). I guess,
this is because they don't have syslog configured. What about wrapping
box.cfg in pcall and checking the status? Since we just want to check
that there's no crash, this should do. Or alternatively you can test
this by setting a custom syslog destination (you have to test it anyway
in your other patches AFAIR). In this case, this particular fix doesn't
need a test case at all - it's suffices to mention that the fix is going
to be tested in the following patches.

> 
> > 
> > diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
> > index ffafdbe42..605623013 100755
> > --- a/test/box-tap/cfg.test.lua
> > +++ b/test/box-tap/cfg.test.lua
> > @@ -6,7 +6,7 @@ local socket = require('socket')
> >  local fio = require('fio')
> >  local uuid = require('uuid')
> >  local msgpack = require('msgpack')
> > -test:plan(95)
> > +test:plan(96)
> >  
> >  --------------------------------------------------------------------------------
> >  -- Invalid values
> > @@ -464,5 +464,11 @@ code = string.format(code_fmt, dir, instance_uuid, uuid.new())
> >  test:is(run_script(code), PANIC, "replicaset_uuid mismatch")
> >  fio.rmdir(dir)
> >  
> > +--
> > +-- Check syslog configuration
> > +--
> > +code = [[box.cfg{log = 'syslog:identity=tarantool'}]]
> > +test:is(run_script(code), 0, "valid log configuration is broken")
> > +
> >  test:check()
> >  os.exit(0)

Re[2]: [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton

[Olga Arkhangelskaia]

[-- Attachment #1: Type: text/plain, Size: 2565 bytes --]




>Вторник, 17 июля 2018, 16:13 +03:00 от Vladimir Davydov <vdavydov.dev@gmail.com>:
>
>As this is the second version of this patch, you should add v2 to the
>subject line and write a brief change log. 
Sorry, I remembered it after sending.
>
>
>On Tue, Jul 17, 2018 at 01:25:07PM +0300, Olga Arkhangelskaia wrote:
>> Added test to check if the valid syslog configuration is ok.
>> 
>> Issue: #3502
>
>Should be:
>
>Closes #3502 
Is ir because it is a bug? Because Kirill told me about Issue: and Branch: and then links.

>
>
>> ---
>>  https://github.com/tarantool/tarantool/issues/3502
>> 
>> Branch: OKriw/gh-3502-syslog-fix
>> ---
>>  https://github.com/tarantool/tarantool/tree/OKriw/gh-3502-syslog-fix
>
>You misunderstood me. The links shouldn't go to the commit message.
>You should open the patch file produced by git-format-patch in your
>favorite text editor and add the links manually after --- generated by
>git-format-patch. Here's a couple of examples of what a patch email is
>supposed to look like:
>
>With a cover letter:
>https://www.freelists.org/post/tarantool-patches/PATCH-v2-02-alter-fix-WAL-error-handling
>
>Without a cover letter:
>https://www.freelists.org/post/tarantool-patches/PATCH-v3-Introduce-replica-local-spaces
>
>Note, neither links nor change log is included in the commit message
>once a patch is committed:
>https://github.com/tarantool/tarantool/commit/f64f46199e19542fa60eede939d62cd115abb83a
>
>Please use them as sample.
Ok, thanks
>
>> ---
>>  test/box-tap/cfg.test.lua | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>
>Please squash the test into the patch with the fix (`git rebase -i`)
>
>> 
>> diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
>> index ffafdbe42..605623013 100755
>> --- a/test/box-tap/cfg.test.lua
>> +++ b/test/box-tap/cfg.test.lua
>> @@ -6,7 +6,7 @@ local socket = require('socket')
>>  local fio = require('fio')
>>  local uuid = require('uuid')
>>  local msgpack = require('msgpack')
>> -test:plan(95)
>> +test:plan(96)
>> 
>>  --------------------------------------------------------------------------------
>>  -- Invalid values
>> @@ -464,5 +464,11 @@ code = string.format(code_fmt, dir, instance_uuid, uuid.new())
>>  test:is(run_script(code), PANIC, "replicaset_uuid mismatch")
>>  fio.rmdir(dir)
>> 
>> +--
>> +-- Check syslog configuration
>> +--
>> +code = [[box.cfg{log = 'syslog:identity=tarantool'}]]
>> +test:is(run_script(code), 0, "valid log configuration is broken")
>> +
>>  test:check()
>>  os.exit(0)


-- 
Olga Arkhangelskaia

[-- Attachment #2: Type: text/html, Size: 4343 bytes --]

Re[2]: [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton

[Olga Arkhangelskaia]

[-- Attachment #1: Type: text/plain, Size: 1981 bytes --]




>Вторник, 17 июля 2018, 16:23 +03:00 от Vladimir Davydov <vdavydov.dev@gmail.com>:
>
>On Tue, Jul 17, 2018 at 04:13:49PM +0300, Vladimir Davydov wrote:
>> >  test/box-tap/cfg.test.lua | 8 +++++++-
>> >  1 file changed, 7 insertions(+), 1 deletion(-)
>> 
>> Please squash the test into the patch with the fix (`git rebase -i`) 
Ok
>
>
>Hmm, the test doesn't pass on certain distros (see travis). I guess,
>this is because they don't have syslog configured. What about wrapping
>box.cfg in pcall and checking the status? Since we just want to check
>that there's no crash, this should do.  
I'll do that way.
>Or alternatively you can test
>this by setting a custom syslog destination (you have to test it anyway
>in your other patches AFAIR).  
Do you mean in syslog destination series? I had a thought about it, but did not do in v2. Will keep in mind

>In this case, this particular fix doesn't
>need a test case at all - it's suffices to mention that the fix is going
>to be tested in the following patches.
>
>> 
>> > 
>> > diff --git a/test/box-tap/cfg.test.lua b/test/box-tap/cfg.test.lua
>> > index ffafdbe42..605623013 100755
>> > --- a/test/box-tap/cfg.test.lua
>> > +++ b/test/box-tap/cfg.test.lua
>> > @@ -6,7 +6,7 @@ local socket = require('socket')
>> >  local fio = require('fio')
>> >  local uuid = require('uuid')
>> >  local msgpack = require('msgpack')
>> > -test:plan(95)
>> > +test:plan(96)
>> > 
>> >  --------------------------------------------------------------------------------
>> >  -- Invalid values
>> > @@ -464,5 +464,11 @@ code = string.format(code_fmt, dir, instance_uuid, uuid.new())
>> >  test:is(run_script(code), PANIC, "replicaset_uuid mismatch")
>> >  fio.rmdir(dir)
>> > 
>> > +--
>> > +-- Check syslog configuration
>> > +--
>> > +code = [[box.cfg{log = 'syslog:identity=tarantool'}]]
>> > +test:is(run_script(code), 0, "valid log configuration is broken")
>> > +
>> >  test:check()
>> >  os.exit(0)


-- 
Olga Arkhangelskaia

[-- Attachment #2: Type: text/html, Size: 3217 bytes --]

Re: [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton

[Vladimir Davydov]

[ Please add an extra new line after a quote when replying to an email -
  this improves readability ]

On Tue, Jul 17, 2018 at 05:07:32PM +0300, Olga Arkhangelskaia wrote:
> >Hmm, the test doesn't pass on certain distros (see travis). I guess,
> >this is because they don't have syslog configured. What about wrapping
> >box.cfg in pcall and checking the status? Since we just want to check
> >that there's no crash, this should do.  
> I'll do that way.
> >Or alternatively you can test
> >this by setting a custom syslog destination (you have to test it anyway
> >in your other patches AFAIR).  
> Do you mean in syslog destination series?

Yes.

> I had a thought about it, but did not do in v2. Will keep in mind

Up to you.

Re: [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton

[Vladimir Davydov]

On Tue, Jul 17, 2018 at 05:05:28PM +0300, Olga Arkhangelskaia wrote:
> 
> 
> 
> >Вторник, 17 июля 2018, 16:13 +03:00 от Vladimir Davydov <vdavydov.dev@gmail.com>:
> >
> >As this is the second version of this patch, you should add v2 to the
> >subject line and write a brief change log. 
> Sorry, I remembered it after sending.
> >
> >
> >On Tue, Jul 17, 2018 at 01:25:07PM +0300, Olga Arkhangelskaia wrote:
> >> Added test to check if the valid syslog configuration is ok.
> >> 
> >> Issue: #3502
> >
> >Should be:
> >
> >Closes #3502 
> Is ir because it is a bug? Because Kirill told me about Issue: and Branch: and then links.

Forget about everything Kirill told you ;-) Just use patches submitted
by other team members as sample (you should be subscribed to the mailing
list so you can see all of them).

We mention an issue in the commit message so that it is recognized by
GitHub. E.g. 'Closes ####' will make GitHub close the issue once the
commit is pushed to the trunk.

We also include hyperlinks to the issue and branch in the patch email.
These are for the reviewer. They are not committed to the git.


BTW I see that you use Mail.Ru web interface. Please don't - it mangles
code snippets. I strongly recommend you to install a mail user agent,
e.g. Thuderbird.

Re: [tarantool-patches] [PATCH 2/2] Fixes logging to syslog

[Vladimir Davydov]

Pushed to 2.0

We agreed that Olga will write a test for logging to syslog in
the scope of #3487 (she'll need it anyway).