Attila Bukor has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/12173 )

Change subject: [docker] Add an initial docker build
......................................................................


Patch Set 3:

(3 comments)

I tried to build it on my Mac and kudu-thirdparty fails for some reason:

The command '/bin/sh -c thirdparty/build-if-necessary.sh   && rm -rf 
thirdparty/src' returned a non-zero code: 137
Chargers-MacBook-Pro:kudu charger$

http://gerrit.cloudera.org:8080/#/c/12173/3/docker/Dockerfile
File docker/Dockerfile:

http://gerrit.cloudera.org:8080/#/c/12173/3/docker/Dockerfile@32
PS3, Line 32: ARG BASE_OS=ubuntu:xenial
I'm curious, why do we default to ubuntu:xenial? In patch set one we used 
centos7.


http://gerrit.cloudera.org:8080/#/c/12173/3/docker/docker-build.sh
File docker/docker-build.sh:

http://gerrit.cloudera.org:8080/#/c/12173/3/docker/docker-build.sh@37
PS3, Line 37:   --build-arg MAINTAINER="Apache Kudu<[email protected]>"
nit: I think there should be a space between the name and the email


http://gerrit.cloudera.org:8080/#/c/12173/3/docker/docker-build.sh@45
PS3, Line 45: docker build "${BUILD_ARGS[@]}" -f $ROOT/docker/Dockerfile 
--target kudu-base -t kudu-base $ROOT
I believe the tag should be kudu:* instead of kudu-* and each image should have 
a tag with and one without the version, e.g.

"... -t kudu:thirdparty -t kudu:$VERSION-thirdparty kudu:latest-thirdparty 
$ROOT"

We should probably tag the kudu-runtime image without the runtime part to serve 
as a "default" image:

"... --target kudu-runtime -t kudu:latest -t kudu:$VERSION $ROOT"

Also I'm not convinced we need to tag kudu-thirdparty and kudu-build. It's good 
that they're in separate stages and If anyone wants to build them for some 
reason, they can still do it.



--
To view, visit http://gerrit.cloudera.org:8080/12173
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I95497b39e47f07301be75cbadd814656c7e2ea42
Gerrit-Change-Number: 12173
Gerrit-PatchSet: 3
Gerrit-Owner: Grant Henke <[email protected]>
Gerrit-Reviewer: Adar Dembo <[email protected]>
Gerrit-Reviewer: Attila Bukor <[email protected]>
Gerrit-Reviewer: Grant Henke <[email protected]>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 23 Jan 2019 12:19:31 +0000
Gerrit-HasComments: Yes

Reply via email to