Hugo, I just want some better engineering process than the current one. Yes, we can disagree and have different views on the same thing. Nonetheless, I just want us to find a better solution, workflow than the current one, just want to help.
Regards. On Fri, Feb 22, 2013 at 3:44 PM, Hugo Trippaers <htrippa...@schubergphilis.com> wrote: > > >> -----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 >> >>
