> -----Original Message----- > From: rohityada...@gmail.com [mailto:rohityada...@gmail.com] On Behalf > Of Rohit Yadav > Sent: vrijdag 22 februari 2013 7:37 > To: cloudstack-dev@incubator.apache.org > Subject: [DISCUSS] Code Reviewing process (was Re: Build failed in Jenkins: > cloudstack-rat-41 #58) > > This and few regressions won't have happened if there was a reviewing > process.
That depends on the reviewer. The people we trust to do these reviews are the same people with commit rights to the tree who are currently pushing in the changes. What makes you think that having a reviewing process would stop us from pushing in the same changes that we do now? You see it happening on the review board as well, we agreed that we would not accept reviews without tests and I forget that on occasion and approve a review without. > > Skip the rant: > I don't know why people are scared of good engineering practices, they just > need to be educated. Depends on ones point of reference. In my book good engineering practices is making sure that anything you push into a shared tree is the highest quality you can deliver. > They would claim that reviewing process would slow us down, yeah but fixing > regressions, builds, bugs introduced due to people checking in stuff does not > slow us down right? And sometimes it does not take us days to fix those > issues, cause fixing them is so easy and painless. > NO it's not and it's not cool. Yes it does slow us down, and pushing for greater quality will slow us down even more. I guess we have to accept that while we work on improving the quality of the submissions and the code in general. That's why we all agreed to focus on testing. I don't like to have to go in and fix stuff other people broke or even reverting commits when I can't fix it fast enough, but if that is what it takes I'll gladly do it. If some committer wants to have his or her code reviewed before pushing it to master I would be more than happy to do the review, but it should be optional. I believe that if we all want to quality to be better we can make it work by helping each other out. We can't fix a people issue with a technical solution, the only way to fix those is by talking. And if that doesn't seem to work, Jenkins does keep track of who broke the build. We should not be afraid to self-police and hold each other accountable broken builds. That's a community thing and a very important one, we are all in this together. Practically we can do a lot to help with this process I think, we need a lot more unit tests, not just for the sake of testing but also to show how it is done. Not every developer is familiar with them and that is something that the people who are can help out with. Prove our points with examples, don't push in any change without a unittest and a functional test description. If we go in an fix a bug, make sure it's tested for so we won't have that regression later on. Yes lots of more work and working on new features is a lot more sexy, but if we are that concerned about quality, that is what we should do in my opinion. I'm in complete agreement with you and share the same frustration about the state of master some times. Our proposed methods might differ, but that's just a personal view on things :-) > > Regards. > > On Fri, Feb 22, 2013 at 8:58 AM, Chip Childers <chip.child...@sungard.com> > wrote: > > Can someone please correct this? > > > > On Feb 21, 2013, at 10:03 PM, Apache Jenkins Server > > <jenk...@builds.apache.org> wrote: > > > >> See <https://builds.apache.org/job/cloudstack-rat-41/58/changes> > >> > >> Changes: > >> > >> [kelveny] CLOUDSTACK-1362: Put a workaround fix to set excutable > >> attribute of injectkys.sh at runtime > >> > >> [sheng.yang] CLOUDSTACK-1288: Fix regression on remove LB rules > >> > >> [prachi] CLOUDSTACK-1362: EC2 dns-name filter support for EC2 > >> describeInstances API is broken > >> > >> [frank.zhang] CloudStack CLOUDSTACK-774 > >> > >> [frank.zhang] CloudStack CLOUDSTACK-774 > >> > >> [frank.zhang] CloudStack CLOUDSTACK-774 > >> > >> [frank.zhang] CloudStack CLOUDSTACK-774 > >> > >> [frank.zhang] CloudStack CLOUDSTACK-774 > >> > >> [sheng.yang] CLOUDSTACK-1303: Fix NPE when extend vlan with ipv4 only > >> > >> [jessica.wang] CLOUDSTACK-1319: cloudstack API - > CreateVpnCustomerGateway API: correct parameter type on server-side. > >> > >> [prachi] CLOUDSTACK-1367 NPE noticed in logs while AgentMonitor is > >> monitoring the host ping interval > >> > >> ------------------------------------------ > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Started by an SCM change > >> Building remotely on ubuntu3 in workspace > >> <https://builds.apache.org/job/cloudstack-rat-41/ws/> > >> Checkout:cloudstack-rat-41 / > >> <https://builds.apache.org/job/cloudstack-rat-41/ws/> - > >> hudson.remoting.Channel@15654a28:ubuntu3 > >> Using strategy: Default > >> Last Built Revision: Revision > >> aea5b268b4590775eff6291ddfbbd6de777d1b63 (origin/4.1) Fetching > >> changes from 1 remote Git repository Fetching upstream changes from > >> https://git-wip-us.apache.org/repos/asf/incubator-cloudstack.git > >> Commencing build of Revision > 96a98f5a71533e7f577b88f0b6116671b97c380a > >> (origin/4.1) Checking out Revision > >> 96a98f5a71533e7f577b88f0b6116671b97c380a (origin/4.1) > >> [cloudstack-rat-41] $ /bin/bash -xe /tmp/hudson2180066406118557127.sh > >> + /home/jenkins/tools/maven/latest2/bin/mvn > >> + --projects=org.apache.cloudstack:cloudstack > >> + org.apache.rat:apache-rat-plugin:0.8:check > >> [INFO] Scanning for projects... > >> [INFO] > >> --------------------------------------------------------------------- > >> --- > >> [INFO] Building Apache CloudStack > >> [INFO] task-segment: [org.apache.rat:apache-rat-plugin:0.8:check] > >> [INFO] > >> --------------------------------------------------------------------- > >> --- [INFO] [apache-rat:check {execution: default-cli}] [INFO] > >> Exclude: CHANGES [INFO] Exclude: INSTALL.md [INFO] Exclude: .idea/ > >> [INFO] Exclude: *.log [INFO] Exclude: **/*.patch [INFO] Exclude: > >> **/.classpath [INFO] Exclude: **/.project [INFO] Exclude: **/*.iml > >> [INFO] Exclude: **/.settings/** [INFO] Exclude: .metadata/** [INFO] > >> Exclude: .git/** [INFO] Exclude: .gitignore [INFO] Exclude: **/*.crt > >> [INFO] Exclude: **/*.csr [INFO] Exclude: **/*.key [INFO] Exclude: > >> **/authorized_keys [INFO] Exclude: **/*.war [INFO] Exclude: **/*.mar > >> [INFO] Exclude: **/*.jar [INFO] Exclude: **/*.iso [INFO] Exclude: > >> **/*.tgz [INFO] Exclude: **/*.zip [INFO] Exclude: **/target/** [INFO] > >> Exclude: **/.vagrant [INFO] Exclude: build/build.number [INFO] > >> Exclude: console-proxy/js/jquery.js [INFO] Exclude: debian/compat > >> [INFO] Exclude: debian/control [INFO] Exclude: debian/dirs [INFO] > >> Exclude: debian/rules [INFO] Exclude: > >> deps/XenServerJava/src/com/xensource/xenapi/*.java > >> [INFO] Exclude: deps/XenServerJava/BSD [INFO] Exclude: > >> deps/XenServerJava/Makefile [INFO] Exclude: > >> dist/console-proxy/js/jquery.js [INFO] Exclude: > >> scripts/vm/systemvm/id_rsa.cloud [INFO] Exclude: > >> tools/devcloud/basebuild/puppet-devcloudinitial/files/network.conf > >> [INFO] Exclude: tools/appliance/definitions/systemvmtemplate/base.sh > >> [INFO] Exclude: > >> tools/appliance/definitions/systemvmtemplate/cleanup.sh > >> [INFO] Exclude: > >> tools/appliance/definitions/systemvmtemplate/definition.rb > >> [INFO] Exclude: > >> tools/appliance/definitions/systemvmtemplate/preseed.cfg > >> [INFO] Exclude: > >> tools/appliance/definitions/systemvmtemplate/zerodisk.sh > >> [INFO] Exclude: > >> tools/devcloud/src/deps/boxes/basebox-build/definition.rb > >> [INFO] Exclude: > >> tools/devcloud/src/deps/boxes/basebox-build/preseed.cfg > >> [INFO] Exclude: ui/lib/flot/jquery.colorhelpers.js > >> [INFO] Exclude: ui/lib/flot/jquery.flot.crosshair.js > >> [INFO] Exclude: ui/lib/flot/jquery.flot.fillbetween.js > >> [INFO] Exclude: ui/lib/flot/jquery.flot.image.js [INFO] Exclude: > >> ui/lib/flot/jquery.flot.js [INFO] Exclude: > >> ui/lib/flot/jquery.flot.navigate.js > >> [INFO] Exclude: ui/lib/flot/jquery.flot.pie.js [INFO] Exclude: > >> ui/lib/flot/jquery.flot.resize.js [INFO] Exclude: > >> ui/lib/flot/jquery.flot.selection.js > >> [INFO] Exclude: ui/lib/flot/jquery.flot.stack.js [INFO] Exclude: > >> ui/lib/flot/jquery.flot.symbol.js [INFO] Exclude: > >> ui/lib/flot/jquery.flot.threshold.js > >> [INFO] Exclude: ui/lib/jquery-ui/css/jquery-ui.css > >> [INFO] Exclude: ui/lib/jquery-ui/index.html [INFO] Exclude: > >> ui/lib/jquery-ui/js/jquery-ui.js [INFO] Exclude: > >> ui/lib/jquery.cookies.js [INFO] Exclude: ui/lib/jquery.easing.js > >> [INFO] Exclude: ui/lib/jquery.js [INFO] Exclude: ui/lib/jquery.md5.js > >> [INFO] Exclude: ui/lib/jquery.validate.js [INFO] Exclude: > >> ui/lib/qunit/qunit.css [INFO] Exclude: ui/lib/qunit/qunit.js [INFO] > >> Exclude: ui/lib/reset.css [INFO] Exclude: waf [INFO] Exclude: > >> patches/systemvm/debian/systemvm.vmx > >> [INFO] Exclude: > >> patches/systemvm/debian/config/root/.ssh/authorized_keys > >> [INFO] Exclude: > patches/systemvm/debian/config/etc/apache2/httpd.conf > >> [INFO] Exclude: > patches/systemvm/debian/config/etc/apache2/ports.conf > >> [INFO] Exclude: > >> patches/systemvm/debian/config/etc/apache2/sites-available/default > >> [INFO] Exclude: > >> patches/systemvm/debian/config/etc/apache2/sites-available/default-ss > >> l [INFO] Exclude: > >> patches/systemvm/debian/config/etc/apache2/vhostexample.conf > >> [INFO] Exclude: patches/systemvm/debian/config/etc/dnsmasq.conf > >> [INFO] Exclude: patches/systemvm/debian/config/etc/vpcdnsmasq.conf > >> [INFO] Exclude: patches/systemvm/debian/config/etc/ssh/sshd_config > >> [INFO] Exclude: patches/systemvm/debian/config/etc/rsyslog.conf > >> [INFO] Exclude: patches/systemvm/debian/config/etc/logrotate.conf > >> [INFO] Exclude: patches/systemvm/debian/config/etc/logrotate.d/* > >> [INFO] Exclude: patches/systemvm/debian/config/etc/sysctl.conf > >> [INFO] Exclude: > >> > patches/systemvm/debian/config/root/redundant_router/keepalived.conf. > >> templ [INFO] Exclude: > >> > patches/systemvm/debian/config/root/redundant_router/arping_gateways > . > >> sh.templ [INFO] Exclude: > >> > patches/systemvm/debian/config/root/redundant_router/conntrackd.conf. > >> templ [INFO] Exclude: patches/systemvm/debian/vpn/etc/ipsec.conf > >> [INFO] Exclude: patches/systemvm/debian/vpn/etc/ppp/options.xl2tpd > >> [INFO] Exclude: patches/systemvm/debian/vpn/etc/xl2tpd/xl2tpd.conf > >> [INFO] Exclude: patches/systemvm/debian/vpn/etc/ipsec.secrets > >> [INFO] Exclude: > >> patches/systemvm/debian/config/etc/haproxy/haproxy.cfg > >> [INFO] Exclude: patches/systemvm/debian/config/etc/cloud-nic.rules > >> [INFO] Exclude: > >> patches/systemvm/debian/config/etc/modprobe.d/aesni_intel > >> [INFO] Exclude: patches/systemvm/debian/config/etc/rc.local > >> [INFO] Exclude: > >> patches/systemvm/debian/config/var/www/html/userdata/.htaccess > >> [INFO] Exclude: > >> patches/systemvm/debian/config/var/www/html/latest/.htaccess > >> [INFO] Exclude: patches/systemvm/debian/vpn/etc/ipsec.d/l2tp.conf > >> [INFO] > >> --------------------------------------------------------------------- > >> --- > >> [ERROR] BUILD FAILURE > >> [INFO] > >> --------------------------------------------------------------------- > >> --- [INFO] Too many unapproved licenses: 6 [INFO] > >> --------------------------------------------------------------------- > >> --- [INFO] For more information, run Maven with the -e switch [INFO] > >> --------------------------------------------------------------------- > >> --- > >> [INFO] Total time: 14 seconds > >> [INFO] Finished at: Fri Feb 22 02:54:11 UTC 2013 [INFO] Final Memory: > >> 19M/377M [INFO] > >> --------------------------------------------------------------------- > >> --- Build step 'Execute shell' marked build as failure Archiving > >> artifacts > >>
