Home page logo
/

wireshark logo Wireshark mailing list archives

Re: Proposed Gerrit workflow (was: Re: Notes from Sharkfest '13)
From: Graham Bloice <graham.bloice () trihedral com>
Date: Sun, 23 Jun 2013 20:41:35 -0700

On 23 June 2013 11:58, Bálint Réczey <balint () balintreczey hu> wrote:

Hi,

2013/6/22 Marc Petit-Huguenin <marc () petit-huguenin org>:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 06/22/2013 09:43 AM, Bálint Réczey wrote:
Hi Marc,

2013/6/22 Marc Petit-Huguenin <marc () petit-huguenin org>:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256

On 06/22/2013 03:47 AM, Bálint Réczey wrote:
Hi All,

2013/6/21 Marc Petit-Huguenin <marc () petit-huguenin org>:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256

On 06/20/2013 04:52 PM, Guy Harris wrote:

On Jun 20, 2013, at 2:58 PM, Marc Petit-Huguenin
<marc () petit-huguenin org> wrote:

On 06/20/2013 02:17 PM, Gerald Combs wrote:

Advantates: - I'm not sure that an in-house equivalent (e.g.
Gerrit plus a private repository) would be better than what
Github offers.

Yes, Gerrit is better than github:

Presumably you mean "Gerrit plus a private repository is better
than github", as Gerrit, as far as I can tell, is just software
that works with a Git repository.

Yes, although managing repositories being what Gerrit do, Gerrit
without a least one repository would be a very boring application.
:-)

I have started describing a Gerrit based workflow which IMO would fit
to the project at http://wiki.wireshark.org/Development/Workflow .
Please check it and share your opinion.


"Code is building and tests are passing on all platforms. (Tests
automatically start when at least one Core Developer gives +1 or +2 to
prevent overloading or cracking the build servers.)"

Why do not build and test all patchsets submitted?  Is that a
limitation
of the build servers?  Having Jenkins automatically verify your
patchset
is IMO one of the nice feature of Gerrit, and it will lower the
workload
of core devs if building and testing are done before they start looking
at the patchset.
Build can be triggered by patchset submissin, too, but it would require
more build server resources. Usually not the first version of the
changeset will be accepted especially from new contributors and this
means
more builds.

My view is the opposite: New contributors patchsets will probably be
rejected
anyway (does not build, does not pass test, etc...), so having the system
doing that lowers the burden on core developers, who they can focus on
more
high level problems.

Note that Core Developers would not have to wait since they can give +1
for
their own changesets.

The other reason behind requiring a +1 from someone we trust is that
otherwise it would be easy to prepare a changeset which does unspeakable
things to the build servers which we don't want to happen. Without
requiring +1 we would have to prepare build systems to cope with
malicious
commits.

That is a good point (basically because of the halting problem).  But
builds
are done in isolation (i.e a git clone is done each time), so apart
using too
much resources or never ending, there is no harm that can be done to the
infrastructure.  And there is a Jenkins plugin to abort a build if it is
stuck.
I was concerned about using the buildbots for attacking other systems, too,
but all of those threats can be handled and we have time for setting
up build bots
properly.

I have updated the proposal to start tests for every change and allow
Code Devs to
bypass peer review.

Comments are still welcome. :-)

Cheers,
Balint


Re the possible change to require peer review of all changes.

My experience of this was in my day job, which involves a commercial
codebase with a long history (20+ years) and 15 or so full time devs.  The
product is a toolkit which means high flexibility and an inability to test
(in a practical length of time) all possible scenarios that customers might
create.  Many of the devs have been with the company for a long time and
have huge experience of the codebase, and of programming in general.

About 8 years ago we switched from a direct commit to trunk to a branch and
peer review and merge to trunk model very similar to that proposed for
Gerrit but using svn and our customised version of a commercial bug
tracking system.

Code quality and product quality has risen dramatically over time.  One
factor was that we didn't have a continuous build system so defects
introduced by changes were either discovered by devs during other work or
during regression testing before a release or by customers.  We have
detailed statistics backing up my anecdotes showing the rate at which
changes cause further defects and amazingly we can now "predict" future
failure rates based on code churn and current failure rates.

Code inspection by peers improved code quality, prevented "collateral
damage" where the immediate problem was fixed but the change broke
something else and we all believe produces better quality solutions to
issues and features.  Unfortunately devs are human and we still manage to
incorporate bugs when making changes so it doesn't fix all ills.

Peer review does add a cost but for our commercial product the benefits far
outweigh the costs.  For Wireshark I'm convinced there will be benefits,
but the cost is harder to quantify. In a commercial setting devs are tasked
for peer review but in our ad-hoc Wireshark world this causes me a little
concern as we would need to get dev traction on getting peer review done.
 IMHO allowing devs to short-circuit per review cuts any benefits gained
almost to be point of why bother with peer review?

tldr;

Peer review has benefits, are they worthwhile for wireshark.
___________________________________________________________________________
Sent via:    Wireshark-dev mailing list <wireshark-dev () wireshark org>
Archives:    http://www.wireshark.org/lists/wireshark-dev
Unsubscribe: https://wireshark.org/mailman/options/wireshark-dev
             mailto:wireshark-dev-request () wireshark org?subject=unsubscribe

  By Date           By Thread  

Current thread:
[ Nmap | Sec Tools | Mailing Lists | Site News | About/Contact | Advertising | Privacy ]
AlienVault