Open Stack

I am gonna add more color to this story by posting my replies on review [1]:

Hi Angus,

You touched on a number of points. Let me try to give you an answer to all
of them.

>> (I'll create a bug report too. I still haven't worked out which class of
changes need an accompanying bug report and which don't.)

The long story can be read below:

https://wiki.openstack.org/wiki/BugFilingRecommendations

https://wiki.openstack.org/wiki/GitCommitMessages

IMO, there's a grey area for some of the issues you found, but when I am
faced with a bug, I tend to answer myself? Would a bug report be useful to
someone else? The author of the code? A consumer of the code? Not everyone
follow the core review system all the time, whereas Launchpad is pretty
much the tool everyone uses to stay abreast with the OpenStack release
cycle. Obviously if you're fixing a grammar nit, or filing a cosmetic
change that has no functional impact then I warrant the lack of a test, but
in this case you're fixing a genuine error: let's say we want to backport
this to icehouse, how else would we make the release manager of that?
He/she is looking at Launchpad.

>> I can add a unittest for this particular code path, but it would only
check this particular short segment of code, would need to be maintained as
the code changes, and wouldn't catch another occurrence somewhere else.
This seems an unsatisfying return on the additional work :(

You're looking at this from the wrong perspective. This is not about
ensuring that other code paths are valid, but that this code path stays
valid over time, ensuring that the code path is exercised and that no other
regression of any kind creep in. The reason why this error was introduced
in the first place is because the code wasn't tested when it should have.
If you don't get that this mechanical effort of fixing errors by static
analysis is kind of ineffective, which leads me to the last point....

>> I actually found this via static analysis using pylint - and my question
is: should I create some sort of pylint unittest that tries to catch this
class of problem across the entire codebase? [...]

I value what you're doing, however I would see even more value if we
prevented these types of errors from occurring in the first place via
automation. You run pylint today, but what about tomorrow, or a week from
now? Are you gonna be filing pylint fixes for ever? We might be better off
automating the check and catch these types of errors before they land in
the tree. This means that the work you are doing it two-pronged: a)
automate the detection of some failures by hooking this into tox.ini via
HACKING/pep8 or equivalent mechanism and b) file all the fixes that require
these validation tests to pass; c) everyone is happy, or at least they
should be.

I'd welcome to explore a better strategy to ensure a better quality of the
code base, without some degree of automation, nothing will stop these
conversation from happening again.

Cheers,

Armando

[1] https://review.openstack.org/#/c/113777/


On 13 August 2014 03:02, Ihar Hrachyshka <ihrachys at redhat.com> wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
> On 13/08/14 09:28, Angus Lees wrote:
> > I'm doing various small cleanup changes as I explore the neutron
> > codebase. Some of these cleanups are to fix actual bugs discovered
> > in the code.  Almost all of them are tiny and "obviously correct".
> >
> > A recurring reviewer comment is that the change should have had an
> >  accompanying bug report and that they would rather that change was
> > not submitted without one (or at least, they've -1'ed my change).
> >
> > I often didn't discover these issues by encountering an actual
> > production issue so I'm unsure what to include in the bug report
> > other than basically a copy of the change description.  I also
> > haven't worked out the pattern yet of which changes should have a
> > bug and which don't need one.
> >
> > There's a section describing blueprints in NeutronDevelopment but
> > nothing on bugs.  It would be great if someone who understands the
> > nuances here could add some words on when to file bugs: Which type
> > of changes should have accompanying bug reports? What is the
> > purpose of that bug, and what should it contain?
> >
>
> It was discussed before at:
> http://lists.openstack.org/pipermail/openstack-dev/2014-May/035789.html
>
> /Ihar
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG/MacGPG2 v2.0.22 (Darwin)
>
> iQEcBAEBCgAGBQJT6zfOAAoJEC5aWaUY1u570wQIAMpoXIK/p5invp+GW0aMMUK0
> C/MR6WIJ83e6e2tOVUrxheK6bncVvidOI4EWGW1xzP1sg9q+8Hs1TNyKHXhJAb+I
> c435MMHWsDwj6p1OeDxHnSOVMthcGH96sgRa1+CIk6+oktDF3IMmiOPRkxdpqWCZ
> 7TkV75mryehrTNwAkVPfpWG3OhWO44d5lLnJFCIMCuOw2NHzyLIOoGQAlWNQpy4V
> a869s00WO37GEed6A5Zizc9K/05/6kpDIQVim37tw91JcZ69VelUlZ1THx+RTd33
> 92r87APm3fC/LioKN3fq1UUo2c94Vzl3gYPFVl8ZateQNMKB7ONMBePOfWR9H1k=
> =wCJQ
> -----END PGP SIGNATURE-----
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20140813/58b79988/attachment.html>