Sorry, I missed your first email, I'll revert the commits soon. I have following questions here, -What's the criteria when unit test is needed, take this as an example, these commits changed two API behaviors, CreateVlanIpRangeCmd and CreateNetworkCmd, the total change is about 100 lines, as you said, it only changed existing class, I think it is like a patch. what kinds of unit test I should write? Unit test for CreateVlanIpRangeCmd.java and CreateNetworkCmd? There is no existing unit tests for them, writing unit tests for them might be much bigger effort compared to these commits. -should we always merge master to feature branch first, then merge back to master? The reason I didn't do it is, the change in sg-in-advanced-zone is small, the change in master is big(it includes javelin check), merging master to sg-in-advanced-zone branch is harder. I merged sg-in-advanced to master, and did basic integrated test.
- should we have staging tree for master? All commits go to staging tree first, after they pass the automation test, they will be merged to master automatically. Thanks, Anthony > -----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
