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