Skip to content
This repository has been archived by the owner on Apr 26, 2022. It is now read-only.

Add support for ActiveMQ 5.14.0 and new alpine version #7

Closed
wants to merge 20 commits into from
Closed

Add support for ActiveMQ 5.14.0 and new alpine version #7

wants to merge 20 commits into from

Conversation

iemejia
Copy link
Contributor

@iemejia iemejia commented Aug 7, 2016

I also switched the default Java environment from JDK -> JRE because ActiveMQ does not need a full SDK to execute.
I also switched the scripts to version 8 because the size is smaller and Java 7 is already in end of life.

See
http://activemq.apache.org/getting-started.html#GettingStarted-Pre-InstallationRequirements

@iemejia
Copy link
Contributor Author

iemejia commented Aug 7, 2016

It would be really cool if you can regenerate all the images at hub.docker.com, with the new versions because the sizes would be really small.

@iemejia
Copy link
Contributor Author

iemejia commented Aug 7, 2016

I checked the CI error and i tseems it is because of the new alpine image, I can overwrite the existing one in case you don't want the previous and the new version to co-exist. However I don't know how to fix the travis tests to cover both cases (the non-alpine and the alpine versions).

@iemejia
Copy link
Contributor Author

iemejia commented Aug 7, 2016

I added some missing commits to fix some extra issues I found while testing the image. Now it should be ok. (with the exception of the -alpine issues).

@rmohr
Copy link
Owner

rmohr commented Aug 8, 2016

Hi @iemejia,

first of all, thanks for the commits! 👍

It would be really cool if you can regenerate all the images at hub.docker.com, with the new versions because the sizes would be really small.

I am not sure if I want to update all of them. There might me cases where this would break extra JVM configuration people applied on their activemq image. At least we have to check that first.

Maybe we can use java8 as default JVM from 5.14.0 on and provide an alpine image too, pretty much like you did it, and add additional images for older activemq versions, which indicate that they are using java8 in their name?

I added some missing commits to fix some extra issues I found while testing the image. Now it should be ok. (with the exception of the -alpine issues).

I am pretty sure that wget is not installed in the alpine image.

@iemejia
Copy link
Contributor Author

iemejia commented Aug 8, 2016

Hi, thanks for considering the move to JRE 8. You are right probably the wisest thing to do is to do so from version 5.14.0 so people are aware of this (that is the reason I didn't rebased all images on the alpine one, even if I would prefer that).
I don't understand your comment about wget (I use curl by the way and you can notice that I install and remove it at build time in the alpine image, so the image keeps lean and without unneeded deps).

@iemejia
Copy link
Contributor Author

iemejia commented Aug 8, 2016

I don't know if we can automate this script generation / build so we don't need to rewrite again and again for any single variation (of java version + activemq version), any ideas ?
And have you checked about the correct way to pass the test for the alpine image ? (because the problem is not that the image is incorrect, the problem is that the test don't expect this kind of sufixes (-alpine).

@iemejia
Copy link
Contributor Author

iemejia commented Aug 8, 2016

I added an additional commit with the change to the license as discussed in #8

@rmohr
Copy link
Owner

rmohr commented Aug 8, 2016

I don't understand your comment about wget (I use curl by the way and you can notice that I install and remove it at build time in the alpine image, so the image keeps lean and without unneeded deps).

What I meant is that this line in .travis.yml

  - docker-compose run activemq-service wget --retry-connrefused --waitretry=5 --read-timeout=20 --timeout=15 -t 10 -O /dev/null http://admin:admin@activemq-service:8161/admin/

calls wget from inside the container. First I thought that wget is not installed there but what it actually says is wget: unrecognized option: retry-connrefused. So there is a "wget". I guess they have something like the busybox wget in alpine which is very limited too and does not have this option either.

@iemejia
Copy link
Contributor Author

iemejia commented Aug 8, 2016

You are absolutely right, my mistake I didn't see this one coming. I just did the easy fix of adding wget to the docker image for the moment, so you can merge the changes.

@iemejia
Copy link
Contributor Author

iemejia commented Aug 8, 2016

I was thinking and it is probably a good idea to maybe do the java 8 switch from version 5.14.0 as you proposed, and maybe do the switch for all the images when we (in case you are interested and they accept it) get those into the official docker images repo.

@rmohr
Copy link
Owner

rmohr commented Aug 8, 2016

It would be better not to include wget in the prebuilt image.
Installing it before running the first test should work:

docker-compose run activemq-service apk add wget
docker-compose run activemq-service wget --retry-connrefused --waitretry=5 --read-timeout=20 --timeout=15 -t 10 -O /dev/null http://admin:admin@activemq-service:8161/admin/

@iemejia
Copy link
Contributor Author

iemejia commented Aug 8, 2016

I agree that it is not nice to add wget, but it is a small price to pass the test, otherwise we would need to change the test. I tried the idea you proposed, but it has a problem, is there a way to choose to execute or no the apk command ? otherwise it will break all the other images (where apk is not included).

@rmohr
Copy link
Owner

rmohr commented Aug 8, 2016

We can check if we are using the alpine image. Something like this should work:

if [[ $AMQ_VERSION = *alpine ]] ; do docker-compose run activemq-service apk add wget  ; fi

I am checking the environment variable AMQ_VERSION which we are setting for every travis subtest if it ends with alpine and only then install wget.

@@ -1,4 +1,4 @@
FROM java:7-jre
FROM java:8-jre
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you remove everything except this line from this commit since we agreed to stay on java7 for the old images?

@rmohr
Copy link
Owner

rmohr commented Aug 10, 2016

I suggest that we split this pull request into 4 so we can merge them individually:
1) Fix docker-compose with travis #10
2) Add 5.14.0 with java8-jre
3) Add alpine support + travis tests
4) License change

@@ -10,7 +10,8 @@ RUN set -x && \
curl -s https://archive.apache.org/dist/activemq/$ACTIVEMQ_VERSION/$ACTIVEMQ-bin.tar.gz | tar xvz -C /opt && \
ln -s /opt/$ACTIVEMQ $ACTIVEMQ_HOME && \
useradd -r -M -d $ACTIVEMQ_HOME activemq && \
chown activemq:activemq /opt/$ACTIVEMQ -R
chown -R activemq:activemq /opt/$ACTIVEMQ && \
chown -h activemq:activemq $ACTIVEMQ_HOME
Copy link
Owner

@rmohr rmohr Aug 10, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably want this line in the other Dockerfiles too. File #12 to track this.

@iemejia
Copy link
Contributor Author

iemejia commented Aug 10, 2016

You are right probably this is the best approach since this PR became quite a mess with all my trials and errors against travis, so I will do as you said

  1. Add 5.14.0 with java8-jre Add support for ActiveMQ 5.14.0 #14
  2. Add alpine support + travis tests Add support for ActiveMQ 5.14.0 (alpine) #15
  3. License change Change LICENSE from MIT to Apache #13

I will wait that you merge both to rebase the alpine version on master at that moment.
And close this issue too after the PR for this.

@iemejia
Copy link
Contributor Author

iemejia commented Aug 13, 2016

I am closing this one since the only missing part is already in PR #15

@iemejia iemejia closed this Aug 13, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants