Copilot commented on code in PR #10449:
URL: https://github.com/apache/gravitino/pull/10449#discussion_r2945074498
##########
dev/release/do-release.sh:
##########
@@ -20,22 +22,46 @@
# Referred from Apache Spark's release script
# dev/create-release/do-release.sh
-SELF=$(cd $(dirname $0) && pwd)
+RUNNING_IN_DOCKER=${RUNNING_IN_DOCKER:-0}
+
+SELF=$(cd "$(dirname "$0")" && pwd)
-while getopts ":b:n" opt; do
+while getopts ":b:s:p:t:r:nyh" opt; do
case $opt in
b) GIT_BRANCH=$OPTARG ;;
n) DRY_RUN=1 ;;
+ s) RELEASE_STEP=$OPTARG ;;
+ p) GPG_PASSPHRASE=$OPTARG ;;
+ t) ASF_PASSWORD=$OPTARG ;;
+ r) RC_COUNT=$OPTARG ;;
+ y) FORCE=1 ;;
+ h)
+ echo "Usage: $0 [options]"
+ echo "Options:"
+ echo " -b <branch> Git branch to release (e.g., branch-1.2)"
+ echo " -s <step> Release step to execute: tag, build, docs,
publish, finalize"
+ echo " -r <num> Release candidate number (e.g., 6 for rc6)"
+ echo " -n Dry run mode"
+ echo " -p <pass> GPG passphrase"
+ echo " -t <pass> ASF password"
+ echo " -y Force continue without confirmation"
+ echo " -h Show this help message"
+ exit 0
+ ;;
\?) error "Invalid option: $OPTARG" ;;
esac
done
-DRY_RUN=${DRY_RUN:-0}
-export DRY_RUN
+export DRY_RUN=${DRY_RUN:-0}
+export FORCE=${FORCE:-0}
+export RC_COUNT=${RC_COUNT:-0}
+export RELEASE_STEP=${RELEASE_STEP:-}
Review Comment:
`-b`, `-p`, and `-t` options currently only assign shell variables; they are
not exported. If `get_release_info` is skipped (e.g., non-interactive runs
where branch/versions are already provided), child scripts (`release-tag.sh` /
`release-build.sh`) won’t see `GIT_BRANCH`, `ASF_PASSWORD`, or `GPG_PASSPHRASE`
and may block prompting. Export these option-provided values (or explicitly
document that callers must provide them via environment).
##########
dev/release/do-release.sh:
##########
@@ -20,22 +22,46 @@
# Referred from Apache Spark's release script
# dev/create-release/do-release.sh
-SELF=$(cd $(dirname $0) && pwd)
+RUNNING_IN_DOCKER=${RUNNING_IN_DOCKER:-0}
+
+SELF=$(cd "$(dirname "$0")" && pwd)
-while getopts ":b:n" opt; do
+while getopts ":b:s:p:t:r:nyh" opt; do
case $opt in
b) GIT_BRANCH=$OPTARG ;;
n) DRY_RUN=1 ;;
+ s) RELEASE_STEP=$OPTARG ;;
+ p) GPG_PASSPHRASE=$OPTARG ;;
+ t) ASF_PASSWORD=$OPTARG ;;
+ r) RC_COUNT=$OPTARG ;;
+ y) FORCE=1 ;;
Review Comment:
`getopts` is invoked with a leading `:` (silent error mode) and several
options that require arguments, but the `case` doesn’t handle the `:)` branch
for “missing option argument”. In that scenario the script can proceed with
unset vars (and under `set -u` may crash). Add explicit handling for `:)` to
print usage and exit non-zero.
##########
dev/release/release-util.sh:
##########
@@ -115,33 +129,40 @@ function get_release_info {
local PREV_REL_REV=$((REV - 1))
local PREV_REL_TAG="v${SHORT_VERSION}.${PREV_REL_REV}"
if check_for_tag "$PREV_REL_TAG"; then
- RC_COUNT=1
+ NRC_COUNT=1
REV=$((REV + 1))
NEXT_VERSION="${SHORT_VERSION}.${REV}-SNAPSHOT"
else
RELEASE_VERSION="${SHORT_VERSION}.${PREV_REL_REV}"
- RC_COUNT=$(git ls-remote --tags "$ASF_REPO" "v${RELEASE_VERSION}-rc*" |
wc -l)
- RC_COUNT=$((RC_COUNT + 1))
+ NRC_COUNT=$(git ls-remote --tags "$ASF_REPO" "v${RELEASE_VERSION}-rc*" |
wc -l)
+ NRC_COUNT=$((NRC_COUNT + 1))
fi
else
REV=$((REV + 1))
NEXT_VERSION="${SHORT_VERSION}.${REV}-SNAPSHOT"
- RC_COUNT=1
+ NRC_COUNT=1
fi
export NEXT_VERSION
- export RELEASE_VERSION=$(read_config "Release" "$RELEASE_VERSION")
+ export RELEASE_VERSION=$(read_config "Release" "$RELEASE_VERSION"
RELEASE_VERSION)
- RC_COUNT=$(read_config "RC #" "$RC_COUNT")
+ if [ "$FORCE" = "1" ]; then
+ NRC_COUNT=$RC_COUNT
+ fi
+ RC_COUNT=$(read_config "RC #" "$RC_COUNT" NRC_COUNT)
Review Comment:
RC count auto-detection is currently not wired into the prompt/default/env
override correctly: `NRC_COUNT` is computed, but `read_config` is called with
default `$RC_COUNT` and env var name `NRC_COUNT`, so the suggested default
becomes 0 (from do-release) and `-r/RC_COUNT` won’t be honored in
force/non-interactive runs. Use the computed count as the default, and use
`RC_COUNT` consistently as the override env var/CLI value (or export
`NRC_COUNT` if you truly want a separate variable).
##########
dev/release/release-util.sh:
##########
@@ -155,23 +176,23 @@ function get_release_info {
if [[ $SKIP_TAG = 0 ]]; then
GIT_REF="$GIT_BRANCH"
fi
- GIT_REF=$(read_config "Ref" "$GIT_REF")
+ GIT_REF=$(read_config "Ref" "$GIT_REF" GIT_REF)
fi
export GIT_REF
export GRAVITINO_PACKAGE_VERSION="$RELEASE_TAG"
# Gather some user information.
if [ -z "$ASF_USERNAME" ]; then
- export ASF_USERNAME=$(read_config "ASF user" "$LOGNAME")
+ export ASF_USERNAME=$(read_config "ASF user" "$LOGNAME" ASF_USERNAME)
fi
if [ -z "$GIT_NAME" ]; then
GIT_NAME=$(git config user.name || echo "")
- export GIT_NAME=$(read_config "Full name" "$GIT_NAME")
+ export GIT_NAME=$(read_config "Full name" "$GIT_NAME" GIT_NAME)
Review Comment:
`do-release.sh` enables `set -u`, but this function still checks
potentially-unset vars with `[ -z "$VAR" ]` (e.g., `ASF_USERNAME`, `GIT_NAME`).
With nounset enabled in the caller, these will raise “unbound variable” before
prompting/reading defaults. Use `${ASF_USERNAME:-}` / `${GIT_NAME:-}` (and
similarly for `ASF_PASSWORD`, `GPG_PASSPHRASE`, etc.) in emptiness checks so
interactive mode doesn’t crash.
##########
dev/release/do-release.sh:
##########
@@ -67,8 +93,15 @@ if [ "$RUNNING_IN_DOCKER" = "1" ]; then
export JAVA_HOME=/usr
fi
else
- # Outside docker, need to ask for information about the release.
- get_release_info
+ # Outside docker, need to ask for information about the release if required
vars are missing.
+ # Set default values to avoid unbound variable errors with set -u
+ RELEASE_VERSION=${RELEASE_VERSION:-}
+ GIT_BRANCH=${GIT_BRANCH:-}
+ RC_COUNT=${RC_COUNT:-0}
+
+ if [ -z "$GIT_BRANCH" ] || [ -z "$RELEASE_VERSION" ] || [ -z "$RC_COUNT" ];
then
+ get_release_info
+ fi
Review Comment:
The pre-check for whether to call `get_release_info` treats `RC_COUNT=0` as
“present” because `-z "0"` is false, so the script can skip `get_release_info`
and proceed with an invalid rc0 tag. Consider treating `RC_COUNT` as missing
when it’s unset or <= 0 (and/or validate it as a positive integer before
proceeding).
##########
dev/release/do-release.sh:
##########
@@ -1,5 +1,7 @@
#!/usr/bin/env bash
+set -euo pipefail
Review Comment:
Adding `set -euo pipefail` changes failure semantics for `run_silent` (from
release-util.sh): a failing command inside `run_silent` will cause an immediate
exit due to `set -e` before the function can tail the log and print the helpful
“Command FAILED” message. Either rework `run_silent` to use an `if ! ...; then
...` pattern / temporarily disable `errexit` inside it, or avoid `set -e` here.
##########
dev/release/do-release.sh:
##########
@@ -67,8 +93,15 @@ if [ "$RUNNING_IN_DOCKER" = "1" ]; then
export JAVA_HOME=/usr
fi
else
Review Comment:
With `set -u`, the docker path will currently crash if `GPG_PASSPHRASE` or
`JAVA_HOME` are unset (e.g., `echo $GPG_PASSPHRASE | ...` and `[ -n
"$JAVA_HOME" ]` both expand unset vars). Use `${GPG_PASSPHRASE:-}` /
`${JAVA_HOME:-}` in those checks, and/or explicitly validate that required vars
are set before use in docker mode.
##########
dev/release/do-release.sh:
##########
@@ -20,22 +22,46 @@
# Referred from Apache Spark's release script
# dev/create-release/do-release.sh
-SELF=$(cd $(dirname $0) && pwd)
+RUNNING_IN_DOCKER=${RUNNING_IN_DOCKER:-0}
+
+SELF=$(cd "$(dirname "$0")" && pwd)
-while getopts ":b:n" opt; do
+while getopts ":b:s:p:t:r:nyh" opt; do
case $opt in
b) GIT_BRANCH=$OPTARG ;;
n) DRY_RUN=1 ;;
+ s) RELEASE_STEP=$OPTARG ;;
+ p) GPG_PASSPHRASE=$OPTARG ;;
+ t) ASF_PASSWORD=$OPTARG ;;
+ r) RC_COUNT=$OPTARG ;;
+ y) FORCE=1 ;;
+ h)
+ echo "Usage: $0 [options]"
+ echo "Options:"
+ echo " -b <branch> Git branch to release (e.g., branch-1.2)"
+ echo " -s <step> Release step to execute: tag, build, docs,
publish, finalize"
+ echo " -r <num> Release candidate number (e.g., 6 for rc6)"
+ echo " -n Dry run mode"
+ echo " -p <pass> GPG passphrase"
+ echo " -t <pass> ASF password"
+ echo " -y Force continue without confirmation"
+ echo " -h Show this help message"
+ exit 0
+ ;;
\?) error "Invalid option: $OPTARG" ;;
esac
Review Comment:
The invalid-option branch calls `error`, but `release-util.sh` (which
defines `error`) is sourced later. If an unknown flag is passed, the script
will fail with `error: command not found` instead of a clean message. Define a
local `error`/usage helper before `getopts`, or replace this case with `echo
...; exit 1`.
##########
dev/release/do-release.sh:
##########
@@ -20,22 +22,46 @@
# Referred from Apache Spark's release script
# dev/create-release/do-release.sh
-SELF=$(cd $(dirname $0) && pwd)
+RUNNING_IN_DOCKER=${RUNNING_IN_DOCKER:-0}
+
+SELF=$(cd "$(dirname "$0")" && pwd)
-while getopts ":b:n" opt; do
+while getopts ":b:s:p:t:r:nyh" opt; do
case $opt in
b) GIT_BRANCH=$OPTARG ;;
n) DRY_RUN=1 ;;
+ s) RELEASE_STEP=$OPTARG ;;
+ p) GPG_PASSPHRASE=$OPTARG ;;
+ t) ASF_PASSWORD=$OPTARG ;;
+ r) RC_COUNT=$OPTARG ;;
+ y) FORCE=1 ;;
+ h)
+ echo "Usage: $0 [options]"
+ echo "Options:"
+ echo " -b <branch> Git branch to release (e.g., branch-1.2)"
+ echo " -s <step> Release step to execute: tag, build, docs,
publish, finalize"
+ echo " -r <num> Release candidate number (e.g., 6 for rc6)"
+ echo " -n Dry run mode"
+ echo " -p <pass> GPG passphrase"
+ echo " -t <pass> ASF password"
Review Comment:
`-p` (GPG passphrase) and `-t` (ASF password) accept secrets via
command-line args, which are commonly exposed via shell history and process
listings (`ps`). For non-interactive usage, prefer reading these from
environment variables or stdin, and at minimum warn in the help text/docs that
these flags are insecure on multi-user systems.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
