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



##########
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:
       For ARM support (as shown in #23) we are using a different `FROM` image, 
which implies that we need different `Architecture` values per image.

##########
File path: generator.sh
##########
@@ -0,0 +1,80 @@
+#!/bin/bash -e
+
+export SOURCE_VARIANTS=(debian )
+
+export LATEST_SCALA="2.12"
+export LATEST_JAVA="8"
+
+# generate "Dockerfile" file
+function generateDockerfile {
+    # define variables
+    dir=$1
+    binary_download_url=$2
+    asc_download_url=$3
+    gpg_key=$4
+    check_gpg=$5
+    java_version=$6
+    source_variant=$7
+
+    from_docker_image="openjdk:${java_version}-jre"
+
+    source_file="Dockerfile-debian"
+
+    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"

Review comment:
       ```suggestion
           "Dockerfile-$source_variant.template" > "$dir/Dockerfile"
   ```

##########
File path: generator.sh
##########
@@ -0,0 +1,80 @@
+#!/bin/bash -e
+
+export SOURCE_VARIANTS=(debian )
+
+export LATEST_SCALA="2.12"
+export LATEST_JAVA="8"

Review comment:
       rename to `DEFAULT_JAVA`? (same for scala)

##########
File path: add-version.sh
##########
@@ -98,18 +98,24 @@ fi
 
 mkdir "$flink_release"
 
+source "$(dirname "$0")"/generator.sh
+
 echo -n >&2 "Generating Dockerfiles..."
 for source_variant in "${SOURCE_VARIANTS[@]}"; do
     for scala_version in "${scala_versions[@]}"; do
-        dir="$flink_release/scala_${scala_version}-${source_variant}"
+        for java_version in "${java_versions[@]}"; do
+            
dir="$flink_release/scala_${scala_version}-java${java_version}-${source_variant}"
 
-        
flink_url_file_path=flink/flink-${flink_version}/flink-${flink_version}-bin-scala_${scala_version}.tgz
+            
flink_url_file_path=flink/flink-${flink_version}/flink-${flink_version}-bin-scala_${scala_version}.tgz
 
-        
flink_tgz_url="https://www.apache.org/dyn/closer.cgi?action=download&filename=${flink_url_file_path}";
-        # Not all mirrors have the .asc files
-        flink_asc_url=https://www.apache.org/dist/${flink_url_file_path}.asc
+            
flink_tgz_url="https://www.apache.org/dyn/closer.cgi?action=download&filename=${flink_url_file_path}";
+            # Not all mirrors have the .asc files
+            
flink_asc_url=https://www.apache.org/dist/${flink_url_file_path}.asc
 
-        generate "${dir}" "${flink_tgz_url}" "${flink_asc_url}" ${gpg_key} 
true ${source_variant}
+            mkdir "$dir"
+            generateDockerfile "${dir}" "${flink_tgz_url}" "${flink_asc_url}" 
${gpg_key} true ${java_version} ${source_variant}
+            generateReleaseMetadata ${dir} ${flink_release} ${flink_version} 
${scala_version} ${java_version} ${source_variant}

Review comment:
       ```suggestion
               generateReleaseMetadata "${dir}" ${flink_release} 
${flink_version} ${scala_version} ${java_version} ${source_variant}
   ```

##########
File path: generator.sh
##########
@@ -0,0 +1,80 @@
+#!/bin/bash -e
+
+export SOURCE_VARIANTS=(debian )
+
+export LATEST_SCALA="2.12"
+export LATEST_JAVA="8"
+
+# generate "Dockerfile" file
+function generateDockerfile {
+    # define variables
+    dir=$1
+    binary_download_url=$2
+    asc_download_url=$3
+    gpg_key=$4
+    check_gpg=$5
+    java_version=$6
+    source_variant=$7
+
+    from_docker_image="openjdk:${java_version}-jre"
+
+    source_file="Dockerfile-debian"
+

Review comment:
       ```suggestion
   ```

##########
File path: generator.sh
##########
@@ -0,0 +1,80 @@
+#!/bin/bash -e
+
+export SOURCE_VARIANTS=(debian )
+
+export LATEST_SCALA="2.12"
+export LATEST_JAVA="8"
+
+# generate "Dockerfile" file
+function generateDockerfile {
+    # define variables
+    dir=$1
+    binary_download_url=$2
+    asc_download_url=$3
+    gpg_key=$4
+    check_gpg=$5
+    java_version=$6
+    source_variant=$7
+
+    from_docker_image="openjdk:${java_version}-jre"
+
+    source_file="Dockerfile-debian"
+
+    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

Review comment:
       ```suggestion
   ```

##########
File path: add-custom.sh
##########
@@ -43,7 +43,7 @@ echo -n >&2 "Generating Dockerfiles..."
 for source_variant in "${SOURCE_VARIANTS[@]}"; do
   dir="dev/${name}-${source_variant}"
   rm -rf "${dir}"
-
-  generate "${dir}" "${binary_download_url}" "" "" false ${source_variant}
+  mkdir "$dir"
+  generateDockerfile "${dir}" "${binary_download_url}" "" "" false 8 
${source_variant}

Review comment:
       A separate parameter makes sense to me, but then please also adjust the 
tests to cover both java 8 and 11.




----------------------------------------------------------------
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:
[email protected]


Reply via email to