Grant Henke 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 backtic Done 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 comp Enhanced getopt requires `brew install gnu-getopt` on Mac. Given this is a rarely run release only script I though this simplified parsing was appropriate. 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 Done 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? Done 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 Done 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 Done 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. Done 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. Done 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? Done 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. Done -- 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 20:04:49 +0000 Gerrit-HasComments: Yes
