Create a Dockerfile.base and ensure it is included in build tarballs.

Create a Dockerfile.base and ensure it is included in build tarballs.

This is an intermediate step to aid in validating image creation script
updates before adding dependencies in derived Dockerfiles.

BUG=616945
Committed: https://crrev.com/bd26b4aa094cb01421a1e7e00deb504abb9a5d6e
Cr-Commit-Position: refs/heads/master@{#400319}

[Jess, 2016-06-09 00:41:41]

Description was changed from

==========
Create a Dockerfile.base and ensure it is included in build tarballs.

This is an intermediate step to aid in validating image creation script
updates before adding dependencies in derived Dockerfiles.

BUG=616945
==========

to

==========
Create a Dockerfile.base and ensure it is included in build tarballs.

This is an intermediate step to aid in validating image creation script
updates before adding dependencies in derived Dockerfiles.

BUG=616945
==========

[Jess, 2016-06-09 00:41:41]

jessicag@chromium.org changed reviewers:
+ kmarshall@chromium.org, maniscalco@chromium.org, sriramsr@chromium.org

[Jess, 2016-06-10 00:07:40]

I'd like to use this to verify downstream work in supporting Dockerfile.base.

[maniscalco, 2016-06-10 22:27:30]

LGTM % two comments...

https://codereview.chromium.org/2043403004/diff/1/blimp/tools/Dockerfile.base
File blimp/tools/Dockerfile.base (right):

https://codereview.chromium.org/2043403004/diff/1/blimp/tools/Dockerfile.base...
blimp/tools/Dockerfile.base:1: # Blimp base Dockerfile. Should be used by all
Dockerfiles used by Blimp to
Does this file replace //blimp/Dockerfile?

Or is //blimp/Dockerfile the "derived" file we'll use for tests?

If it's to be used for testing, consider moving it under a dir that makes that
more obvious (//blimp/engine/testing/Dockerfile?).  Can be done in follow up CL
if you prefer.

If Dockerfile.base replaces //blimp/Dockerfile, how about removing
//blimp/Dockerfile in this CL.

https://codereview.chromium.org/2043403004/diff/1/blimp/tools/Dockerfile.base...
blimp/tools/Dockerfile.base:12: RUN apt-get update && apt-get install -yq
stunnel4 wget
Note, earlier today I landed a change to the file from which this was branched
that you'll want to pick up.  I believe we'll want all three of the apt-get
lines in (and their updated comments) from:

https://cs.chromium.org/chromium/src/blimp/engine/Dockerfile?q=Dockerfile&sq=...

added here.

[Jess, 2016-06-11 00:45:12]

https://codereview.chromium.org/2043403004/diff/1/blimp/tools/Dockerfile.base
File blimp/tools/Dockerfile.base (right):

https://codereview.chromium.org/2043403004/diff/1/blimp/tools/Dockerfile.base...
blimp/tools/Dockerfile.base:1: # Blimp base Dockerfile. Should be used by all
Dockerfiles used by Blimp to
On 2016/06/10 22:27:29, maniscalco wrote:
> Does this file replace //blimp/Dockerfile?
> 
> Or is //blimp/Dockerfile the "derived" file we'll use for tests?
> 
> If it's to be used for testing, consider moving it under a dir that makes that
> more obvious (//blimp/engine/testing/Dockerfile?).  Can be done in follow up
CL
> if you prefer.
> 
> If Dockerfile.base replaces //blimp/Dockerfile, how about removing
> //blimp/Dockerfile in this CL.

It was intended as the latter.

I placed the test Dockerfile under //blimp, because I was concerned putting it
under //blimp/engine/testing would imply it was only for Blimp unittests, but
thinking about it I think the confusion you bring up is a more immediate
concern.

https://codereview.chromium.org/2043403004/diff/1/blimp/tools/Dockerfile.base...
blimp/tools/Dockerfile.base:12: RUN apt-get update && apt-get install -yq
stunnel4 wget
On 2016/06/10 22:27:29, maniscalco wrote:
> Note, earlier today I landed a change to the file from which this was branched
> that you'll want to pick up.  I believe we'll want all three of the apt-get
> lines in (and their updated comments) from:
> 
>
https://cs.chromium.org/chromium/src/blimp/engine/Dockerfile?q=Dockerfile&sq=...
> 
> added here.

Updated.

Proof of value of having one place for these commands. :)

[Jess, 2016-06-16 17:22:32]

Ping

[Sriram, 2016-06-16 18:04:08]

Looks good except for the following nits:

https://codereview.chromium.org/2043403004/diff/20001/blimp/engine/BUILD.gn
File blimp/engine/BUILD.gn (right):

https://codereview.chromium.org/2043403004/diff/20001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:535: action("blimp_engine_bundle") {
Should blimp_engine_env_tests_bundle target be updated to include the base file
as well?

https://codereview.chromium.org/2043403004/diff/20001/blimp/engine/testing/Do...
File blimp/engine/testing/Dockerfile (left):

https://codereview.chromium.org/2043403004/diff/20001/blimp/engine/testing/Do...
blimp/engine/testing/Dockerfile:1: # Testing dockerfile stub
Nit: Why is testing under engine instead of blimp/testing/Dockerfile?

https://codereview.chromium.org/2043403004/diff/20001/blimp/tools/Dockerfile....
File blimp/tools/Dockerfile.base (left):

https://codereview.chromium.org/2043403004/diff/20001/blimp/tools/Dockerfile....
blimp/tools/Dockerfile.base:17: 
Do we need a todo in blimp/engine/Dockerfile to use Dockerfile.base at later
point?

[Sriram, 2016-06-16 18:05:15]

LGTM except for the two nits:

https://codereview.chromium.org/2043403004/diff/20001/blimp/engine/BUILD.gn
File blimp/engine/BUILD.gn (right):

https://codereview.chromium.org/2043403004/diff/20001/blimp/engine/BUILD.gn#n...
blimp/engine/BUILD.gn:535: action("blimp_engine_bundle") {
On 2016/06/16 18:04:08, Sriram wrote:
> Should blimp_engine_env_tests_bundle target be updated to include the base
file
> as well?

grr - coffee time! Ignore this.

[Sriram, 2016-06-16 18:05:17]

LGTM except for the two nits:

[Sriram, 2016-06-16 18:05:20]

LGTM except for the two nits:

[Jess, 2016-06-16 18:18:12]

https://codereview.chromium.org/2043403004/diff/20001/blimp/engine/testing/Do...
File blimp/engine/testing/Dockerfile (left):

https://codereview.chromium.org/2043403004/diff/20001/blimp/engine/testing/Do...
blimp/engine/testing/Dockerfile:1: # Testing dockerfile stub
On 2016/06/16 18:04:08, Sriram wrote:
> Nit: Why is testing under engine instead of blimp/testing/Dockerfile?

I thought at little about that option when Nick suggested moving this file. The
environmental testing only makes sense in the context of the engine so I went
with the more specific location to start.

If you'd like me to drop it down a level, I'd prefer reusing blimp/test instead
of having both it and blimp/testing, unless there is some Chrome convention
around those directories.

https://codereview.chromium.org/2043403004/diff/20001/blimp/tools/Dockerfile....
File blimp/tools/Dockerfile.base (left):

https://codereview.chromium.org/2043403004/diff/20001/blimp/tools/Dockerfile....
blimp/tools/Dockerfile.base:17: 
On 2016/06/16 18:04:08, Sriram wrote:
> Do we need a todo in blimp/engine/Dockerfile to use Dockerfile.base at later
> point?

I didn't worry about it given the tracking bug http://crbug/616945 . Honestly
was expecting this change to take less than a day to commit and be getting that
follow up in by today. :)

[Sriram, 2016-06-16 18:53:26]

https://codereview.chromium.org/2043403004/diff/20001/blimp/engine/testing/Do...
File blimp/engine/testing/Dockerfile (left):

https://codereview.chromium.org/2043403004/diff/20001/blimp/engine/testing/Do...
blimp/engine/testing/Dockerfile:1: # Testing dockerfile stub
On 2016/06/16 18:18:11, Jess wrote:
> On 2016/06/16 18:04:08, Sriram wrote:
> > Nit: Why is testing under engine instead of blimp/testing/Dockerfile?
> 
> I thought at little about that option when Nick suggested moving this file.
The
> environmental testing only makes sense in the context of the engine so I went
> with the more specific location to start.
> 
> If you'd like me to drop it down a level, I'd prefer reusing blimp/test
instead
> of having both it and blimp/testing, unless there is some Chrome convention
> around those directories.

That logic does make sense and then it would make sense to move the test bundle
target to also be under engine instead of blimp (the current locaiton).
Something to move in follow-up CL?

[Kevin M, 2016-06-16 20:52:27]

The CQ bit was checked by kmarshall@chromium.org

[Kevin M, 2016-06-16 20:52:28]

lgtm

[Kevin M, 2016-06-16 20:52:28]

The patchset sent to the CQ was uploaded after l-g-t-m from
maniscalco@chromium.org
Link to the patchset: https://codereview.chromium.org/2043403004/#ps20001
(title: "Pull in new apt-get and move testing Dockerfile as suggested.")

[commit-bot: I haz the power, 2016-06-16 20:53:35]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2043403004/20001

[commit-bot: I haz the power, 2016-06-17 00:46:17]

Description was changed from

==========
Create a Dockerfile.base and ensure it is included in build tarballs.

This is an intermediate step to aid in validating image creation script
updates before adding dependencies in derived Dockerfiles.

BUG=616945
==========

to

==========
Create a Dockerfile.base and ensure it is included in build tarballs.

This is an intermediate step to aid in validating image creation script
updates before adding dependencies in derived Dockerfiles.

BUG=616945
==========

[commit-bot: I haz the power, 2016-06-17 00:46:19]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-06-17 00:48:16]

Description was changed from

==========
Create a Dockerfile.base and ensure it is included in build tarballs.

This is an intermediate step to aid in validating image creation script
updates before adding dependencies in derived Dockerfiles.

BUG=616945
==========

to

==========
Create a Dockerfile.base and ensure it is included in build tarballs.

This is an intermediate step to aid in validating image creation script
updates before adding dependencies in derived Dockerfiles.

BUG=616945

Committed: https://crrev.com/bd26b4aa094cb01421a1e7e00deb504abb9a5d6e
Cr-Commit-Position: refs/heads/master@{#400319}
==========

[commit-bot: I haz the power, 2016-06-17 00:48:17]

Patchset 2 (id:??) landed as
https://crrev.com/bd26b4aa094cb01421a1e7e00deb504abb9a5d6e
Cr-Commit-Position: refs/heads/master@{#400319}