Skip to content

Conversation

@essobedo
Copy link
Contributor

@essobedo essobedo commented Sep 7, 2023

Fixes https://issues.apache.org/jira/browse/AMQ-9305

Motivation

The Dockerfile and corresponding README need to be adapted to Java 17 in order to be able to launch a snapshot of ActiveMQ 5.19 inside a Docker container.

Modifications

  • Use eclipse-temurin:17-jre as new root image
  • Modify the documentation to refer to the new root image used

@jbonofre jbonofre self-requested a review September 7, 2023 11:15
@jbonofre
Copy link
Member

jbonofre commented Sep 7, 2023

ActiveMQ 5.18.x is guaranteed on Java 11. That's why it's temurin 11 by default. I would keep 11 for now and upgrade for 5.19.x.

@essobedo
Copy link
Contributor Author

essobedo commented Sep 7, 2023

I'm getting confused, the branch that I'm targeting (main) is not the branch for 5.19.x?

@jbonofre
Copy link
Member

jbonofre commented Sep 7, 2023

main is the future 5.19.0 but the Dockerfile is used to create also 5.18.x images (you can see the build.sh can download different ActiveMQ version using --from-release option).
So I would:

  1. either wait 5.19.0 out
  2. either limit the release version to 5.19.x in the build.sh

@mattrpav
Copy link
Contributor

mattrpav commented Sep 7, 2023

How about we introduce a Dockerfile for each "LTS", that can then have the JDK level set? The JDK revisions are going to continue to rapidly iterate, so having an approach to support multiple will keep things simple to manage.

Dockerfile-5.18.x
Dockerfile-5.19.x
..

Thoughts?

@essobedo
Copy link
Contributor Author

essobedo commented Sep 7, 2023

OK I better understand let me try to improve it a bit more

@jbonofre
Copy link
Member

jbonofre commented Sep 7, 2023

IMHO, the best approach is to have a specific Dockerfile per branch and scope release-version option to only the major version of the branch.
It's what I planned to do after I added the Dockerfile.

E.g. it means:

  • activemq-5.18.x should have a Dockerfile scoped for 5.18.x (jdk11)
  • main (5.19.x) should have a Dockerfile scoped for 5.19.x (jdk17)

@essobedo
Copy link
Contributor Author

essobedo commented Sep 7, 2023

So your idea is to have a specific version of the folder assembly/src/docker in each LTS branch?

@jbonofre
Copy link
Member

jbonofre commented Sep 7, 2023

Yup

@essobedo
Copy link
Contributor Author

essobedo commented Sep 7, 2023

I added a commit to show how it could be managed with a new parameter to define the target java version. The idea is the following:

  • If the AMQ version is set then if it is a version before 5.19, it uses Java 11 otherwise it uses Java 17
  • If the AMQ version is not set then it is by default Java 17 unless it has been provided explicitly with --target-java-version <version>

WDYT?
If you prefer to have a special copy per branch, I will revert this commit and we can just wait for 5.19 to be released, let me know what you prefer.

@essobedo
Copy link
Contributor Author

essobedo commented Sep 7, 2023

Example of output:

./build.sh --from-release --activemq-version 5.18.2  
Downloading apache-activemq-5.18.2-bin.tar.gz from https://dlcdn.apache.org/activemq/5.18.2/
Using 11 as target Java version
./build.sh --from-local-dist                                    
Using ActiveMQ dist: ../../target/apache-activemq-*.tar.gz
Using 17 as target Java version
 ./build.sh --from-local-dist --target-java-version 11
Using ActiveMQ dist: ../../target/apache-activemq-*.tar.gz
Using 11 as target Java version

@jbonofre
Copy link
Member

jbonofre commented Sep 7, 2023

Thanks I gonna take a look. But I think we can do that in a easy way: as the Java version is specific to a branch, we can have specified in the dockerfile.

@essobedo
Copy link
Contributor Author

essobedo commented Sep 7, 2023

Just note that even if the folder is duplicated in each branch, if in branch 5.19, I execute ./build.sh --from-release --activemq-version 5.18.2, the Java version will be 17 while the expected version is still 11.
In other words, the flexibility of the script can have side effects if the target Java version is not fully managed.

@essobedo essobedo force-pushed the AMQ-9305/adapt-docker-files-to-java-17 branch from a8f9f82 to 51115f0 Compare September 7, 2023 14:57
@essobedo
Copy link
Contributor Author

essobedo commented Sep 7, 2023

Ok, I reverted the proposal, let's wait for 5.19

@mattrpav
Copy link
Contributor

mattrpav commented Sep 7, 2023

Yeah, image names change too much. I don't think we can rely on a a simple '11', '17' parameter.

Perhaps we should move the full container image name up to the parent pom right next to JDK version, so we can remember to update them at the same time.

@jbonofre
Copy link
Member

jbonofre commented Sep 7, 2023

I propose to do the following change:

  • one specific Dockerfile per branch, specific for the branch (JDK version but we could also have some jetty options, etc)
  • square the --release-version arg in the build.sh to allow only minor version on the major version of the branch

Easier to maintain and more logical IMHO.

@jbonofre
Copy link
Member

jbonofre commented Sep 9, 2023

To summarize:

  1. I will merge this PR on main branch as the target is 5.19.x for JDK 17
  2. I will do another change to update the --release-version option to square the version to the branch version
  3. I keep JDK 11 (temurin) on activemq-5.18.x with the same change for --release-versionoption

@essobedo essobedo force-pushed the AMQ-9305/adapt-docker-files-to-java-17 branch from 51115f0 to d7e166d Compare October 16, 2023 08:09
@essobedo essobedo requested a review from jbonofre October 16, 2023 08:11
@jbonofre jbonofre merged commit e259b0e into apache:main Oct 16, 2023
@essobedo essobedo deleted the AMQ-9305/adapt-docker-files-to-java-17 branch October 25, 2023 06:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants