[GitHub] [hbase] mattf-apache commented on a change in pull request #1726: HBASE-24296 install yetus as a part of building the rm docker image.

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

mattf-apache commented on a change in pull request #1726:
URL: https://github.com/apache/hbase/pull/1726#discussion_r426958791



##########
File path: dev-support/create-release/release-util.sh
##########
@@ -423,17 +430,14 @@ function get_jira_name {
 
 # Update the CHANGES.md
 # DOES NOT DO COMMITS! Caller should do that.
+# requires yetus to have a defined home already.

Review comment:
       In order to make this use of YETUS_HOME work okay when not doing a Docker build, that is when invoking `do-release.sh` directly, it is necessary to collect the value of YETUS_HOME in `release_util.sh:get_release_info()`.  Perhaps you could add at line 232 something like:
   ```
     if [[ -z "$YETUS_HOME" && "$RUNNING_IN_DOCKER" != "1" ]]; then
       peer_dir="$(cd "$SELF/../../../ & echo `pwd`/yetus)"
       YETUS_HOME="$(read_config "YETUS_HOME" "$peer_dir")
       export YETUS_HOME
   fi
   ```
   
   Then in `do-release-docker.sh`, before the invocation of `get_release_info` in Line 116, add the line
   ```
   RUNNING_IN_DOCKER=1
   ```

##########
File path: dev-support/create-release/hbase-rm/Dockerfile
##########
@@ -43,6 +43,12 @@ RUN DEBIAN_FRONTEND=noninteractive apt-get -qq -y update \
   && update-alternatives --set java /usr/lib/jvm/java-8-openjdk-amd64/jre/bin/java \
   && pip install \
     python-dateutil==2.8.1
+# Install Apache Yetus
+ENV YETUS_VERSION 0.11.1
+SHELL ["/bin/bash", "-o", "pipefail", "-c"]
+RUN wget -qO- "https://www.apache.org/dyn/mirrors/mirrors.cgi?action=download&filename=/yetus/${YETUS_VERSION}/apache-yetus-${YETUS_VERSION}-bin.tar.gz" | \
+        tar xvz -C /opt
+ENV YETUS_HOME /opt/apache-yetus-${YETUS_VERSION}

Review comment:
       I don't really understand the value of pulling it at docker build time vs during the build:
   - The reason that occurred to me is maybe concern that the Docker container won't have internet access.  But the Docker container must have internet access, or it can't do anything other than a dry run.
   - It takes the same amount of time to load it in the Dockerfile vs during the job.
   - It breaks locality between where it's needed and where it's loaded.
   - and therefore spends that time to load it, even if tagging isn't going to be invoked.
   - It introduces another magic variable that needs to be pre-set before running the non-docker build.
   
   I don't feel strongly about these reasons, except for the last one.  If you really want to do this despite the arguments above, go ahead, but please accept the comments in the remainder of this review, which address the problem with that env variable.

##########
File path: dev-support/create-release/do-release-docker.sh
##########
@@ -75,6 +75,7 @@ Options:
                If none specified, runs tag, then publish-dist, and then publish-release.
                'publish-snapshot' is also an allowed, less used, option.
 EOF
+  exit 0

Review comment:
       I agree with making the exit explicit.  But if I'd thought of it, I probably would have made it exit with an error status, in case it is accidentally invoked by a build process that probably shouldn't report all-okay.




----------------------------------------------------------------
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