Re: [openstack-dev] [neutron] Which changes need accompanying bugs?
On 08/14/2014 11:37 AM, Ihar Hrachyshka wrote:
On 14/08/14 02:43, Angus Lees wrote:
On Wed, 13 Aug 2014 11:11:51 AM Kevin Benton wrote:
Is the pylint static analysis that caught that error prone to
false positives? If not, I agree that it would be really nice if
that were made part of the tox check so these don't have to be
fixed after the fact.
At the moment pylint on neutron is *very* noisy, and I've been
looking through the reported issues by hand to get a feel for
what's involved. Enabling pylint is a separate discussion that I'd
like to have - in some other thread.
Just start with non-voting check. Once all the issues are fixed, we
can enable it as voting.
Or blacklist all failures, make it voting - and then fix one issue after
the other - and with each patch, remove the blacklisted number.
That way no regressions come in...
Andreas
--
Andreas Jaeger aj@{suse.com,opensuse.org} Twitter/Identica: jaegerandi
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn,Jennifer Guild,Felix Imendörffer,HRB16746 (AG Nürnberg)
GPG fingerprint = 93A3 365E CE47 B889 DF7F FED1 389A 563C C272 A126
___
OpenStack-dev mailing list
OpenStack-dev@lists.openstack.org
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [neutron] Which changes need accompanying bugs?
On Aug 13, 2014, at 11:11 AM, Kevin Benton blak...@gmail.com wrote: Is the pylint static analysis that caught that error prone to false positives? If not, I agree that it would be really nice if that were made part of the tox check so these don't have to be fixed after the fact. To me that particular patch seems like one that should be accompanied with a unit test because it's a failure case with cleanup code that isn't being touched by the unit tests. +1 As a general rule I would like to see test addition included with any fix targeting a bug that was merged due to a lack of coverage. m. On Aug 13, 2014 8:34 AM, Armando M. arma...@gmail.com wrote: 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:
[openstack-dev] [neutron] Which changes need accompanying bugs?
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? -- Thanks, - Gus ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [neutron] Which changes need accompanying bugs?
I'm not sure what the guideline is, but I would like to point out a good reason to have the bug report even for obvious fixes. When users encounters bugs, they go to launchpad to report them. They don't first scan the commits of the master branch to see what was fixed. Having the bug in launchpad provides a way to track the status (fixed, backported, impact, etc) of the bug and reduces the chances of duplicated bugs. Can you provide an example of a patch that you felt was trivial that a reviewer requested a bug for so we have something concrete to discuss and establish guidelines around? On Aug 13, 2014 12:32 AM, Angus Lees g...@inodes.org 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? -- Thanks, - Gus ___ 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
Re: [openstack-dev] [neutron] Which changes need accompanying bugs?
-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
Re: [openstack-dev] [neutron] Which changes need accompanying bugs?
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
Re: [openstack-dev] [neutron] Which changes need accompanying bugs?
Is the pylint static analysis that caught that error prone to false positives? If not, I agree that it would be really nice if that were made part of the tox check so these don't have to be fixed after the fact. To me that particular patch seems like one that should be accompanied with a unit test because it's a failure case with cleanup code that isn't being touched by the unit tests. On Aug 13, 2014 8:34 AM, Armando M. arma...@gmail.com wrote: 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
Re: [openstack-dev] [neutron] Which changes need accompanying bugs?
On Wed, 13 Aug 2014 11:11:51 AM Kevin Benton wrote: Is the pylint static analysis that caught that error prone to false positives? If not, I agree that it would be really nice if that were made part of the tox check so these don't have to be fixed after the fact. At the moment pylint on neutron is *very* noisy, and I've been looking through the reported issues by hand to get a feel for what's involved. Enabling pylint is a separate discussion that I'd like to have - in some other thread. -- - Gus ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [neutron] Which changes need accompanying bugs?
On Wed, 13 Aug 2014 12:46:03 AM Kevin Benton wrote: I'm not sure what the guideline is, but I would like to point out a good reason to have the bug report even for obvious fixes. When users encounters bugs, they go to launchpad to report them. They don't first scan the commits of the master branch to see what was fixed. Having the bug in launchpad provides a way to track the status (fixed, backported, impact, etc) of the bug and reduces the chances of duplicated bugs. Aha, I didn't realise there were people interested in the state of the code but following only bugs and not review/commit logs. In particular release backporting. Good to know! Can you provide an example of a patch that you felt was trivial that a reviewer requested a bug for so we have something concrete to discuss and establish guidelines around? Here's one that fixes unittests in the presence of HTTP_PROXY: https://review.openstack.org/#/c/110853/ (I see I haven't filed the bug report for that yet, I'll do that now) .. and another that is removing an unused argument to a function: https://review.openstack.org/#/c/80/ I _think_ the intention here was to have a discussion of implementation impact on the bug rather than in the review(?) but that discussion hasn't moved (yet). To be clear: I'm perfectly happy to file an accompanying bug for each change (_every_ change if desired!). I want to make sure my accompanying bug reports are as useful as they can be, so I'm trying to learn how and when they're used. At the moment I'm basically waiting until a reviewer asks for one and then copying the change description and a prose version of the initial patch into a new bug report. -- - Gus ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Re: [openstack-dev] [neutron] Which changes need accompanying bugs?
On Wed, Aug 13, 2014 at 5:43 PM, Angus Lees g...@inodes.org wrote: On Wed, 13 Aug 2014 11:11:51 AM Kevin Benton wrote: Is the pylint static analysis that caught that error prone to false positives? If not, I agree that it would be really nice if that were made part of the tox check so these don't have to be fixed after the fact. At the moment pylint on neutron is *very* noisy, and I've been looking through the reported issues by hand to get a feel for what's involved. Enabling pylint is a separate discussion that I'd like to have - in some other thread. I think enabling pylint check was discussed at the start of the project, but for the reasons you mention, it was not considered. -- - Gus ___ 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
Re: [openstack-dev] [neutron] Which changes need accompanying bugs?
At the moment pylint on neutron is *very* noisy, and I've been looking through the reported issues by hand to get a feel for what's involved. Enabling pylint is a separate discussion that I'd like to have - in some other thread. I think enabling pylint check was discussed at the start of the project, but for the reasons you mention, it was not considered. Yes, noise can be a problem, but we should be able to adjust it to a level we're comfortable with, at least for catching the dangerous violations. ___ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
