Skip to content
This repository was archived by the owner on Dec 15, 2023. It is now read-only.

Address post-merge comments on PR #71 #74

Open
jo2y wants to merge 1 commit into
base: master
Choose a base branch
from
+32 −38
Open

Address post-merge comments on PR #71 #74

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 12 additions & 15 deletions appengine/java/appengine_deploy.sh.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,24 +24,21 @@ if [[ -z "$JAVA_RUNFILES" ]]; then
fi
fi

root_path=$(pwd)
tmp_dir=$(mktemp -d ${TMPDIR:-/tmp}/war.XXXXXXXX)
trap "{ cd ${root_path}; rm -rf ${tmp_dir}; }" EXIT
cd ${tmp_dir}
root_path="$(pwd)"
Copy link
Contributor

@rogerhub rogerhub Apr 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think quotes are needed for the right-hand side of assignments. Also, this could be simplified to root_path=$PWD, right?

tmp_dir=$(mktemp -d "${TMPDIR:-/tmp}/war.XXXXXXXX")
trap "{ cd "${root_path}"; rm -rf "${tmp_dir}"; }" EXIT
Copy link
Contributor

@rogerhub rogerhub Apr 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these double quotes aren't having their intended effect, since they're nested inside more double quotes. Ideally, this should look like:

cleanup() {
  cd "${root_path}"
  rm -rf "${tmp_dir}"
}
trap cleanup EXIT

That's a tad bit more complicated though, so I'll leave this up to your discretion.

cd "${tmp_dir}"

${JAVA_RUNFILES}/%{zipper} x ${JAVA_RUNFILES}/%{war}
cd ${root_dir}
"${JAVA_RUNFILES}/%{zipper}" x "${JAVA_RUNFILES}/%{war}"
cd "${root_dir}"

APP_ENGINE_ROOT=${JAVA_RUNFILES}/%{appengine_sdk}
APP_ENGINE_ROOT="${JAVA_RUNFILES}/%{appengine_sdk}"
if [ -n "${1-}" ]; then
${APP_ENGINE_ROOT}/bin/appcfg.sh -A "$1" update ${tmp_dir}
retCode=$?
"${APP_ENGINE_ROOT}/bin/appcfg.sh" -A "$1" update "${tmp_dir}"
Copy link
Contributor

@rogerhub rogerhub Apr 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a question: why is -A required in the python version but not here?

retCode="$?"
else
${APP_ENGINE_ROOT}/bin/appcfg.sh update ${tmp_dir}
retCode=$?
"${APP_ENGINE_ROOT}/bin/appcfg.sh" update "${tmp_dir}"
retCode="$?"
fi

rm -rf ${tmp_dir}
trap - EXIT

exit $retCode
exit "$retCode"
Copy link
Contributor

@rogerhub rogerhub Apr 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional: ret_code seems more consistent with other variables

8 changes: 4 additions & 4 deletions appengine/java/appengine_runner.sh.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ if [[ -z "$JAVA_RUNFILES" ]]; then
fi

jvm_bin="${JAVA_RUNFILES}/%{java}"
if [[ ! -x ${jvm_bin} ]]; then
jvm_bin=$(which java)
if [[ ! -x "${jvm_bin}" ]]; then
jvm_bin="$(which java)"
fi

APP_ENGINE_ROOT="${JAVA_RUNFILES}/%{appengine_sdk}"
Expand All @@ -41,7 +41,7 @@ classpath="%{classpath}"
cd "%{data_path}"

mkdir -p WEB-INF/lib
for i in $(echo $classpath | tr ":" "\n"); do
for i in $(echo "$classpath" | tr ":" "\n"); do
jar="WEB-INF/lib/$(basename "$i")"
rm -f "$jar"
ln -s "$i" "$jar"
Expand All @@ -54,7 +54,7 @@ JVM_FLAGS_CMDLINE=()
function process_wrapper_argument() {
case "$1" in
--jvm_flag=*) JVM_FLAGS_CMDLINE+=( "${1#--jvm_flag=}" ) ;;
--jvm_flags=*) JVM_FLAGS_CMDLINE+=( ${1#--jvm_flags=} ) ;;
--jvm_flags=*) JVM_FLAGS_CMDLINE+=( "${1#--jvm_flags=}" ) ;;
*)
return 1 ;;
esac
Expand Down
33 changes: 15 additions & 18 deletions appengine/py/appengine_deploy.sh.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,37 +19,34 @@ case "$0" in
esac
if [[ -e "$self.runfiles/%{workspace_name}" ]]; then
RUNFILES="$self.runfiles/%{workspace_name}"
cd $RUNFILES
cd "$RUNFILES"
fi

ROOT=$PWD
tmp_dir=$(mktemp -d ${{TMPDIR:-/tmp}}/war.XXXXXXXX)
cp -R $ROOT $tmp_dir
trap "{{ cd ${{root_path}}; rm -rf $tmp_dir; }}" EXIT
rm -Rf $tmp_dir/%{workspace_name}/external/com_google_appengine_py
if [ -n "${{1-}}" ]; then
has_app_yaml=$(echo ${{@:2}} | grep -E "^([^ ]+ +)*app.yaml")
other_mod_cfgs=$(echo ${{@:2}} | xargs printf "%s\n" | grep -v "^app\.yaml$" | xargs echo)
ROOT="$PWD"
Copy link
Contributor

@rogerhub rogerhub Apr 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe root_path to match the Java version?

tmp_dir="$(mktemp -d "${TMPDIR:-/tmp}/war.XXXXXXXX")"
cp -R "$ROOT" "$tmp_dir"
trap "{ cd "$ROOT"; rm -rf "$tmp_dir"; }}" EXIT
rm -Rf "$tmp_dir/%{workspace_name}/external/com_google_appengine_py"
Copy link
Contributor

@rogerhub rogerhub Apr 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: lowercase -rf seems more consistent with other usages

if [ -n "${1-}" ]; then
has_app_yaml="$(echo "${@:2}" | grep -E "^([^ ]+ +)*app.yaml")"
other_mod_cfgs="$(echo "${@:2}" | xargs printf "%s\n" | grep -v "^app\.yaml$" | xargs echo)"
Copy link
Contributor

@rogerhub rogerhub Apr 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional: Some of this code is brittle against paths with spaces in them, but users probably shouldn't be using those anyway. If you really care, there's probably a way to do all this in just bash. Maybe something like:

has_app_yaml=false
other_mod_cfgs=()

for p in "${@:2}"; do
  if [[ "$p" == "app.yaml" ]]; then
    has_app_yaml=true
  else
    modules+=("$p")
  fi
done

And then you can use ${other_mod_cfgs[@]} to pass all of the other .yaml files to appcfg update below.

ret_code=0
# Secondary modules need to be uploaded first because if any of them are
# referenced in dispatch.yaml, App Engine must have a version of that module
# prior to the app.yaml upload.
if [ -n "$other_mod_cfgs" ]; then
(cd $tmp_dir/%{workspace_name} && $ROOT/%{appcfg} -A "$1" update $other_mod_cfgs)
ret_code=$?
(cd "$tmp_dir/%{workspace_name}" && "$ROOT/%{appcfg}" -A "$1" update "$other_mod_cfgs")
ret_code="$?"
fi
if [ $ret_code -eq 0 ] && [ -n "$has_app_yaml" ] || [ -z "$other_mod_cfgs" ]; then
$ROOT/%{appcfg} -A "$1" update $tmp_dir/%{workspace_name}
ret_code=$?
if [ "$ret_code" -eq 0 ] && [ -n "$has_app_yaml" ] || [ -z "$other_mod_cfgs" ]; then
"$ROOT/%{appcfg}" -A "$1" update "$tmp_dir/%{workspace_name}"
ret_code="$?"
fi

else
echo "\033[1;31mERROR:\033[0m Application ID must be provided as first argument
echo -e "\033[1;31mERROR:\033[0m Application ID must be provided as first argument
USAGE: bazel run path/to/my/gae_binary_target.deploy -- my-project-id [module.yaml files ...]"
ret_code=-1
fi

rm -Rf $tmp_dir
trap - EXIT

exit $ret_code
2 changes: 1 addition & 1 deletion appengine/py/appengine_runner.sh.template
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ case "$0" in
esac
if [[ -e "$self.runfiles/%{workspace_name}" ]]; then
RUNFILES="$self.runfiles/%{workspace_name}"
cd $RUNFILES
cd "$RUNFILES"
fi

%{devappserver} --skip_sdk_update_check 1 app.yaml