Re: [Spacewalk-devel] On quality of patches
On 3/4/14, 12:31 AM, Silvio Moioli smoi...@suse.de wrote: From: Paul Robert Marino prmari...@gmail.com mailto:prmari...@gmail.com I think we need a map of what these shared file and functions effect so we can do more formal QA testing in the future On 03/03/2014 08:09 PM, Lamont Peterson wrote: Agreed. Unfortunately I think such a map, if ever existed, would be far too complex to be of any practical value. Consider this image, which maps only database tables, a relatively small fraction of the total software complexity: http://turing.suse.de/~smoioli/relationships.real.compact.png Developers already have tools in modern IDEs such as Eclipse to check for (partial) call trees[1], and are expected to use them. I do not think we can do much more than that in this regard, of course new opinions are welcome :-) Cheers, [1] http://eclipse-tools.sourceforge.net/call-hierarchy/usage.html -- Silvio Moioli SUSE LINUX Products GmbH Maxfeldstraße 5, 90409 Nürnberg Germany ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel You’re right (and thanks for the vivid DB relationships diagram). In my mind, the auto-generated map should simply be a calling-me list: Function A is called by: file.java : 45 file2.java : 134 That’s for developers. The map for QA could be simpler and generated from merely stuff one can hit “submit” on? -- Lamont Peterson Sr. Systems Administrator | Unix Systems Operations Intermountain Healthcare Office: 801.442.6497 | lamont.peter...@imail.org ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] On quality of patches
On 1/30/14, 3:57 AM, Matej Kollar mkol...@redhat.com wrote: Hi all. We welcome community contributions and we want to maintain some level of quality. Natural expectation is that every proposed patch is tested for the functionality. However, recently I came across quite nasty little thing in Spacewalk... Using SSM to schedule reboot one might end up very unpleasantly surprised, expecting it not to occur for specified period of time, when in fact it is being scheduled as soon as possible, no matter what you try. (There was no way it ever worked - parameters for forms were not passed to script in request (where DatePicker looks for them) and new Date object was returned. Well, someone scamped his work, so I had to do it for him last night. And I consider myself lucky that I found out... As en exercise in applied imagination, try to imagine OSAD running on such machine. To stretch your imagination even further, imagine the machine acting as a life supporting computer during child surgery...) Nice example :) . FYI, around here (Intermountain Healthcare), any activity requiring a planned reboot of a server would also require an SA to be on “live” and monitoring the system and would likely involve a DBA or App Admin to be online to confirm all services are properly started/running. Therefore, this example, while great, wouldn’t happen in our industry because we would never use such functionality (yes, mistakes can happen). BTW, there are systems that can directly impact patient care in general bed space, ICU, NICU, ED, OR and other facilities. So this example is an excellent idea in general to keep in mind. Yes, most systems in the world will not cause harm to life, limb or property should something go wrong, but there are a lot of places where it could (not just health care, but think about Air Traffic Control, City-wide Traffic Management, National Weather Service, Automated Industrial/Manufacturing and the list could go on and on). In the other end of the scale, what if something went sideways in a small company? People with 10 servers to manage or 100 servers and 5 employees can go out of business if their stuff is down/broken (by systems management software, like Spacewalk?). That’s a huge impact to their worlds. Btw: next time have a peek into struts-config.xml and look for form-bean and form-property tags... say like in eea3c6320bfc3dae8532844a7c0e71dcc5712c39. To continue with our story: Purpose of that form is to schedule action to occur not sooner than. It fails on all but one particular case. It simply cold not have been tested. Reboot is considered destructive operation. Therefore we insisted on two-step scheduling for this feature. I fixed this one but wonder how many other similar features there were recently introduced. And the final rhetorical question: Have the author even tried it before submitting the patch? I tend to doubt it. Matej ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel Thanks! Keep up the great work. -- Lamont Peterson Sr. Systems Administrator | Unix Systems Operations Intermountain Healthcare Office: 801.442.6497 ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] On quality of patches
Agreed. Such a map needs to be generated from source, please? -- Lamont Peterson Sr. Systems Administrator | Unix Systems Operations [cid:image002.jpg@01CEC98D.A33033E0]http://intermountainhealthcare.org/ From: Paul Robert Marino prmari...@gmail.commailto:prmari...@gmail.com Reply-To: spacewalk-devel@redhat.commailto:spacewalk-devel@redhat.com spacewalk-devel@redhat.commailto:spacewalk-devel@redhat.com Date: Thursday, January 30, 2014 at 9:50 AM To: spacewalk-devel@redhat.commailto:spacewalk-devel@redhat.com spacewalk-devel@redhat.commailto:spacewalk-devel@redhat.com Subject: Re: [Spacewalk-devel] On quality of patches There is an other factor here that no one has mentioned. Recently I've found and reported a lot of bugs in the nightly repo. Where they came from is usually some one updates a portion of the code to fix a specific issue but since a portion of that code is called by multiple parts of the web interface it may break other pages than the one the developer is focused on. I think we need a map of what these shared file and functions effect so we can do more formal QA testing in the future -- Sent from my HP Pre3 On Jan 30, 2014 7:33, Duncan Mac-Vicar P. dmacvi...@suse.demailto:dmacvi...@suse.de wrote: On 30/01/14 11:57, Matej Kollar wrote: Hi all. We welcome community contributions and we want to maintain some level of quality. Natural expectation is that every proposed patch is tested for the functionality. I fully agree with you that this is not acceptable. Now, I think your diagnostic is IMHO neither fair or correct. This does not have to do with the community contributions but with the setup of the project itself. We sometimes have sent patches that are very strictly reviewed, sometimes needing change multiple times to get a reviewer happy. Then next day our internal testsuite fails only to realize that someone with direct commit access did a big refactoring and committed very broken code in a big patch that was not reviewed by anyone and which quality was also obviously not the best. We asked ourselves, what is the point of reviewing some of them?. Not only code but also design decisions and way of approaching solutions. Sometimes this happens with our own patches, depending on the reviewer, they may get committed faster. The setup of the review process is broken. - All code should be committed in similar units (features/branches) - All code should be able to be reviewed and vetoed by everyone OpenStack has this model working quite successfully. Every patch is reviewed with +1, and they need a certain amounts of ACKS to get committed. Everyone can review and people learn in the process, and it is a great source of inspiration for other projects. We care about quality and you can see that we not only contributed our own testsuite in early 2011 with the first release of our own product, but then focused lot of contributions in having the testsuites enabled and working again. But all this is pointless if the process has holes. Right now it depends on a lot of luck to be useful. We suggested not long time ago in the mailing list to move to a github approach where code could be submitted only with pull-requests that need to be reviewed. Policies could be created that at least 2-3 acks is needed to merge a feature. People can study those reviews and learn in the process. Continuous integration could be setup on top of this process, to have up to date results. But what you are seeing, the process is kind of designed to produce those results, which is a process where code has different ways to arrive to master, which different quality outcomes. -- Duncan Mac-Vicar P. - http://www.suse.com/ SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) Maxfeldstraße 5, 90409 Nürnberg, Germany ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.commailto:Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel inline: 43D9F316-7150-4FD9-97C0-94E6B1732B1F[96].png___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
[Spacewalk-devel] On quality of patches
Hi all. We welcome community contributions and we want to maintain some level of quality. Natural expectation is that every proposed patch is tested for the functionality. However, recently I came across quite nasty little thing in Spacewalk... Using SSM to schedule reboot one might end up very unpleasantly surprised, expecting it not to occur for specified period of time, when in fact it is being scheduled as soon as possible, no matter what you try. (There was no way it ever worked - parameters for forms were not passed to script in request (where DatePicker looks for them) and new Date object was returned. Well, someone scamped his work, so I had to do it for him last night. And I consider myself lucky that I found out... As en exercise in applied imagination, try to imagine OSAD running on such machine. To stretch your imagination even further, imagine the machine acting as a life supporting computer during child surgery...) Btw: next time have a peek into struts-config.xml and look for form-bean and form-property tags... say like in eea3c6320bfc3dae8532844a7c0e71dcc5712c39. To continue with our story: Purpose of that form is to schedule action to occur not sooner than. It fails on all but one particular case. It simply cold not have been tested. Reboot is considered destructive operation. Therefore we insisted on two-step scheduling for this feature. I fixed this one but wonder how many other similar features there were recently introduced. And the final rhetorical question: Have the author even tried it before submitting the patch? I tend to doubt it. Matej ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] On quality of patches
On 30/01/14 11:57, Matej Kollar wrote: Hi all. We welcome community contributions and we want to maintain some level of quality. Natural expectation is that every proposed patch is tested for the functionality. I fully agree with you that this is not acceptable. Now, I think your diagnostic is IMHO neither fair or correct. This does not have to do with the community contributions but with the setup of the project itself. We sometimes have sent patches that are very strictly reviewed, sometimes needing change multiple times to get a reviewer happy. Then next day our internal testsuite fails only to realize that someone with direct commit access did a big refactoring and committed very broken code in a big patch that was not reviewed by anyone and which quality was also obviously not the best. We asked ourselves, what is the point of reviewing some of them?. Not only code but also design decisions and way of approaching solutions. Sometimes this happens with our own patches, depending on the reviewer, they may get committed faster. The setup of the review process is broken. - All code should be committed in similar units (features/branches) - All code should be able to be reviewed and vetoed by everyone OpenStack has this model working quite successfully. Every patch is reviewed with +1, and they need a certain amounts of ACKS to get committed. Everyone can review and people learn in the process, and it is a great source of inspiration for other projects. We care about quality and you can see that we not only contributed our own testsuite in early 2011 with the first release of our own product, but then focused lot of contributions in having the testsuites enabled and working again. But all this is pointless if the process has holes. Right now it depends on a lot of luck to be useful. We suggested not long time ago in the mailing list to move to a github approach where code could be submitted only with pull-requests that need to be reviewed. Policies could be created that at least 2-3 acks is needed to merge a feature. People can study those reviews and learn in the process. Continuous integration could be setup on top of this process, to have up to date results. But what you are seeing, the process is kind of designed to produce those results, which is a process where code has different ways to arrive to master, which different quality outcomes. -- Duncan Mac-Vicar P. - http://www.suse.com/ SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) Maxfeldstraße 5, 90409 Nürnberg, Germany ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] On quality of patches
On 01/30/2014 07:32 AM, Duncan Mac-Vicar P. wrote: I fully agree with you that this is not acceptable. Now, I think your diagnostic is IMHO neither fair or correct. This does not have to do with the community contributions but with the setup of the project itself. We sometimes have sent patches that are very strictly reviewed, sometimes needing change multiple times to get a reviewer happy. Then next day our internal testsuite fails only to realize that someone with direct commit access did a big refactoring and committed very broken code in a big patch that was not reviewed by anyone and which quality was also obviously not the best. We asked ourselves, what is the point of reviewing some of them?. Not only code but also design decisions and way of approaching solutions. Sometimes this happens with our own patches, depending on the reviewer, they may get committed faster. The setup of the review process is broken. - All code should be committed in similar units (features/branches) - All code should be able to be reviewed and vetoed by everyone OpenStack has this model working quite successfully. Every patch is reviewed with +1, and they need a certain amounts of ACKS to get committed. Everyone can review and people learn in the process, and it is a great source of inspiration for other projects. For what it's worth I agree with Duncan. I think that having a consistent process requiring multiple reviews for all changes could only improve code quality and make Spacewalk more accessible to community contributors. The process Duncan describes is very common among modern open source projects and seems to work well. -Stpehen ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] On quality of patches
There is an other factor here that no one has mentioned. Recently I've found and reported a lot of bugs in the nightly repo. Where they came from is usually some one updates a portion of the code to fix a specific issue but since a portion of that code is called by multiple parts of the web interface it may break other pages than the one the developer is focused on.I think we need a map of what these shared file and functions effect so we can do more formal QA testing in the future-- Sent from my HP Pre3On Jan 30, 2014 7:33, Duncan Mac-Vicar P. dmacvi...@suse.de wrote: On 30/01/14 11:57, Matej Kollar wrote: Hi all. We welcome community contributions and we want to maintain some level of quality. Natural expectation is that every proposed patch is tested for the functionality. I fully agree with you that this is not acceptable. Now, I think your diagnostic is IMHO neither fair or correct. This does not have to do with the "community contributions" but with the setup of the project itself. We sometimes have sent patches that are very strictly reviewed, sometimes needing change multiple times to get a reviewer happy. Then next day our internal testsuite fails only to realize that someone with direct commit access did a big refactoring and committed very broken code in a big patch that was not reviewed by anyone and which quality was also obviously not the best. We asked ourselves, what is the point of reviewing "some of them"?. Not only code but also design decisions and way of approaching solutions. Sometimes this happens with our own patches, depending on the reviewer, they may get committed faster. The setup of the review process is broken. - All code should be committed in similar units (features/branches) - All code should be able to be reviewed and vetoed by everyone OpenStack has this model working quite successfully. Every patch is reviewed with +1, and they need a certain amounts of ACKS to get committed. Everyone can review and people learn in the process, and it is a great source of inspiration for other projects. We care about quality and you can see that we not only contributed our own testsuite in early 2011 with the first release of our own product, but then focused lot of contributions in having the testsuites enabled and working again. But all this is pointless if the process has holes. Right now it depends on a lot of luck to be useful. We suggested not long time ago in the mailing list to move to a github approach where code could be submitted only with pull-requests that need to be reviewed. Policies could be created that at least 2-3 acks is needed to merge a feature. People can study those reviews and learn in the process. Continuous integration could be setup on top of this process, to have up to date results. But what you are seeing, the process is kind of designed to produce those results, which is a process where code has different ways to arrive to master, which different quality outcomes. -- Duncan Mac-Vicar P. - http://www.suse.com/ SUSE LINUX Products GmbH, GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer, HRB 16746 (AG Nürnberg) Maxfeldstraße 5, 90409 Nürnberg, Germany ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
Re: [Spacewalk-devel] On quality of patches
-Original Message- From: spacewalk-devel-boun...@redhat.com [mailto:spacewalk-devel-boun...@redhat.com] On Behalf Of Stephen Herr Sent: 30 January 2014 13:33 To: spacewalk-devel@redhat.com Subject: Re: [Spacewalk-devel] On quality of patches On 01/30/2014 07:32 AM, Duncan Mac-Vicar P. wrote: I fully agree with you that this is not acceptable. Now, I think your diagnostic is IMHO neither fair or correct. This does not have to do with the community contributions but with the setup of the project itself. We sometimes have sent patches that are very strictly reviewed, sometimes needing change multiple times to get a reviewer happy. Then next day our internal testsuite fails only to realize that someone with direct commit access did a big refactoring and committed very broken code in a big patch that was not reviewed by anyone and which quality was also obviously not the best. We asked ourselves, what is the point of reviewing some of them?. Not only code but also design decisions and way of approaching solutions. Sometimes this happens with our own patches, depending on the reviewer, they may get committed faster. The setup of the review process is broken. - All code should be committed in similar units (features/branches) - All code should be able to be reviewed and vetoed by everyone OpenStack has this model working quite successfully. Every patch is reviewed with +1, and they need a certain amounts of ACKS to get committed. Everyone can review and people learn in the process, and it is a great source of inspiration for other projects. For what it's worth I agree with Duncan. I think that having a consistent process requiring multiple reviews for all changes could only improve code quality and make Spacewalk more accessible to community contributors. The process Duncan describes is very common among modern open source projects and seems to work well. -Stpehen I can't agree more with Duncan on this (at least one Duncan talks sense!). Duncan This message has been checked for viruses and spam by the Virgin Money email scanning system powered by Messagelabs. This e-mail is intended to be confidential to the recipient. If you receive a copy in error, please inform the sender and then delete this message. Virgin Money plc - Registered in England and Wales (Company no. 6952311). Registered office - Jubilee House, Gosforth, Newcastle upon Tyne NE3 4PL. Virgin Money plc is authorised by the Prudential Regulation Authority and regulated by the Financial Conduct Authority and the Prudential Regulation Authority. The following companies also trade as Virgin Money. They are both authorised and regulated by the Financial Conduct Authority, are registered in England and Wales and have their registered office at Jubilee House, Gosforth, Newcastle upon Tyne NE3 4PL: Virgin Money Personal Financial Service Limited (Company no. 3072766) and Virgin Money Unit Trust Managers Limited (Company no. 3000482). For further details of Virgin Money group companies please visit our website at virginmoney.com ___ Spacewalk-devel mailing list Spacewalk-devel@redhat.com https://www.redhat.com/mailman/listinfo/spacewalk-devel
