

<HTML><BODY><div>LGTM.<br><br> <blockquote style="border-left:1px solid #0857A6; margin:10px; padding:0 0 0 10px;">Четверг, 21 мая 2020, 14:38 +03:00 от Alexander V. Tikhonov <avtikhon@tarantool.org>:<br> <div id=""><div class="js-helper js-readmsg-msg"><style type="text/css"></style><div><div id="style_15900611090632560880_BODY">Alexander, thanks for the review, I've corrected all as you suggested.<br>Sure I'll control that test-run must be pushed before this patch.<br><br>On Thu, May 21, 2020 at 02:29:33PM +0300, Alexander Turenko wrote:<div class="mail-quote-collapse">> LGTM.<br>><br>> (But should NOT be pushed to master until I'll push test-run fix from PR<br>> #211.)<br>><br>> Left a couple of nits, however. See below.<br>><br>> WBR, Alexander Turenko.<br>><br>> > 60f84cbf ('test: use unix sockets for iproto connections')<br>><br>> Nit: We mention commits in the form<br>> 0000000000000000000000000000000000000000 ('commit message header'): full<br>> 40-digits hash and the commit message header. You'll need to carry the<br>> header when will use full commit hash and it is okay.<br>><br>> Personally I think that short hashes lookes better when used inline<br>> within a text and that our style will recommend them sooner or later.<br>> But now it is not so.<br>><br>> > diff --git a/test/app-tap/tarantoolctl.test.lua b/test/app-tap/tarantoolctl.test.lua<br>> > index 4d7059559..dd90c8a25 100755<br>> > --- a/test/app-tap/tarantoolctl.test.lua<br>> > +++ b/test/app-tap/tarantoolctl.test.lua<br>> > @@ -465,12 +465,9 @@ else<br>> > local remote_path = create_script(dir, 'remote.lua', remote_code)<br>> > test_run:cmd(("create server remote with script='%s'"):format(remote_path))<br>> > test_run:cmd("start server remote")<br>> > - local port = tonumber(<br>> > - test_run:eval("remote",<br>> > - "return require('uri').parse(box.cfg.listen).service")[1]<br>> > - )<br>> > + local admin_socket = test_run:eval("remote", "return box.cfg.listen")[1]<br>><br>> Nit: Admin port / socket usually refer a console port / socket. I would<br>> name the variable `listen_url` or `remote_uri`. The former is used<br>> several times across tests and also is part of `return_listen_uri`<br>> option name.</div></div></div></div></div></blockquote> <div> </div><div data-signature-widget="container"><div data-signature-widget="content"><div>--<br>Oleg Piskunov</div></div></div><div> </div></div></BODY></HTML>