Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

NIFI-7717 Updated main NiFi images to JRE 11. #4460

Closed
wants to merge 3 commits into from

Conversation

MikeThomsen
Copy link
Contributor

Thank you for submitting a contribution to Apache NiFi.

Please provide a short description of the PR here:

Description of PR

Enables X functionality; fixes bug NIFI-YYYY.

In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced
    in the commit message?

  • Does your PR title start with NIFI-XXXX where XXXX is the JIRA number you are trying to resolve? Pay particular attention to the hyphen "-" character.

  • Has your PR been rebased against the latest commit within the target branch (typically main)?

  • Is your initial contribution a single, squashed commit? Additional commits in response to PR reviewer feedback should be made on this branch and pushed to allow change tracking. Do not squash or use --force when pushing to allow for clean monitoring of changes.

For code changes:

  • Have you ensured that the full suite of tests is executed via mvn -Pcontrib-check clean install at the root nifi folder?
  • Have you written or updated unit tests to verify your changes?
  • Have you verified that the full build is successful on JDK 8?
  • Have you verified that the full build is successful on JDK 11?
  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?
  • If applicable, have you updated the LICENSE file, including the main LICENSE file under nifi-assembly?
  • If applicable, have you updated the NOTICE file, including the main NOTICE file found under nifi-assembly?
  • If adding new Properties, have you added .displayName in addition to .name (programmatic access) for each of the new properties?

For documentation related changes:

  • Have you ensured that format looks appropriate for the output in which it is rendered?

Note:

Please ensure that once the PR is submitted, you check GitHub Actions CI for build issues and submit an update to your PR as soon as possible.

@joewitt
Copy link
Contributor

joewitt commented Aug 10, 2020

@MikeThomsen given we support a base of java 8 my perspective here is it makes sense to base images on 8 but allow for 11 rather than basing images on 11 but allowing for 8. Folks can opt in to the newer bits is what I mean. Does this solve something you're facing?

My experience, sadly, is that in the wild users are very slow to upgrade their JDK/JRE

@jfrazee
Copy link
Member

jfrazee commented Aug 13, 2020

@joewitt @MikeThomsen Can I suggest that this be a build arg in the Dockerfile (they can be used in a FROM) and a property in the pom? That'd make the build pretty flexible and let people twiddle the versions without making changes to the Dockerfile each time.

@MikeThomsen
Copy link
Contributor Author

@joewitt I just figured we'd nudge Docker users along a little. To @jfrazee's point, we might want to consider having Maven parameterize it with the resource plugin or something like that.

@jfrazee
Copy link
Member

jfrazee commented Aug 17, 2020

@MikeThomsen The Docker maven plugin has a <buildArgs/> section so I think with a few small changes this PR could strike the right balance.

@MikeThomsen
Copy link
Contributor Author

@jfrazee @joewitt updated with the ARG so it's parameterized, with a default to JDK 8. Let me know what you think.

@jfrazee
Copy link
Member

jfrazee commented Oct 1, 2020

@MikeThomsen I'm going to nit on something. This being called JAVA_VERSION would make me think it'd be something like 11 or 1.8 or one of the many openjdk tags and not the base image name.

That brings up another question. What's the tradeoff between having the base image be completely specified vs. just a tag for the openjdk image?

My thinking is:

  1. If it was just the tag for openjdk it's more likely the image will be as expected (since most but not all of the openjdk tags are Debian based).
  2. Whereas allowing the entire base image to be changed would let people use the released source to build images of their own, let's say hardened or approved images, with the drawback of it not working if they provide a non-Debian based image. (That said, it's very light on Debian assumptions, so there might be even some room for some easy changes to make it work with other distros later.)

I think (2) is reasonable motivation.

@jfrazee
Copy link
Member

jfrazee commented Feb 15, 2021

@MikeThomsen I wanted to come back to this. I've been using this for a while now and think it'd be helpful to split off the image/repository name, the image tag, and the maintainer as ARGs. For example:

ARG IMAGE_NAME=openjdk
ARG IMAGE_TAG=8-jre
ARG MAINTAINER="Apache NiFi <dev@nifi.apache.org>"

FROM ${IMAGE_NAME}:${IMAGE_TAG} AS artifactbase
LABEL maintainer="${MAINTAINER}"

...

FROM ${IMAGE_NAME}:${IMAGE_TAG}
LABEL maintainer="${MAINTAINER}"

And:

<properties>
    <docker.image.name>openjdk</docker.image.name>
    <docker.image.tag>8-jre</docker.image.tag>
    <docker.maintainer><![CDATA[Apache NiFi <dev@nifi.apache.org>]]></docker.maintainer>
</properties>

What do you think?

Comment on lines 18 to 19
ARG JAVA_VERSION
FROM ${JAVA_VERSION}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ARG JAVA_VERSION
FROM ${JAVA_VERSION}
ARG IMAGE_NAME=openjdk
ARG IMAGE_TAG=8-jre
ARG MAINTAINER="Apache NiFi <dev@nifi.apache.org>"
FROM ${IMAGE_NAME}:${IMAGE_TAG} AS artifactbase


FROM openjdk:8-jre
ARG JAVA_VERSION
FROM ${JAVA_VERSION}
LABEL maintainer="Apache NiFi <dev@nifi.apache.org>"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
LABEL maintainer="Apache NiFi <dev@nifi.apache.org>"
LABEL maintainer="${MAINTAINER}"

@@ -37,6 +41,7 @@
</goals>
<configuration>
<buildArgs>
<JAVA_VERSION>${docker.java.version}</JAVA_VERSION>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<JAVA_VERSION>${docker.java.version}</JAVA_VERSION>
<IMAGE_NAME>${docker.image.name}</IMAGE_NAME >
<IMAGE_TAG>${docker.image.tag}</IMAGE_TAG >
<MAINTAINER>${docker.maintainer}</MAINTAINER >

Comment on lines 19 to 20
ARG JAVA_VERSION
FROM $JAVA_VERSION AS artifactbase
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ARG JAVA_VERSION
FROM $JAVA_VERSION AS artifactbase
ARG IMAGE_NAME=openjdk
ARG IMAGE_TAG=8-jre
ARG MAINTAINER="Apache NiFi <dev@nifi.apache.org>"
FROM ${IMAGE_NAME}:${IMAGE_TAG} AS artifactbase

@@ -16,7 +16,8 @@
# under the License.
#

FROM openjdk:8-jre AS artifactbase
ARG JAVA_VERSION
FROM $JAVA_VERSION AS artifactbase
LABEL maintainer="Apache NiFi <dev@nifi.apache.org>"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
LABEL maintainer="Apache NiFi <dev@nifi.apache.org>"
LABEL maintainer="${MAINTAINER}"

@@ -53,7 +54,7 @@ RUN mkdir -p ${NIFI_HOME}/conf \
&& mkdir -p ${NIFI_HOME}/state \
&& mkdir -p ${NIFI_LOG_DIR}

FROM openjdk:8-jre
FROM $JAVA_VERSION
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
FROM $JAVA_VERSION
FROM ${IMAGE_NAME}:${IMAGE_TAG}

@@ -53,7 +54,7 @@ RUN mkdir -p ${NIFI_HOME}/conf \
&& mkdir -p ${NIFI_HOME}/state \
&& mkdir -p ${NIFI_LOG_DIR}

FROM openjdk:8-jre
FROM $JAVA_VERSION
LABEL maintainer="Apache NiFi <dev@nifi.apache.org>"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
LABEL maintainer="Apache NiFi <dev@nifi.apache.org>"
LABEL maintainer="${MAINTAINER}"

@@ -20,6 +20,10 @@

<artifactId>dockerhub</artifactId>

<properties>
<docker.java.version>openjdk:8-jre</docker.java.version>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<docker.java.version>openjdk:8-jre</docker.java.version>
<docker.image.name>openjdk</docker.image.name>
<docker.image.tag>8-jre</docker.image.tag>
<docker.maintainer><![CDATA[Apache NiFi <dev@nifi.apache.org>]]></docker.maintainer>

@@ -20,6 +20,10 @@

<artifactId>dockermaven</artifactId>

<properties>
<docker.java.version>openjdk:8-jre</docker.java.version>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<docker.java.version>openjdk:8-jre</docker.java.version>
<docker.image.name>openjdk</docker.image.name>
<docker.image.tag>8-jre</docker.image.tag>
<docker.maintainer><![CDATA[Apache NiFi <dev@nifi.apache.org>]]></docker.maintainer>

@@ -37,6 +41,7 @@
</goals>
<configuration>
<buildArgs>
<JAVA_VERSION>${docker.java.version}</JAVA_VERSION>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<JAVA_VERSION>${docker.java.version}</JAVA_VERSION>
<IMAGE_NAME>${docker.image.name}</IMAGE_NAME >
<IMAGE_TAG>${docker.image.tag}</IMAGE_TAG >
<MAINTAINER>${docker.maintainer}</MAINTAINER >

@MikeThomsen
Copy link
Contributor Author

I'll try to take a look at this tonight. Sorry for missing it.

@MikeThomsen
Copy link
Contributor Author

@jfrazee made the updates. Let me know if they match your expectations.

@jfrazee
Copy link
Member

jfrazee commented Mar 8, 2021

@MikeThomsen Hey, I'm +1 on this with one caveat. It looks like LABEL can only use an ARG that's after a FROM. Since it's just moving a line, I can do this on merge unless you object?

@MikeThomsen
Copy link
Contributor Author

Yeah, go ahead and make the change.

@jfrazee jfrazee closed this in 160297f Mar 10, 2021
@jfrazee
Copy link
Member

jfrazee commented Mar 10, 2021

@MikeThomsen Thanks for this. Was able to build custom image using Java 11 with the following:

mvn package -DskipTests -ff -nsu \
    -Pdocker \
    -Ddocker.image.name=openjdk \
    -Ddocker.image.tag=11-jre \
    -Ddocker.maintainer='jfrazee@apache.org'

driesva pushed a commit to driesva/nifi that referenced this pull request Mar 19, 2021
This closes apache#4460

Signed-off-by: Joey Frazee <jfrazee@apache.org>
krisztina-zsihovszki pushed a commit to krisztina-zsihovszki/nifi that referenced this pull request Jun 28, 2022
This closes apache#4460

Signed-off-by: Joey Frazee <jfrazee@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants