Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/12325 )
Change subject: [build] Add a script to publish the kudu-binary jars ...................................................................... Patch Set 1: (10 comments) http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh File build-support/mini-cluster/publish_mini_cluster_binaries.sh: http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@59 PS1, Line 59: VERSION=`cat ${SOURCE_ROOT}/version.txt` New bash scripts should prefer $(...) for command substitution over backticks. Below too. http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@67 PS1, Line 67: # Parse the command line parameters. I'd recommend using getopt for this; it's the most idiomatic way to do complex parameter parsing in shell. I presume it's portable to macOS? Here's a decent example of how it works: https://gist.github.com/madx/2767550 http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@102 PS1, Line 102: exit 1 Nit: indentation http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@105 PS1, Line 105: if [[ -z ${MVN_USERNAME} ]]; then Curious why you're using braces here but not above? Also, do these expansions need to be surrounded by quotes? If not, what happens when they're not set? Doesn't bash wind up evaluating something looking like "if [[ -z ]]; then"? http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@132 PS1, Line 132: local PROP=`unzip -q -c ${JAR} ${PROP_FILE} | grep "$KEY" | cut -d'=' -f2` Validate that the JAR exists first, and provide a nice error message if it doesn't? http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@145 PS1, Line 145: mvn install:install-file ${@} Maybe validate that 'mvn' is on the path up front, and provide a nice error message if it's not? http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@160 PS1, Line 160: # Create a minimal POM file. Nit: indentation. http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@162 PS1, Line 162: rm -f ${POM_FILE} Not necessary given the non-append redirection below. http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@182 PS1, Line 182: MVN_ARGS="$MVN_ARGS -Dfile=$POM_FILE" : MVN_ARGS="$MVN_ARGS -Dpackaging=pom" Won't this leak into the JAR publishing done below? Is that intentional? http://gerrit.cloudera.org:8080/#/c/12325/1/build-support/mini-cluster/publish_mini_cluster_binaries.sh@191 PS1, Line 191: JAR_VERSION=`read_prop_or_die "$JAR" "artifact.version"` This isn't used. -- To view, visit http://gerrit.cloudera.org:8080/12325 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I83fe6b81ef70ec75b10f66fa8582a861daf285e4 Gerrit-Change-Number: 12325 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Brian McDevitt <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Comment-Date: Thu, 31 Jan 2019 19:16:09 +0000 Gerrit-HasComments: Yes
