[GitHub] [flink-docker] zentol commented on a change in pull request #32: [FLINK-16260] Add support for Java11 in dev-master / introduce new release.metadata file

[GitBox <gi...@apache.org>]

zentol commented on a change in pull request #32:
URL: https://github.com/apache/flink-docker/pull/32#discussion_r461508280



##########
File path: generator.sh
##########
@@ -0,0 +1,80 @@
+#!/bin/bash -e
+
+# Defaults, can vary between versions
+export SOURCE_VARIANTS=(java11-debian debian )
+
+export LATEST_SCALA="2.12"
+
+function generate() {

Review comment:
       I'd split this into 2 functions; one for generating the image, one for generating the meta data.
   Right now its a function doing 2 things with (pretty much) 2 non-overlapping argument sets.
   Then we don't have to deal with it for `add-custom.sh`.

##########
File path: generator.sh
##########
@@ -0,0 +1,80 @@
+#!/bin/bash -e
+
+# Defaults, can vary between versions
+export SOURCE_VARIANTS=(java11-debian debian )
+
+export LATEST_SCALA="2.12"
+
+function generate() {
+    # define variables
+    dir=$1
+    binary_download_url=$2
+    asc_download_url=$3
+    gpg_key=$4
+    check_gpg=$5
+    flink_release=$6
+    flink_version=$7
+    scala_version=$8
+    source_variant=$9
+
+    from_docker_image="openjdk:8-jre"
+    if [[ $source_variant =~ "java11" ]] ; then
+        from_docker_image="openjdk:11-jre"
+    fi
+
+    ########################################
+    ### generate "Dockerfile" file
+    ########################################
+
+    # overwrite variable based on $source_variant to support non-debian releases
+    source_file="Dockerfile-debian"
+
+    mkdir "$dir"
+    cp docker-entrypoint.sh "$dir/docker-entrypoint.sh"
+
+    # '&' has special semantics in sed replacement patterns
+    escaped_binary_download_url=$(echo "$binary_download_url" | sed 's/&/\\\&/')
+
+    # generate Dockerfile
+    sed \
+        -e "s,%%BINARY_DOWNLOAD_URL%%,${escaped_binary_download_url}," \
+        -e "s,%%ASC_DOWNLOAD_URL%%,$asc_download_url," \
+        -e "s/%%GPG_KEY%%/$gpg_key/" \
+        -e "s/%%CHECK_GPG%%/${check_gpg}/" \
+        -e "s/%%FROM_IMAGE%%/${from_docker_image}/" \
+        "$source_file.template" > "$dir/Dockerfile"
+
+    ########################################
+    ### generate "release.metadata" file
+    ########################################
+    
+    # docker image tags:
+    java11_suffix=""
+    if [[ $source_variant =~ "java11" ]] ; then

Review comment:
       its a bit weird to encode the java version into the source_variant when the scala version has its own argument.
   I'd rather have the java version as an explicit argument, with a dedicated set of `-java8` tags, + it being considered the default java version for the tag set without java versions.

##########
File path: generator.sh
##########
@@ -0,0 +1,80 @@
+#!/bin/bash -e
+
+# Defaults, can vary between versions

Review comment:
       can be removed now; everything on the dev branches can vary between versions

##########
File path: generator.sh
##########
@@ -0,0 +1,80 @@
+#!/bin/bash -e
+
+# Defaults, can vary between versions
+export SOURCE_VARIANTS=(java11-debian debian )
+
+export LATEST_SCALA="2.12"
+
+function generate() {
+    # define variables
+    dir=$1
+    binary_download_url=$2
+    asc_download_url=$3
+    gpg_key=$4
+    check_gpg=$5
+    flink_release=$6
+    flink_version=$7
+    scala_version=$8
+    source_variant=$9
+
+    from_docker_image="openjdk:8-jre"
+    if [[ $source_variant =~ "java11" ]] ; then
+        from_docker_image="openjdk:11-jre"
+    fi
+
+    ########################################
+    ### generate "Dockerfile" file
+    ########################################
+
+    # overwrite variable based on $source_variant to support non-debian releases

Review comment:
       this seems to do the opposite; you're hard-coding debian.
   It seems overly complicated to add `java11-debian` into `SOURCE_VARIANTS` only to overwrite it later with `debian` anyway.
   A separate java version argument would be a simpler solution.

##########
File path: generator.sh
##########
@@ -0,0 +1,80 @@
+#!/bin/bash -e
+
+# Defaults, can vary between versions
+export SOURCE_VARIANTS=(java11-debian debian )
+
+export LATEST_SCALA="2.12"
+
+function generate() {
+    # define variables
+    dir=$1
+    binary_download_url=$2
+    asc_download_url=$3
+    gpg_key=$4
+    check_gpg=$5
+    flink_release=$6
+    flink_version=$7
+    scala_version=$8
+    source_variant=$9
+
+    from_docker_image="openjdk:8-jre"
+    if [[ $source_variant =~ "java11" ]] ; then
+        from_docker_image="openjdk:11-jre"
+    fi
+
+    ########################################
+    ### generate "Dockerfile" file
+    ########################################
+
+    # overwrite variable based on $source_variant to support non-debian releases
+    source_file="Dockerfile-debian"
+
+    mkdir "$dir"
+    cp docker-entrypoint.sh "$dir/docker-entrypoint.sh"
+
+    # '&' has special semantics in sed replacement patterns
+    escaped_binary_download_url=$(echo "$binary_download_url" | sed 's/&/\\\&/')
+
+    # generate Dockerfile
+    sed \
+        -e "s,%%BINARY_DOWNLOAD_URL%%,${escaped_binary_download_url}," \
+        -e "s,%%ASC_DOWNLOAD_URL%%,$asc_download_url," \
+        -e "s/%%GPG_KEY%%/$gpg_key/" \
+        -e "s/%%CHECK_GPG%%/${check_gpg}/" \
+        -e "s/%%FROM_IMAGE%%/${from_docker_image}/" \
+        "$source_file.template" > "$dir/Dockerfile"
+
+    ########################################
+    ### generate "release.metadata" file
+    ########################################
+    
+    # docker image tags:
+    java11_suffix=""
+    if [[ $source_variant =~ "java11" ]] ; then
+        java11_suffix="-java11"
+    fi
+    # example "1.2.0-scala_2.11-java11"
+    full_tag=${flink_version}-scala_${scala_version}${java11_suffix}
+
+    # example "1.2-scala_2.11-java11"
+    short_tag=${flink_release}-scala_${scala_version}${java11_suffix}
+
+    # example "scala_2.12-java11"
+    scala_tag="scala_${scala_version}${java11_suffix}"
+
+    tags="$full_tag, $short_tag, $scala_tag"
+
+    if [[ "$scala_version" == "$LATEST_SCALA" ]]; then
+        # we are generating the image for the latest scala version, add:
+        # "1.2.0-java11"
+        # "1.2-java11"
+        # "latest-java11"
+        tags="$tags, ${flink_version}${java11_suffix}, ${flink_release}${java11_suffix}, latest${java11_suffix}"
+    fi
+
+    echo "Tags: $tags" >> $dir/release.metadata
+
+    # We currently only support amd64 with Flink.
+    echo "Architectures: amd64" >> $dir/release.metadata

Review comment:
       I'm not sure if we can get by with this; if we want ARM support (FLINK-14241) then we'll something more sophisticated. Not saying it has to be the docker magic we had before (which I'd kinda prefer not to use), but we should have a plan on how to support other architectures.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org