Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/12342 )
Change subject: [docker] Adjust Kudu image tagging ...................................................................... Patch Set 3: (11 comments) http://gerrit.cloudera.org:8080/#/c/12342/3/docker/README.adoc File docker/README.adoc: http://gerrit.cloudera.org:8080/#/c/12342/3/docker/README.adoc@65 PS3, Line 65: kudu-thirdparty > kudu-thirdparty is no longer the right name. Nor is kudu-base below. I will fix this in my compose patch. http://gerrit.cloudera.org:8080/#/c/12342/3/docker/README.adoc@105 PS3, Line 105: images : > images: I will fix this in my compose patch. 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 alwa By default the docker cache will leverage it's own contents from previous builds. This lets you specify local or remote images that you didn't build yourself to be considered for the cache. 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 Done 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 " Each target is defined in the Dockerfile and the readme. http://gerrit.cloudera.org:8080/#/c/12342/1/docker/docker-build.sh@123 PS1, Line 123: # Constructs a tag, excluding the OS_TAG if it is empty. : # Additionally ignores the target when it is the default target "kudu". > Could you add a couple examples of expected inputs and outputs? Done http://gerrit.cloudera.org:8080/#/c/12342/1/docker/docker-build.sh@134 PS1, Line 134: # Only include the target if thi > Do we ever expect this to be empty? Done http://gerrit.cloudera.org:8080/#/c/12342/1/docker/docker-build.sh@146 PS1, Line 146: > nit: extra space Done http://gerrit.cloudera.org:8080/#/c/12342/1/docker/docker-build.sh@152 PS1, Line 152: L_TAG" ${ROOT} > nit: thinking that there was one tag per image, I read this to mean that do Done http://gerrit.cloudera.org:8080/#/c/12342/3/docker/kudu-entrypoint.sh File docker/kudu-entrypoint.sh: http://gerrit.cloudera.org:8080/#/c/12342/3/docker/kudu-entrypoint.sh@42 PS3, Line 42: echo "MASTER_ARGS:" : echo " Defines the arguments passed to kudu-master. Defaults to DEFAULT_ARGS" : echo "TSERVER_ARGS:" : echo " Defines the arguments passed to kudu-tserver. Defaults to DEFAULT_ARGS" > Maybe more clear as "Defaults to the value of the DEFAULT_ARGS environment Will fix in my docker-compose patch. http://gerrit.cloudera.org:8080/#/c/12342/3/docker/kudu-entrypoint.sh@48 PS3, Line 48: exit 0 > Don't we want a non-zero exit status here? Well, we don't want it if the he Will fix in my docker compose patch. -- 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: 3 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: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Comment-Date: Sun, 03 Feb 2019 20:31:18 +0000 Gerrit-HasComments: Yes
