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-10701 Update MiNiFi docker base images to eclipse-temurin #6587

Conversation

briansolo1985
Copy link
Contributor

@briansolo1985 briansolo1985 commented Oct 27, 2022

Summary

NIFI-10701

Tracking

Please complete the following tracking steps prior to pull request creation.

Issue Tracking

Pull Request Tracking

  • Pull Request title starts with Apache NiFi Jira issue number, such as NIFI-00000
  • Pull Request commit message starts with Apache NiFi Jira issue number, as such NIFI-00000

Pull Request Formatting

  • Pull Request based on current revision of the main branch
  • Pull Request refers to a feature branch with one commit containing changes

Verification

Please indicate the verification steps performed prior to pull request creation.

Build

  • Build completed using mvn clean install -P contrib-check
    • JDK 8
    • JDK 11
    • JDK 17

Licensing

  • New dependencies are compatible with the Apache License 2.0 according to the License Policy
  • New dependencies are documented in applicable LICENSE and NOTICE files

Documentation

  • Documentation formatting appears as expected in rendered files

@briansolo1985 briansolo1985 force-pushed the NIFI-10701_Update_MiNiFi_Base_Docker_Image branch from 780c167 to 5ba17c2 Compare October 27, 2022 08:14
@briansolo1985
Copy link
Contributor Author

As openjdk images are deprecated and discontinued I updated the base images to eclipse-temurin which can be considered as a successor of the openjdk line.
Also unified the image structures and documentation.
Removed the minifi arm64 definitions as eclipse-temurin images are multiarch ones, so can be built on arbitrary platforms.
Tested: run minifi and minifi-c2 intergration tests which rely on these images. Built the dockerhub images and run them locally.

@bejancsaba bejancsaba added the minifi Pull requests that updates minifi/c2 codes label Oct 27, 2022
@kevdoran kevdoran self-requested a review October 27, 2022 14:24
Copy link
Contributor

@bejancsaba bejancsaba left a comment

Choose a reason for hiding this comment

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

Thanks for the improvement +1 from my side

@@ -16,13 +16,13 @@
# under the License.
#

FROM openjdk:8-jre-alpine
FROM eclipse-temurin:8-jre
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is good this way just wanted to double check whether it would make sense to pin the exact version as this way we get the latest and we can get "silent" java updates which could cause problems (at least I faced an issue on a previous project because of this). Again I think we are good and this is how it was earlier just wanted to double check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I think it's beneficial to have the latest version for every build even though as you said there is a risk that a new Java version will cause issues. Would keep it as it is if no objections.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I'm ok with this given it should not be being built frequently. I noticed the same about the apt-get update && ... command, but on the whole, I think the benefits are worth it. We can revisit if it becomes a problem.

@@ -47,17 +46,17 @@ This can be accomplished through:
The following example shows the usage of two volumes to provide both a `config.yml` and a `bootstrap.conf` to the container instance. This makes use of configuration files on the host and maps them to be used by the MiNiFi instance. This is helpful in scenarios where a single image is used for a variety of configurations.

docker run -d \
-v ~/minifi-conf/config.yml:/opt/minifi/minifi-0.5.0/conf/config.yml \
-v ~/minifi-conf/bootstrap.conf:/opt/minifi/minifi-0.5.0/conf/bootstrap.conf \
apache/nifi-minifi:0.5.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Quite a big jump in versions :)

@kevdoran
Copy link
Contributor

kevdoran commented Nov 9, 2022

Thanks for the contribution @briansolo1985 and for the help reviewing @bejancsaba @ferencerdei. I should be able to complete my review soon

Copy link
Contributor

@exceptionfactory exceptionfactory left a comment

Choose a reason for hiding this comment

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

Thanks for putting this together @briansolo1985! The change from the unsupported OpenJDK image to the current Eclipse Temurin JDK looks good. That is one of the two JDK versions used for regular GitHub Actions, so it makes sense as a base image.

Copy link
Contributor

@kevdoran kevdoran left a comment

Choose a reason for hiding this comment

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

Nice work @briansolo1985! The code changes look good, and I had a chance to try it out.

  • I did a dockermaven build and verified the resulting images
  • I tested dockerhub builds using 1.18.0
  • I tested dockerhub builds for arm64 using the process you shared with me. This will be a nice addition for the community!

All worked great! I opened NIFI-10820 to track doing a similar update to the base image for nifi, registry, etc. That should be easy following what you have done here for minifi and minifi-c2. Thanks!

+1, merging to main.

@kevdoran
Copy link
Contributor

Merged in commit 709110da35f4f2cc91a916d0663742c594cba126

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minifi Pull requests that updates minifi/c2 codes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants