incubator-cloudstack-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Hugo Trippaers <HTrippa...@schubergphilis.com>
Subject RE: [DISCUSS] Code Reviewing process (was Re: Build failed in Jenkins: cloudstack-rat-41 #58)
Date Fri, 22 Feb 2013 10:14:03 GMT


> -----Original Message-----
> From: rohityadav89@gmail.com [mailto:rohityadav89@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.childers@sungard.com>
> wrote:
> > Can someone please correct this?
> >
> > On Feb 21, 2013, at 10:03 PM, Apache Jenkins Server
> > <jenkins@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
> >>

Mime
View raw message