Tarantool development patches archive
 help / color / mirror / Atom feed
From: "Alexander Tikhonov" <avtikhon@tarantool.org>
To: "Sergey Bronnikov" <sergeyb@tarantool.org>
Cc: o.piskunov@tarantool.org, "Sergey Bronnikov" <estetus@gmail.com>,
	tarantool-patches@dev.tarantool.org
Subject: Re: [Tarantool-patches] [PATCH v2 6/6] gitlab-ci: enable static analysis with luacheck
Date: Wed, 22 Apr 2020 11:45:11 +0300	[thread overview]
Message-ID: <1587545111.412249565@f172.i.mail.ru> (raw)
In-Reply-To: <20200410145316.GA72590@pony.bronevichok.ru>

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


Hi Sergey, please check my comments below.

  
>Пятница, 10 апреля 2020, 17:53 +03:00 от Sergey Bronnikov <sergeyb@tarantool.org>:
> 
>Hello,
>
>thanks for review. Please see my answers inline.
>
>On 07:20 Thu 09 Apr , Alexander Tikhonov wrote:
>>
>> Hi Sergey, thank you for the patch, please check my comments below.
>>
>>  
>> >Среда, 8 апреля 2020, 18:43 +03:00 от Sergey Bronnikov < estetus@gmail.com >:
>> > 
>> >From: Sergey Bronnikov <  sergeyb@tarantool.org >
>> >
>> >Closes #4681
>> >---
>> Please add in the review letter the link to the branch here, also the issue link.
>
>Will do next time.
>
>> > .gitlab-ci.yml | 9 +++++++++
>> > .travis.mk | 12 +++++++++++-
>> > 2 files changed, 20 insertions(+), 1 deletion(-)
>> >
>> >diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
>> >index cd710027f..adbd892c8 100644
>> >--- a/.gitlab-ci.yml
>> >+++ b/.gitlab-ci.yml
>> >@@ -1,4 +1,5 @@
>> > stages:
>> >+ - static_analysis
>> Do we really need the separate stage for this job ? As we agreed with
>> A. Turenko before, we create stages only for jobs that needs additional
>> steps w/o next steps wont work and which can be used in the next stage
>> for more than single job.
>
>I think the answer is "yes". The arguments are:
>- it is a best practice in industry to split CI/CD pipeline for several
>stages. In canonical example they are: build, test, deploy.
Actually right that canonical way is good, but do we really need it. Is
it actually better than have the faster way to find the issues? I mean
that out gitlab-ci is using for now not for the automate production, but
for the fast developers testing — it means that they use it not to check
the final code, but to develop it and run it a lot of times during it. I can
absolutely agree that we need CI/CD process and we can start create
it here, but it should be the other separate pipeline from branches
testings. So my suggestion is to left the branch testing flat to be fast,
and add your change just for releases branches testings. By the way
gitlab-ci is too weak for CI/CD processes — much better to use Jenkins
either other CI/CD tools which can use pipelines in parallel instead of
gitlab-ci which can’t do it.
>- build, static analysis, unit testing, module testing are successive
>steps in finding bugs. Each step requires different amount of efforts to
>debug in case of failure. No reasons to perform unit testing when compiler
>warns about non-initialized variables and no reasons to perform complex
>functional testing when unit tests failed.
>
>We have future plans to add clang-analyzer and more strict warnings in
>compilers and 'static_analysis' stage will be a good place for these
>tasks on pipeline.
>
>> Anyway separate stage will add depends for the rest of the jobs, which
>> can be blocked because of any issue with luacheck, like it can be
>> luacheck cloning either its depends installations:
>
><snipped>
>
>> >index f709a18b6..32222621b 100644
>> >--- a/.travis.mk
>> >+++ b/.travis.mk
>> >@@ -77,8 +77,9 @@ deps_buster_clang_8: deps_debian
>> > # Release
>> > 
>> > build_debian:
>> >- cmake . -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}
>> >+ cmake . -DENABLE_DIST=ON -DCMAKE_BUILD_TYPE=RelWithDebInfo -DENABLE_WERROR=ON ${CMAKE_EXTRA_PARAMS}
>> This target is common wide along all the jobs and if ENABLE_DIST
>> define is not adding changes for them may be just need add the comment
>> the purpose on its adding.
>
>Well, I'll describe this in a commit message.
>
>> >  make -j
>> >+ sudo make install
>> This target is common wide along all the jobs and may be it better to
>> move the installation to the target ‘luacheck’ which only needs it.
>
>Well, I'll move 'make install' to a separate target.
>
><snipped>
>
>S. 
 
 
--
Alexander Tikhonov
 

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

      reply	other threads:[~2020-04-22  8:45 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-08 15:42 [Tarantool-patches] [PATCH v2 0/6] Add " Sergey Bronnikov
2020-04-08 15:43 ` [Tarantool-patches] [PATCH v2 1/6] Fix luacheck warnings in src/lua/ Sergey Bronnikov
2020-04-09  4:31   ` Alexander Tikhonov
2020-04-11 16:54   ` Vladislav Shpilevoy
2020-04-08 15:43 ` [Tarantool-patches] [PATCH v2 2/6] Fix luacheck warnings in test/ Sergey Bronnikov
2020-04-09  4:31   ` Alexander Tikhonov
2020-04-11 16:54   ` Vladislav Shpilevoy
2020-04-14  7:49     ` Sergey Bronnikov
2020-04-11 16:54   ` Vladislav Shpilevoy
2020-04-08 15:43 ` [Tarantool-patches] [PATCH v2 3/6] Fix luacheck warnings in src/box/lua/ Sergey Bronnikov
2020-04-09  4:29   ` Alexander Tikhonov
2020-04-08 15:43 ` [Tarantool-patches] [PATCH v2 4/6] Fix luacheck warnings in extra/dist/tarantoolctl.in Sergey Bronnikov
2020-04-09  4:29   ` Alexander Tikhonov
2020-04-09  7:30   ` Oleg Babin
2020-04-10 14:05     ` Sergey Bronnikov
2020-04-15 15:14   ` Igor Munkin
2020-04-15 15:37     ` Igor Munkin
2020-04-08 15:43 ` [Tarantool-patches] [PATCH v2 5/6] Add luacheck config Sergey Bronnikov
2020-04-09  4:27   ` Alexander Tikhonov
2020-04-11 16:54   ` Vladislav Shpilevoy
2020-04-13 15:16     ` Sergey Bronnikov
2020-04-14 23:29       ` Vladislav Shpilevoy
2020-04-15  8:30         ` Sergey Bronnikov
2020-04-08 15:43 ` [Tarantool-patches] [PATCH v2 6/6] gitlab-ci: enable static analysis with luacheck Sergey Bronnikov
2020-04-09  4:20   ` Alexander Tikhonov
2020-04-10 14:53     ` Sergey Bronnikov
2020-04-22  8:45       ` Alexander Tikhonov [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:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

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

  git send-email \
    --in-reply-to=1587545111.412249565@f172.i.mail.ru \
    --to=avtikhon@tarantool.org \
    --cc=estetus@gmail.com \
    --cc=o.piskunov@tarantool.org \
    --cc=sergeyb@tarantool.org \
    --cc=tarantool-patches@dev.tarantool.org \
    --subject='Re: [Tarantool-patches] [PATCH v2 6/6] gitlab-ci: enable static analysis with luacheck' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* 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