Allow generating the docker image via make. by igorpeshansky · Pull Request #31 · Stackdriver/metadata-agent

igorpeshansky · 2017-09-14T23:59:29Z

Remove the package from the docker image once installed.

dhrupadb · 2017-09-15T00:19:51Z

@@ -0,0 +1,13 @@
+FROM ubuntu:trusty


I think we should add a comment of some sort saying that this dockerfile cannot be used directly and is used as a part of the Makefile.

You can absolutely use this directly:

curl -O http://.../stackdriver-metadata_${version}.trusty.deb \ && docker build --build-arg package=stackdriver-metadata_${version}.trusty.deb -t stackdriver-metadata-agent:${version}-trusty -f Dockerfile-trusty .

You just have to supply the correct parameters. :-)

Ah didn't know that docker build would take a dockerfile not names Dockerfile. Can we add an instruction as a comment as to use this directly?

dhrupadb · 2017-09-15T00:20:23Z


 rpm: $(RPM_PKG)

+ifeq ($(findstring |$(DISTRO)|,|trusty|xenial|),)


Any reason we're building an image for trusty as well? Once it's in a container it shouldn't matter what the underlying distro is no?

The idea is to build the container with the version of the agent you've just built locally, e.g., for testing. My machine is trusty, so the package I'll build will be for ubuntu:trusty. We should eventually add Dockerfiles for CentOS distros too.

Makes sense. Thanks.

I will have to take a step back and try to reinforce what Dhrupad said though, why don't we just make a dockerfile that encompasses the entire build itself? Being a user on mac, I certainly don't want to have to build this on my machine and then copy that package into the docker container. I have a dockerfile locally that does the entire build locally for context in paste "5195402634592256"

We could add one like that as well. But the purpose of these specific Dockerfiles is to build locally. I'm happy to put them into a subdirectory of pkg/docker, e.g., pkg/docker/local if you want them distinct from the "official" Dockerfiles.

After an offline discussion, unified the "official" Dockerfile with Dockerfile-xenial. The issue with hermetic build is orthogonal to this, so let's address that separately.

dhrupadb

LGTM 👌

dhrupadb · 2017-09-15T00:45:20Z

@@ -0,0 +1,13 @@
+FROM ubuntu:trusty


Ah didn't know that docker build would take a dockerfile not names Dockerfile. Can we add an instruction as a comment as to use this directly?

dhrupadb · 2017-09-15T00:45:38Z


 rpm: $(RPM_PKG)

+ifeq ($(findstring |$(DISTRO)|,|trusty|xenial|),)


Makes sense. Thanks.

igorpeshansky

Added comments. PTAL.

igorpeshansky · 2017-09-15T02:16:21Z

@@ -0,0 +1,13 @@
+FROM ubuntu:trusty


qingling128

LGTM.

…ntainer package filename.

igorpeshansky

Addressed feedback. PTAL.

igorpeshansky · 2017-09-15T16:22:07Z


 rpm: $(RPM_PKG)

+ifeq ($(findstring |$(DISTRO)|,|trusty|xenial|),)


After an offline discussion, unified the "official" Dockerfile with Dockerfile-xenial. The issue with hermetic build is orthogonal to this, so let's address that separately.

…ake conditional.

igorpeshansky

Made the build even simpler. PTAL.

bmoyles0117 · 2017-09-15T16:37:39Z

+# To use directly, run: docker build --build-arg package=<pkg>.trusty.deb -t <tag> -f Dockerfile-trusty
+FROM ubuntu:trusty
+
+ARG package


Do we not have a default package to set this to?

Not for trusty. Our official packages are only xenial and centos7.

bmoyles0117 · 2017-09-15T16:39:29Z

+
+ARG package
+COPY ${package} /stackdriver-metadata.deb
+RUN apt-get update \


To ensure consistency going forward, should we have an install.sh in this directory that runs these commands, and change this to something like below? If you feel it's better to be explicit that's fine by me too, just throwing out the idea.

ADD install.sh RUN bash install.sh

The commands are different for different install schemas. We'd have to have two install.sh scripts, so we might as well embed the commands in the Dockerfiles.

igorpeshansky

Responded. PTAL.

igorpeshansky · 2017-09-15T17:43:23Z

+# To use directly, run: docker build --build-arg package=<pkg>.trusty.deb -t <tag> -f Dockerfile-trusty
+FROM ubuntu:trusty
+
+ARG package


Not for trusty. Our official packages are only xenial and centos7.

igorpeshansky · 2017-09-15T17:45:39Z

+
+ARG package
+COPY ${package} /stackdriver-metadata.deb
+RUN apt-get update \


The commands are different for different install schemas. We'd have to have two install.sh scripts, so we might as well embed the commands in the Dockerfiles.

bmoyles0117 · 2017-09-15T19:12:02Z

+FROM ubuntu:trusty
+
+ARG package
+COPY ${package} /stackdriver-metadata.deb


Final nit: we should use "ADD" in both Dockerfiles, just to allow for the flexibility if their parameter is indeed a URL.

Whoops, right you are. That wasn't intentional. Fixed.

dhrupadb

Pending resolution of remaining comments, LGTM 👌

bmoyles0117

LGTM

Allow generating the docker image via make.

e1b8258

Remove the package from the docker image once installed.

igorpeshansky requested review from bmoyles0117 and dhrupadb September 14, 2017 23:59

dhrupadb suggested changes Sep 15, 2017

View reviewed changes

dhrupadb approved these changes Sep 15, 2017

View reviewed changes

Add comments for standalone use.

df84d88

igorpeshansky commented Sep 15, 2017

View reviewed changes

Comment thread pkg/docker/Dockerfile-trusty

@@ -0,0 +1,13 @@

FROM ubuntu:trusty

Copy link
Copy Markdown

Contributor Author

igorpeshansky Sep 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

qingling128 approved these changes Sep 15, 2017

View reviewed changes

Unify Dockerfile and Dockerfile-xenial; remove version from the in-co…

6c4b7d4

…ntainer package filename.

igorpeshansky commented Sep 15, 2017

View reviewed changes

igorpeshansky added 2 commits September 15, 2017 12:35

Rely on the presence of a properly suffixed Dockerfile instead of a m…

dd28512

…ake conditional.

Split variable assignment from export to retain the exit code of mktemp.

54b96e9

igorpeshansky commented Sep 15, 2017

View reviewed changes

bmoyles0117 reviewed Sep 15, 2017

View reviewed changes

igorpeshansky commented Sep 15, 2017

View reviewed changes

bmoyles0117 reviewed Sep 15, 2017

View reviewed changes

dhrupadb approved these changes Sep 15, 2017

View reviewed changes

Oops. COPY->ADD.

b780e9e

bmoyles0117 approved these changes Sep 16, 2017

View reviewed changes

igorpeshansky merged commit 10bd4ff into master Sep 16, 2017

igorpeshansky deleted the igorp-docker-make branch September 16, 2017 00:46


		rpm: $(RPM_PKG)

		ifeq ($(findstring \|$(DISTRO)\|,\|trusty\|xenial\|),)

Conversation

igorpeshansky commented Sep 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky Sep 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhrupadb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qingling128 left a comment

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igorpeshansky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhrupadb left a comment

Choose a reason for hiding this comment

Uh oh!

bmoyles0117 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

igorpeshansky Sep 15, 2017 •

edited

Loading