On 09/05/2012 04:17 PM, Chip Childers wrote:
On Wed, Sep 5, 2012 at 10:08 AM, Pradeep Soundararajan
<[email protected]> wrote:
Hello Hugo/Chip,
These are the review comments I have received so far.
- For build-cloud.properties, that line should be optional. This means add a
'#' in front of it and others an make it required when copying it into override.
- The change for developer.xml to read in the build-cloud.xml should be done as
a precondition to all build targets.
- The change does not take into account that the file may be on the class path
and so the property value should only be optional. Here it made it explicit.
- Also, we need to add the mysql connector as a rpm dependency. That change is
not in.
Hugo >> I did a quick fix to get the build working again, just pushed the
commit to master. The mysql connector rpm is already installed on the Jenkins server,
I just modified the scripts to it is picked up by the build.
>> I can add it to the spec file for rpm based distro's. I think it
should be added to the client rpm.
I see that you have added the class path explicitly in order to resolve this
temporarily.
# Add mysql jar from mysql-connector-java package to CP
# for Jenkins
CP=${CP}${PATHSEP}/usr/share/java/mysql-connector-java.jar
Also, I have seen that you have added that rpm in ‘cloud.spec’ as well.
Since you are calling mysql-connector explicitly from ‘deploy-db-dev.sh’
class-path, the changes from ‘build-cloud.properties’ and ‘developer.xml’ are
not required from my patch. Please correct me if I am wrong.
If everything is fine, shall we close this topic. Please share if anyone have
any comments?
I *think* that's right. Let's see how the Maven build process shakes
out, since this is actually tied to that as well. We know that
mysql-connector needs to be a system dependency, and not be
distributed with the ASF release.
One item that does come to mind though... shouldn't it be included in
the DEB packages too? Can you add that?
Ubuntu and Debian both ship the MySQL connector in a package. I already
added a dependency for that:
https://git-wip-us.apache.org/repos/asf?p=incubator-cloudstack.git;a=commit;h=9064236879dc9f2f11538d891271545e1ff10e9c
Wido
Thanks,
Pradeep.S
-----Original Message-----
From: Alex Huang
[mailto:[email protected]]<mailto:[mailto:[email protected]]>
On Behalf Of Alex Huang
Sent: Friday, August 31, 2012 8:37 PM
To: David Nalley; Alex Huang
Cc: cloudstack; Pradeep Soundararajan
Subject: Re: Review Request: CS-15694:Remove MYSQL connector. Added the
explicit class path for mysql connector which will call the jar from the
desired location
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6752/#review10943
-----------------------------------------------------------
Also, we need to add the mysql connector as a rpm dependency. That change is
not in.
- Alex Huang
On Aug. 24, 2012, 10:19 a.m., Pradeep Soundararajan wrote:
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/6752/
-----------------------------------------------------------
(Updated Aug. 24, 2012, 10:19 a.m.)
Review request for cloudstack, David Nalley and Alex Huang.
Description
-------
CS-15694:Remove MYSQL connector. Added the explicit class path for mysql
connector which will call the jar from the desired location
Diffs
-----
build/build-cloud.properties 552de7f74db7b715da70cc48ff4dc8945fa066f8
build/developer.xml f2e5aa6463ec849a3e97343e82423ff0ac622222
setup/db/deploy-db-dev.sh f149e9efd029bd311b1d247e21764b9103fd01d9
Diff: https://reviews.apache.org/r/6752/diff/
Testing
-------
Executed: "ant build-all deploy-server deploydb" successfully.
Thanks,
Pradeep Soundararajan
