Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12342 )
Change subject: [docker] Adjust Kudu image tagging ...................................................................... Patch Set 1: Code-Review+1 (7 comments) http://gerrit.cloudera.org:8080/#/c/12342/1/docker/docker-build.sh File docker/docker-build.sh: http://gerrit.cloudera.org:8080/#/c/12342/1/docker/docker-build.sh@63 PS1, Line 63: Optional images passed to the `docker build` commands : # via the `--cache-from` option. This option tells docker : # images to consider as cache sources. Not quite familiar with docker caching, but want to clarify: are these always pulled either from a local docker cache or a public docker artifactory specified by the user (that would perhaps be hosted by the Apache Kudu project in the future)? http://gerrit.cloudera.org:8080/#/c/12342/1/docker/docker-build.sh@80 PS1, Line 80: BASE_OS=${BASE_OS:="$DEFAULTS_OS"} Since we have this here, maybe we should get rid of the default arg in the Dockerfile? http://gerrit.cloudera.org:8080/#/c/12342/1/docker/docker-build.sh@84 PS1, Line 84: TARGETS=("base" "thirdparty" "build" "kudu") nit: Might be helpful to comment what is included in each image? E.g. how "base" is different from "thirdparty" and how "build" is different from "kudu" http://gerrit.cloudera.org:8080/#/c/12342/1/docker/docker-build.sh@123 PS1, Line 123: # Constructs a tag, ignoring a component where the argument is empty. : # Additionally ignores the target when it is the default target "kudu". Could you add a couple examples of expected inputs and outputs? http://gerrit.cloudera.org:8080/#/c/12342/1/docker/docker-build.sh@134 PS1, Line 134: if [[ -n "$VERSION_TAG" ]]; then Do we ever expect this to be empty? http://gerrit.cloudera.org:8080/#/c/12342/1/docker/docker-build.sh@146 PS1, Line 146: " nit: extra space http://gerrit.cloudera.org:8080/#/c/12342/1/docker/docker-build.sh@152 PS1, Line 152: remove the OS component of the tag. nit: thinking that there was one tag per image, I read this to mean that docker remove the FULL_TAG and replace it with one that was OS-less. Maybe reword as "If this is the default OS, also tag it without the OS-specific tag" -- To view, visit http://gerrit.cloudera.org:8080/12342 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4c96a3d5e1ca92022dc614c878f3853a4d99bcb8 Gerrit-Change-Number: 12342 Gerrit-PatchSet: 1 Gerrit-Owner: Grant Henke <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sun, 03 Feb 2019 03:53:42 +0000 Gerrit-HasComments: Yes
