Mailing-List: contact cloudstack-dev-help@incubator.apache.org; run by ezmlm
Precedence: bulk
Reply-To: cloudstack-dev@incubator.apache.org
Received-SPF: neutral (athena.apache.org: local policy)
MIME-Version: 1.0
In-Reply-To: <CD2DCD11.2EB6D%murali.reddy@citrix.com>
References: 
 <CA+96GG5OfOq9zRpB=ggHrUJBhWGPy11P9Ot1djLPbMVzOtpZQQ@mail.gmail.com>
	<CD2DCD11.2EB6D%murali.reddy@citrix.com>
Date: Tue, 29 Jan 2013 09:14:55 -0500
Message-ID: 
 <CA+96GG5zU5nez6YRvPbaFUuCF5Zq0nb_eD7hnaWiWEm_dJjNow@mail.gmail.com>
Subject: Re: [MERGE] merge 'events-framework' branch to master
From: Chip Childers <chip.childers@sungard.com>
To: "cloudstack-dev@incubator.apache.org"
 <cloudstack-dev@incubator.apache.org>
Content-Type: text/plain; charset=ISO-8859-1

On Tue, Jan 29, 2013 at 8:36 AM, Murali Reddy <Murali.Reddy@citrix.com> wrote:
>
>>Great to see this being close to coming into master!  A couple of
>>questions:
>>
>>1 - I don't seem to be able to find any new unit tests that cover the
>>feature.  Are there any that I'm missing?
>
> I am working on unit tests. I will update the thread once I have unit
> tests pushed to feature branch

I think it would be great to have unit tests that come along with the feature.

>>
>>2 - Doing a diff between master and the events-framework branches, I
>>see a number of unrelated changes that would be applied.  Perhaps
>>events framework isn't completely up to date.  I'd ask that you review
>>the diff to see if there are any surprises in there.
>
> Sure. Will double check if I have unintended changes.
>

Great - I was just concerned, but perhaps incorrectly so.  Hence why I
raised the question.

>>
>>3 - As I read the pom's (which may be incorrect), RabbitMQ's client is
>>now a required dependency.  Does everyone agree that we should include
>>Rabbit as a dependency?
>
> I replied back to this concern in other mail.

Yes, we're good from my perspective.  Would you like me to take care
of the legal doc changes required?

>>
>>4 - Is the feature disabled by default?
>
> Yes. I kept EventBus as CloudStack 'Adapter'. So unless a implementation
> class is configured in the components.xml (or spring variant of it) there
> is no impact.

Ok, thanks!


Generally, I'm +1 to this merge, assuming the following:

1) Unit tests are added that test the new code.
2) You review and confirm the diffs between your branch and master
before merging.
3) One of us has to take care of the legal docs.  I'll actually take
care of it directly in master today.