* [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton @ 2018-07-17 10:25 Olga Arkhangelskaia 2018-07-17 10:25 ` [tarantool-patches] [PATCH 2/2] Fixes logging to syslog Olga Arkhangelskaia 2018-07-17 13:13 ` [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton Vladimir Davydov 0 siblings, 2 replies; 9+ messages in thread From: Olga Arkhangelskaia @ 2018-07-17 10:25 UTC (permalink / raw) To: tarantool-patches; +Cc: 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) ^ permalink raw reply [flat|nested] 9+ messages in thread
* [tarantool-patches] [PATCH 2/2] Fixes logging to syslog 2018-07-17 10:25 [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton Olga Arkhangelskaia @ 2018-07-17 10:25 ` Olga Arkhangelskaia 2018-07-19 13:08 ` Vladimir Davydov 2018-07-17 13:13 ` [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton Vladimir Davydov 1 sibling, 1 reply; 9+ messages in thread From: Olga Arkhangelskaia @ 2018-07-17 10:25 UTC (permalink / raw) To: tarantool-patches; +Cc: 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) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [tarantool-patches] [PATCH 2/2] Fixes logging to syslog 2018-07-17 10:25 ` [tarantool-patches] [PATCH 2/2] Fixes logging to syslog Olga Arkhangelskaia @ 2018-07-19 13:08 ` Vladimir Davydov 0 siblings, 0 replies; 9+ messages in thread From: Vladimir Davydov @ 2018-07-19 13:08 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches 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). ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton 2018-07-17 10:25 [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton Olga Arkhangelskaia 2018-07-17 10:25 ` [tarantool-patches] [PATCH 2/2] Fixes logging to syslog Olga Arkhangelskaia @ 2018-07-17 13:13 ` Vladimir Davydov 2018-07-17 13:23 ` Vladimir Davydov 2018-07-17 14:05 ` Re[2]: " Olga Arkhangelskaia 1 sibling, 2 replies; 9+ messages in thread From: Vladimir Davydov @ 2018-07-17 13:13 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches 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) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton 2018-07-17 13:13 ` [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton Vladimir Davydov @ 2018-07-17 13:23 ` Vladimir Davydov 2018-07-17 14:07 ` Re[2]: " Olga Arkhangelskaia 2018-07-17 14:05 ` Re[2]: " Olga Arkhangelskaia 1 sibling, 1 reply; 9+ messages in thread From: Vladimir Davydov @ 2018-07-17 13:23 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches 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) ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re[2]: [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton 2018-07-17 13:23 ` Vladimir Davydov @ 2018-07-17 14:07 ` Olga Arkhangelskaia 2018-07-17 14:10 ` Vladimir Davydov 0 siblings, 1 reply; 9+ messages in thread From: Olga Arkhangelskaia @ 2018-07-17 14:07 UTC (permalink / raw) To: Vladimir Davydov; +Cc: tarantool-patches [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton 2018-07-17 14:07 ` Re[2]: " Olga Arkhangelskaia @ 2018-07-17 14:10 ` Vladimir Davydov 0 siblings, 0 replies; 9+ messages in thread From: Vladimir Davydov @ 2018-07-17 14:10 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches [ 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re[2]: [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton 2018-07-17 13:13 ` [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton Vladimir Davydov 2018-07-17 13:23 ` Vladimir Davydov @ 2018-07-17 14:05 ` Olga Arkhangelskaia 2018-07-17 14:22 ` Vladimir Davydov 1 sibling, 1 reply; 9+ messages in thread From: Olga Arkhangelskaia @ 2018-07-17 14:05 UTC (permalink / raw) To: Vladimir Davydov; +Cc: tarantool-patches [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton 2018-07-17 14:05 ` Re[2]: " Olga Arkhangelskaia @ 2018-07-17 14:22 ` Vladimir Davydov 0 siblings, 0 replies; 9+ messages in thread From: Vladimir Davydov @ 2018-07-17 14:22 UTC (permalink / raw) To: Olga Arkhangelskaia; +Cc: tarantool-patches 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-07-19 13:08 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-07-17 10:25 [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton Olga Arkhangelskaia 2018-07-17 10:25 ` [tarantool-patches] [PATCH 2/2] Fixes logging to syslog Olga Arkhangelskaia 2018-07-19 13:08 ` Vladimir Davydov 2018-07-17 13:13 ` [tarantool-patches] [PATCH 1/2] Test for valid syslog configuraton Vladimir Davydov 2018-07-17 13:23 ` Vladimir Davydov 2018-07-17 14:07 ` Re[2]: " Olga Arkhangelskaia 2018-07-17 14:10 ` Vladimir Davydov 2018-07-17 14:05 ` Re[2]: " Olga Arkhangelskaia 2018-07-17 14:22 ` Vladimir Davydov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox