Chip, I support what you're saying here but want to offer up a compromise in the process to lessen the work. I'm sure Anthony will want to add unit tests. How about
- We file a bug for unit tests for 4.2. - Add the commits you said here to the bug. - Give Anthony a week to two weeks to add the unit tests. - If Anthony decides he doesn't have the bandwidth to add the unit tests in that time, then he has to revert and then he can use that bug to track adding back his changes along with unit tests. It saves some time on testing the revert and he can use the time instead to add unit tests. Will be happy to add the bug if that's okay with you and others on the list. --Alex > -----Original Message----- > From: Chip Childers [mailto:chip.child...@sungard.com] > Sent: Friday, February 08, 2013 7:48 AM > To: cloudstack-dev@incubator.apache.org; Anthony Xu > Subject: Re: [MERGE] Security Group in Advanced zone > > On Tue, Feb 5, 2013 at 2:20 PM, Chip Childers <chip.child...@sungard.com> > wrote: > > On Tue, Feb 05, 2013 at 10:50:17AM -0800, Anthony Xu wrote: > >> I would like to request merge of feature " security group in advanced > zone" > >> > > > > The latest update in the Jira record states that it's 70% complete. > > > > Also, the branch (sg-in-advanced-zone) seems to be quite different from > > the current state of master. I'd like everyone to have an opportunity > > to review the specific changes being proposed for the merge. > > > > Last request: what unit or integration tests are included to increase > > our automated test coverage? > > Anythony, > > I see that this was merged into master, but I never got any answers to > my questions above. > > I'm concerned about *yet another feature* without unit tests of the > new code. I understand that this is mostly changing existing classes, > but why not at least have some level of testing for the changes? > There is *no reason* to rush features into master at this time. > > So here's my -1 veto to that merge until tests are added, or some > acceptable reason for not doing tests is explained (and I agree). > > Please revert the following commits from master until this discussion > is complete: > > 95aef332cc851f91bafb9af7bf5f0f682bb566ce > d7201dfe1f49fb75054e1f0b6922ed21446ad130 > 65210f4e7ee62b237ccdd8d853553e7c990f19c8 > 8a86d08fe307719e50d28d61d8e9025e56ab27da > 951cba92bb7a036ddb25d256fb6e1838aed49d79 > > -chip
