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 <ihrac...@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@lists.openstack.org > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >
_______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
