[Tarantool-patches] [PATCH v1] Move .tarantoolctl to test-run tool submodule

[Tarantool-patches] [PATCH v1] Move .tarantoolctl to test-run tool submodule

[Alexander V. Tikhonov]

Moved .tarantoolctl configuration file to test-run tool submodule
repository as:

  <tarantool repository>/test-run/.tarantoolctl

Also set backward compability to use old path location:

  <tarantool repository>/test/.tarantoolctl

as its primary place.

Also updated test with replication_sync_timeout check value.
---

Github: https://github.com/tarantool/tarantool/tree/avtikhon/move_tarantoolctl_config

 extra/dist/tarantoolctl.in |  3 +++
 test-run                   |  2 +-
 test/.tarantoolctl         | 15 ---------------
 test/CMakeLists.txt        | 13 ++++++++++---
 test/box/admin.result      |  2 +-
 test/box/cfg.result        |  4 ++--
 6 files changed, 17 insertions(+), 22 deletions(-)
 delete mode 100644 test/.tarantoolctl

diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
index 0726e7f46..92a5c4225 100755
--- a/extra/dist/tarantoolctl.in
+++ b/extra/dist/tarantoolctl.in
@@ -70,6 +70,9 @@ local function check_user_level()
     local pwd = os.getenv('PWD')
     local udir = pwd and pwd .. '/.tarantoolctl'
     udir = udir and fio.stat(udir) and udir or nil
+    -- or test-run dir
+    udir = udir or pwd and pwd .. '/../test-run/.tarantoolctl'
+    udir = udir and fio.stat(udir) and udir or nil
     -- or home dir configuration
     local homedir = os.getenv('HOME')
     udir = udir or homedir and homedir .. '/.config/tarantool/tarantool'
diff --git a/test-run b/test-run
index 96dea99e6..1638eaa38 160000
--- a/test-run
+++ b/test-run
@@ -1 +1 @@
-Subproject commit 96dea99e69242380258d8dbccfb3735cce753d5a
+Subproject commit 1638eaa384c9d2c30b6c2240add11e45d27ecd95
diff --git a/test/.tarantoolctl b/test/.tarantoolctl
deleted file mode 100644
index 5c46f8acf..000000000
--- a/test/.tarantoolctl
+++ /dev/null
@@ -1,15 +0,0 @@
--- Options for test-run tarantoolctl
-
-local workdir = os.getenv('TEST_WORKDIR')
-default_cfg = {
-    pid_file   = workdir,
-    wal_dir    = workdir,
-    memtx_dir  = workdir,
-    vinyl_dir  = workdir,
-    log        = workdir,
-    background = false,
-}
-
-instance_dir = workdir
-
--- vim: set ft=lua :
diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
index 10882c6a1..f1c8c8d46 100644
--- a/test/CMakeLists.txt
+++ b/test/CMakeLists.txt
@@ -80,10 +80,17 @@ add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/luajit/test
 
 # Move tarantoolctl config
 if (NOT ${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR})
-    configure_file(
-        "${PROJECT_SOURCE_DIR}/test/.tarantoolctl"
-        "${PROJECT_BINARY_DIR}/test/.tarantoolctl"
+    if(EXISTS "${PROJECT_SOURCE_DIR}/test/.tarantoolctl")
+        configure_file(
+            "${PROJECT_SOURCE_DIR}/test/.tarantoolctl"
+            "${PROJECT_BINARY_DIR}/test/.tarantoolctl"
         )
+    else()
+        configure_file(
+            "${PROJECT_SOURCE_DIR}/test-run/.tarantoolctl"
+            "${PROJECT_BINARY_DIR}/test/.tarantoolctl"
+        )
+    endif()
 endif()
 
 # Disable connector_c for 1.6
diff --git a/test/box/admin.result b/test/box/admin.result
index 8c5626c36..59ab4791e 100644
--- a/test/box/admin.result
+++ b/test/box/admin.result
@@ -86,7 +86,7 @@ cfg_filter(box.cfg)
   - - replication_sync_lag
     - 10
   - - replication_sync_timeout
-    - 300
+    - 100
   - - replication_synchro_quorum
     - 1
   - - replication_synchro_timeout
diff --git a/test/box/cfg.result b/test/box/cfg.result
index 4ad3c6493..2603650bc 100644
--- a/test/box/cfg.result
+++ b/test/box/cfg.result
@@ -74,7 +74,7 @@ cfg_filter(box.cfg)
  |   - - replication_sync_lag
  |     - 10
  |   - - replication_sync_timeout
- |     - 300
+ |     - 100
  |   - - replication_synchro_quorum
  |     - 1
  |   - - replication_synchro_timeout
@@ -187,7 +187,7 @@ cfg_filter(box.cfg)
  |   - - replication_sync_lag
  |     - 10
  |   - - replication_sync_timeout
- |     - 300
+ |     - 100
  |   - - replication_synchro_quorum
  |     - 1
  |   - - replication_synchro_timeout
-- 
2.25.1

Re: [Tarantool-patches] [PATCH v1] Move .tarantoolctl to test-run tool submodule

[Alexander Turenko]

> Github: https://github.com/tarantool/tarantool/tree/avtikhon/move_tarantoolctl_config

> diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
> index 0726e7f46..92a5c4225 100755
> --- a/extra/dist/tarantoolctl.in
> +++ b/extra/dist/tarantoolctl.in
> @@ -70,6 +70,9 @@ local function check_user_level()
>      local pwd = os.getenv('PWD')
>      local udir = pwd and pwd .. '/.tarantoolctl'
>      udir = udir and fio.stat(udir) and udir or nil
> +    -- or test-run dir
> +    udir = udir or pwd and pwd .. '/../test-run/.tarantoolctl'
> +    udir = udir and fio.stat(udir) and udir or nil
>      -- or home dir configuration
>      local homedir = os.getenv('HOME')
>      udir = udir or homedir and homedir .. '/.config/tarantool/tarantool'

Now test-run copies test/.tarantoolctl to vardir. After [1] it should
copy it either from test/.tarantoolctl or from test-run/.tarantoolctl.
We don't need and should not change tarantoolctl for that.

[1]: https://github.com/tarantool/test-run/pull/242

> diff --git a/test-run b/test-run
> index 96dea99e6..1638eaa38 160000
> --- a/test-run
> +++ b/test-run
> @@ -1 +1 @@
> -Subproject commit 96dea99e69242380258d8dbccfb3735cce753d5a
> +Subproject commit 1638eaa384c9d2c30b6c2240add11e45d27ecd95

I suggest to just mention the PR in test-run, where the feature is
implemented. The logic is the following: when you send a patch for
review, it should be in the state that is ready to push to master.
However we should not update a submodule in master to a transient commit
(one that is not in the submodule's master / long term branch).
Otherwise we'll hit by problems like [2].

[2]: https://github.com/tarantool/small/issues/20

> diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> index 10882c6a1..f1c8c8d46 100644
> --- a/test/CMakeLists.txt
> +++ b/test/CMakeLists.txt
> @@ -80,10 +80,17 @@ add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/luajit/test
>  
>  # Move tarantoolctl config
>  if (NOT ${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR})
> -    configure_file(
> -        "${PROJECT_SOURCE_DIR}/test/.tarantoolctl"
> -        "${PROJECT_BINARY_DIR}/test/.tarantoolctl"
> +    if(EXISTS "${PROJECT_SOURCE_DIR}/test/.tarantoolctl")
> +        configure_file(
> +            "${PROJECT_SOURCE_DIR}/test/.tarantoolctl"
> +            "${PROJECT_BINARY_DIR}/test/.tarantoolctl"
>          )
> +    else()
> +        configure_file(
> +            "${PROJECT_SOURCE_DIR}/test-run/.tarantoolctl"
> +            "${PROJECT_BINARY_DIR}/test/.tarantoolctl"
> +        )
> +    endif()
>  endif()

I didn't find anything like @BLABLA@ or ${BLABLA} in the file (see [3]).
So it just copies the file to the build directory. Since we want to ship
it within test-run, there is no need for copying anymore.

It seems, the block can be just removed with explanation in the commit
message. `git blame` is your friend.

[3]: https://cmake.org/cmake/help/latest/command/configure_file.html

> diff --git a/test/box/admin.result b/test/box/admin.result
> index 8c5626c36..59ab4791e 100644
> --- a/test/box/admin.result
> +++ b/test/box/admin.result
> @@ -86,7 +86,7 @@ cfg_filter(box.cfg)
>    - - replication_sync_lag
>      - 10
>    - - replication_sync_timeout
> -    - 300
> +    - 100
>    - - replication_synchro_quorum
>      - 1
>    - - replication_synchro_timeout

How about add it to _hide in test/box/box.lua with appropriate comment?
AFAIU we test default values here, but this one is not default anymore
(and controlled by test-run). If we'll pass another value into the
test-run option, the test would fail that does not look as a good
effect.

The same for test/box/cfg.test.lua.

Re: [Tarantool-patches] [PATCH v1] Move .tarantoolctl to test-run tool submodule

[Alexander V. Tikhonov]

Hi Alexander, thanks for the review and suggestions. I've made all
of them, please check below. CI results are available here [1].

[1] - https://gitlab.com/tarantool/tarantool/-/pipelines/223728321

On Sun, Nov 29, 2020 at 04:36:14AM +0300, Alexander Turenko wrote:
> > Github: https://github.com/tarantool/tarantool/tree/avtikhon/move_tarantoolctl_config
> 
> > diff --git a/extra/dist/tarantoolctl.in b/extra/dist/tarantoolctl.in
> > index 0726e7f46..92a5c4225 100755
> > --- a/extra/dist/tarantoolctl.in
> > +++ b/extra/dist/tarantoolctl.in
> > @@ -70,6 +70,9 @@ local function check_user_level()
> >      local pwd = os.getenv('PWD')
> >      local udir = pwd and pwd .. '/.tarantoolctl'
> >      udir = udir and fio.stat(udir) and udir or nil
> > +    -- or test-run dir
> > +    udir = udir or pwd and pwd .. '/../test-run/.tarantoolctl'
> > +    udir = udir and fio.stat(udir) and udir or nil
> >      -- or home dir configuration
> >      local homedir = os.getenv('HOME')
> >      udir = udir or homedir and homedir .. '/.config/tarantool/tarantool'
> 
> Now test-run copies test/.tarantoolctl to vardir. After [1] it should
> copy it either from test/.tarantoolctl or from test-run/.tarantoolctl.
> We don't need and should not change tarantoolctl for that.
> 
> [1]: https://github.com/tarantool/test-run/pull/242
> 

Right, added PWD environment to be able to use .tarantoolctl file right
from the test working directories.

> > diff --git a/test-run b/test-run
> > index 96dea99e6..1638eaa38 160000
> > --- a/test-run
> > +++ b/test-run
> > @@ -1 +1 @@
> > -Subproject commit 96dea99e69242380258d8dbccfb3735cce753d5a
> > +Subproject commit 1638eaa384c9d2c30b6c2240add11e45d27ecd95
> 
> I suggest to just mention the PR in test-run, where the feature is
> implemented. The logic is the following: when you send a patch for
> review, it should be in the state that is ready to push to master.
> However we should not update a submodule in master to a transient commit
> (one that is not in the submodule's master / long term branch).
> Otherwise we'll hit by problems like [2].
> 
> [2]: https://github.com/tarantool/small/issues/20
>

It was temporary used just to check the changes, for now removed.

> > diff --git a/test/CMakeLists.txt b/test/CMakeLists.txt
> > index 10882c6a1..f1c8c8d46 100644
> > --- a/test/CMakeLists.txt
> > +++ b/test/CMakeLists.txt
> > @@ -80,10 +80,17 @@ add_subdirectory(${PROJECT_SOURCE_DIR}/third_party/luajit/test
> >  
> >  # Move tarantoolctl config
> >  if (NOT ${PROJECT_BINARY_DIR} STREQUAL ${PROJECT_SOURCE_DIR})
> > -    configure_file(
> > -        "${PROJECT_SOURCE_DIR}/test/.tarantoolctl"
> > -        "${PROJECT_BINARY_DIR}/test/.tarantoolctl"
> > +    if(EXISTS "${PROJECT_SOURCE_DIR}/test/.tarantoolctl")
> > +        configure_file(
> > +            "${PROJECT_SOURCE_DIR}/test/.tarantoolctl"
> > +            "${PROJECT_BINARY_DIR}/test/.tarantoolctl"
> >          )
> > +    else()
> > +        configure_file(
> > +            "${PROJECT_SOURCE_DIR}/test-run/.tarantoolctl"
> > +            "${PROJECT_BINARY_DIR}/test/.tarantoolctl"
> > +        )
> > +    endif()
> >  endif()
> 
> I didn't find anything like @BLABLA@ or ${BLABLA} in the file (see [3]).
> So it just copies the file to the build directory. Since we want to ship
> it within test-run, there is no need for copying anymore.
> 
> It seems, the block can be just removed with explanation in the commit
> message. `git blame` is your friend.
> 
> [3]: https://cmake.org/cmake/help/latest/command/configure_file.html
> 

Sure, removed and comment added in commit message.

> > diff --git a/test/box/admin.result b/test/box/admin.result
> > index 8c5626c36..59ab4791e 100644
> > --- a/test/box/admin.result
> > +++ b/test/box/admin.result
> > @@ -86,7 +86,7 @@ cfg_filter(box.cfg)
> >    - - replication_sync_lag
> >      - 10
> >    - - replication_sync_timeout
> > -    - 300
> > +    - 100
> >    - - replication_synchro_quorum
> >      - 1
> >    - - replication_synchro_timeout
> 
> How about add it to _hide in test/box/box.lua with appropriate comment?
> AFAIU we test default values here, but this one is not default anymore
> (and controlled by test-run). If we'll pass another value into the
> test-run option, the test would fail that does not look as a good
> effect.
> 
> The same for test/box/cfg.test.lua.

Ok, made as suggested.