You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@brooklyn.apache.org by nakomis <gi...@git.apache.org> on 2015/02/04 15:33:21 UTC
[GitHub] incubator-brooklyn pull request: Fixes RabbitMQ installation on Ce...
GitHub user nakomis opened a pull request:
https://github.com/apache/incubator-brooklyn/pull/503
Fixes RabbitMQ installation on Centos 6 and 7
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/nakomis/incubator-brooklyn fix/rabbit-on-centos
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/incubator-brooklyn/pull/503.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #503
----
commit 73389e587b2e5bdf46d5aba51fa1d4c7a5bade12
Author: Martin Harris <gi...@nakomis.com>
Date: 2015-02-04T14:32:53Z
Fixes RabbitMQ installation on Centos 6 and 7
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Fixes RabbitMQ installation on Ce...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/503#discussion_r24094730
--- Diff: software/messaging/src/main/java/brooklyn/entity/messaging/rabbit/RabbitSshDriver.java ---
@@ -24,6 +24,7 @@
import java.util.List;
import java.util.Map;
+import com.google.api.client.repackaged.com.google.common.base.Strings;
--- End diff --
I'm tempted to set Checkstyle to completely forbid `com.google.api.client.repackaged`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Fixes RabbitMQ installation on Ce...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/503#discussion_r24244870
--- Diff: software/messaging/src/main/java/brooklyn/entity/messaging/rabbit/RabbitSshDriver.java ---
@@ -44,6 +45,13 @@
public class RabbitSshDriver extends AbstractSoftwareProcessSshDriver implements RabbitDriver {
private static final Logger log = LoggerFactory.getLogger(RabbitSshDriver.class);
+
+ // See http://fedoraproject.org/wiki/EPEL/FAQ#howtouse
+ private static final Map<String, String> CENTOS_VERSION_TO_EPEL_VERSION = ImmutableMap.of(
--- End diff --
It would make sense to do this somewhere generic (e.g. `BashCommands`) as EPEL is not specific to Rabbit or even to Erlang. However, I'm ok with merging it here and then it being moved.
I don't like just "waiting for the second usage" before refactoring, because downstream projects may want to use that - those projects should ideally be written without having to change core.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Fixes RabbitMQ installation on Ce...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/503#discussion_r24095111
--- Diff: software/messaging/src/test/java/brooklyn/entity/messaging/rabbit/RabbitEc2LiveTest.java ---
@@ -81,7 +81,16 @@ private Channel getAmqpChannel(RabbitBroker rabbit) throws Exception {
Channel channel = conn.createChannel();
return channel;
}
-
+
+ @Override
+ public void test_CentOS_5_6() throws Exception {
+ // Not supported. The EPEL repository described here at [1] does not contain erlang, and the
--- End diff --
Could throw a [`SkipException`](http://testng.org/javadoc/org/testng/SkipException.html) to be explicit to the build.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Fixes RabbitMQ installation on Ce...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/incubator-brooklyn/pull/503
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Fixes RabbitMQ installation on Ce...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/503#issuecomment-73489094
Thanks @nakomis and @sjcorbett - merging.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Fixes RabbitMQ installation on Ce...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/503#discussion_r24160763
--- Diff: software/messaging/src/main/java/brooklyn/entity/messaging/rabbit/RabbitSshDriver.java ---
@@ -72,8 +80,15 @@ public void preInstall() {
public void install() {
List<String> urls = resolver.getTargets();
String saveAs = resolver.getFilename();
+ String osMajorVersion = Strings.nullToEmpty(getMachine().getOsDetails().getVersion());
+ osMajorVersion = osMajorVersion.indexOf(".") > 0 ? osMajorVersion.substring(0, osMajorVersion.indexOf('.')) : osMajorVersion;
+ String epelVersion = Strings.nullToEmpty(CENTOS_VERSION_TO_EPEL_VERSION.get(osMajorVersion));
+ String osArchitecture = Strings.nullToEmpty(getMachine().getOsDetails().getArch());
--- End diff --
Is this behaviour desirable? NPEs are guarded against but it could generate URLs like `http://download.fedoraproject.org/pub/epel///epel-release-.noarch.rpm`. Should we pick a default if we can't detect versions and architectures? Or it could even just fail.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Fixes RabbitMQ installation on Ce...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/503#discussion_r24095005
--- Diff: software/messaging/src/main/java/brooklyn/entity/messaging/rabbit/RabbitSshDriver.java ---
@@ -73,7 +81,15 @@ public void install() {
List<String> urls = resolver.getTargets();
String saveAs = resolver.getFilename();
+ String osMajorVersion = getMachine().getOsDetails().getVersion();
+ osMajorVersion = osMajorVersion.indexOf(".") > 0 ? osMajorVersion.substring(0, osMajorVersion.indexOf('.')) : osMajorVersion;
--- End diff --
`osMajorVersion` and `osArchitecture` are both annotated `@Nullable` so there's a chance of `NullPointerExceptions` here.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Fixes RabbitMQ installation on Ce...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/503#issuecomment-73042855
Added selection of default for EPEL package download
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Fixes RabbitMQ installation on Ce...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on the pull request:
https://github.com/apache/incubator-brooklyn/pull/503#issuecomment-73039941
PR comments addressed
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Fixes RabbitMQ installation on Ce...
Posted by aledsage <gi...@git.apache.org>.
Github user aledsage commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/503#discussion_r24245067
--- Diff: software/messaging/src/test/java/brooklyn/entity/messaging/rabbit/RabbitEc2LiveTest.java ---
@@ -81,7 +82,16 @@ private Channel getAmqpChannel(RabbitBroker rabbit) throws Exception {
Channel channel = conn.createChannel();
return channel;
}
-
+
+ @Override
+ public void test_CentOS_5_6() throws SkipException {
+ // Not supported. The EPEL repository described here at [1] does not contain erlang, and the
--- End diff --
Agree with @sjcorbett `throw new SkipException()` means we don't pretend it worked in the test results.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Fixes RabbitMQ installation on Ce...
Posted by nakomis <gi...@git.apache.org>.
Github user nakomis commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/503#discussion_r24160319
--- Diff: software/messaging/src/main/java/brooklyn/entity/messaging/rabbit/RabbitSshDriver.java ---
@@ -24,6 +24,7 @@
import java.util.List;
import java.util.Map;
+import com.google.api.client.repackaged.com.google.common.base.Strings;
--- End diff --
Good idea. I usually spot it when my IDE picks this one, but obviously missed it this time
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] incubator-brooklyn pull request: Fixes RabbitMQ installation on Ce...
Posted by sjcorbett <gi...@git.apache.org>.
Github user sjcorbett commented on a diff in the pull request:
https://github.com/apache/incubator-brooklyn/pull/503#discussion_r24094419
--- Diff: software/messaging/src/main/java/brooklyn/entity/messaging/rabbit/RabbitSshDriver.java ---
@@ -24,6 +24,7 @@
import java.util.List;
import java.util.Map;
+import com.google.api.client.repackaged.com.google.common.base.Strings;
--- End diff --
Wrong package. Should be `com.google.common.base.Strings`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---