Pass env vars to docker build

[drewrobb]

@erikdw fixes #90, although a little bit janky because of repeating variable names 3 times...

Also, perhaps the base image should match MESOS_RELEASE, but that is a separate issue.

[dsKarthick]

@drewrobb Fyi, here is an alternative way of passing environment variable moby/moby#2637 (comment) . That said, I prefer your approach.

[drewrobb]

@dsKarthick cool trick!

[erikdw]

@drewrobb & @dsKarthick : it may actually be better to use that env file trick to integrate with docker hub. I'm not sure of how the docker hub mechanism works, and it's unclear to me how we'll set these parameters when using docker hub. Maybe they're unneeded.

I'm a bit worried that the proposed change here in #91 might not work if we don't have all those variables specified. ... Oh! You can specify a default value:

• https://docs.docker.com/engine/reference/builder/#arg

ARG <name>[=<default value>]

That should suffice until we can gain a better understanding of the docker hub integration. Maybe @brndnmtthws can speak to that?

[erikdw]

@DarinJ just created an issue (#92) that is directly related to this.

[dsKarthick]

@drewrobb Lets set the default values for these variables like @erikdw mentioned?

[drewrobb]

There are default values set in bin/build-release.sh (since the environment variables are always set to something, so I think it would just be making those values redundantly specified unless I'm missing something?

[dsKarthick]

@drewrobb What happens when someone tries to do docker build -t <tag_name> directly without using the build script?

[drewrobb]

Ah good point, I was thinking people would only invoke build via the build-release.sh script.

[dsKarthick]

@erikdw Good point regarding the default values.

[dsKarthick]

@drewrobb @erikdw On a second thought, I feel a combination of sourcing technique and @drewrobb's build-arg technique would be better for couple of reasons
* It allows us to specify as many environment variables as we like in the env file without making $cmd in build-release.sh look big. (Related Issue: #92)
* It helps keep the Dockerfile small as well.

By combination, I mean the environment variables specified in the env script will have precedence over those we pass with build-arg trick.

Docker seems to support conditionals for RUN commands as discussed in this stackoverflow post. Not sure if it does for ARG. Maybe @drewrobb could shed some light on that.

[DarinJ]

Would be cool here to embed the storm/mesos versions into the tag (possibly in the version). Then it'd be trivial to release a few different images on dockerhub. Related to #18, #83.

[dsKarthick]

@DarinJ We should also make mesos/storm configurable. Something like <user_name>/<image_name> where <image_name> is something that is in accordance with #83

For now, lets keep that change separate so we don't block this PR on #83 ?

[DarinJ]

Might consider using a stock Ubuntu image and installing the correct version of mesos as defined by MESOS_VERSION. Debs available here: https://open.mesosphere.com/downloads/mesos/

[brndnmtthws]

👍 for parameterizing the version. I'm not sure how to make it integrate well with the automated builds on Docker hub, however.

[erikdw]

Ok, so the goals seem to be:

1. Allow the bin/build-release.sh dockerImage invocation to accept configurable storm & mesos versions, and properly build the docker image with the chosen version of mesos.
2. Allow the integration between GitHub & Docker Hub to continue working.

I think we can accomplish both of those with a combination of our ideas from this PR's conversation:

1. Populate our Dockerfile with ARGS for the mesos & storm versions, giving them default values which will be used when we don't specify the --build-arg parameters (i.e., when Docker Hub does the build upon a commit to the mesos/storm repo).
2. Change our Dockerfile to be based on a vanilla ubuntu image that we install mesos on top of, to allow for the mesos version to be configurable. i.e., DarinJ's idea proposed above.
3. As an aside, something else we need to do is update the mesos dependency in the pom.xml to match the chosen mesos version.

Anyone see any issues with the proposal?

....

Oh, I see an issue with @DarinJ 's idea from my naive perspective: it's not clear how we can get the actual mesos deb package name/URL given the mesos version. There are extra bits in the package name and URL. i.e.,

• http://downloads.mesosphere.io/master/ubuntu/15.04/mesos_0.27.0-0.2.190.ubuntu1504_amd64.deb
• http://downloads.mesosphere.io/master/ubuntu/15.04/mesos_0.26.0-0.2.145.ubuntu1504_amd64.deb
• http://downloads.mesosphere.io/master/ubuntu/15.04/mesos_0.25.0-0.2.70.ubuntu1504_amd64.deb

I don't know what -0.2.70, -0.2.145, -0.2.190 mean nor how to predict them.

[dsKarthick]

@erikdw Regarding point 3, what do you think about passing the mesos and storm versions as maven parameters? Currently we grep to figure out mesos and storm versions. Instead we could pass the version we intend to build like mvn install "-Dstorm.version=0.9.6 -Dmesos.version=0.26.0?

[dsKarthick]

@erikdw Also regarding point 1, it would be nice to source the environment variables defined in a separate file. That way we could define global env varaiables in both here, here and wherever else?

[erikdw]

This feels like it is snowballing a bit. I still think there are fundamental things we need to iron out to fully satisfy the goals as specified above, but it's going to take time to figure out the "right" way to handle the weird suffix thing for the mesos packages. I've asked a question about that on the mesos dev list.

So for now in this PR, how about we just do the "ARGS with defaults" thing? (err... of course with the other changes that @drewrobb has made in bin/build-release.sh... by "just" I meant not worrying about the inconsistent pom.xml mesos dependency and inconsistent Docker base image).

[dsKarthick]

@erikdw Yes. I agree. Right solutions are almost always inversely propotional to quick solutions.

[drewrobb]

I like this plan. I updated my commit to include defaults (defaults that are consistent with the existing dockerfile, but not pom.xml)

[erikdw]

@drewrobb : that looks good to me, let me test it later tonight or tomorrow morning, I expect I'll just merge it.

[erikdw]

hey @drewrobb : can you please make 1 more change? pom.xml has the mesos versions as 0.24.1, can you change them to 0.25.0 so the defaults are all the same? Thanks!

[dsKarthick]

@drewrobb @erikdw why not mesos 0.27.0 ?

[erikdw]

@dsKarthick : I'm fine with 0.27.0 too, let's just be consistent in the various files where it's sprinkled around for now. I'll file an issue for follow-up work that came up in this PR.

@drewrobb : if you update the FROM base image, please note that you'll need to change it to mesosphere/mesos:0.27.0-0.2.190.ubuntu1404.

[drewrobb]

Updated to mesos 0.27.0. I tested default build locally and it worked for me.

[erikdw]

Thanks @drewrobb and others that contributed to the discussion. Merging!

[erikdw]

FYI, I've filed 2 issues for a the unresolved topics that came up in this review:

• improve docker image tag name #95: improve the docker image tag name
• improve handling of mesos version in the build #96: improve handling of mesos version in the build