-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/65464/#review196694
-----------------------------------------------------------


Fix it, then Ship it!




Thanks for these patches mpark. I left some comments, mostly around reducing 
the size of the images the CI would need to download.

Before rolling this out we should make sure we have some automation in tool 
keeping the images up to date from source. Like discussed offline we might be 
able to leverage dockerhub's infra for that.


support/mesos-build.sh
Lines 33 (patched)
<https://reviews.apache.org/r/65464/#comment276487>

    Nit: I think we don't use markdown in logging output, maybe use single 
quotes around `mesos-build` here.



support/mesos-build/centos-7.dockerfile
Lines 18 (patched)
<https://reviews.apache.org/r/65464/#comment276494>

    Just as a heads-up, while We also use this in the `mesos-tidy` image, the 
syntax is actually deprecated upstream by now, 
https://docs.docker.com/engine/reference/builder/#maintainer-deprecated.



support/mesos-build/centos-7.dockerfile
Lines 21 (patched)
<https://reviews.apache.org/r/65464/#comment276477>

    This seems even worse than the unpinned dependencies we have below since we 
might pick up random, disruptive upstream changes.
    
    Can we just get rid of this? It seems to not be required.



support/mesos-build/centos-7.dockerfile
Lines 22-24 (patched)
<https://reviews.apache.org/r/65464/#comment276478>

    Let's use a single layer for this, i.e., just chain the commands with `&&`.



support/mesos-build/centos-7.dockerfile
Lines 42 (patched)
<https://reviews.apache.org/r/65464/#comment276479>

    Let's clean up local caches in this layer, i.e.,
    
        yum clean all && \
        rm -rf /var/cache/yum



support/mesos-build/centos-7.dockerfile
Lines 47 (patched)
<https://reviews.apache.org/r/65464/#comment276480>

    Let's remove the downloaded script in this layer, i.e., add
    
        rm -f /tmp/install-cmake.sh



support/mesos-build/centos-7.dockerfile
Lines 52 (patched)
<https://reviews.apache.org/r/65464/#comment276476>

    Let's use `COPY` here, 
https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#add-or-copy.



support/mesos-build/entrypoint.sh
Lines 53 (patched)
<https://reviews.apache.org/r/65464/#comment276485>

    Quote this.



support/mesos-build/entrypoint.sh
Lines 77 (patched)
<https://reviews.apache.org/r/65464/#comment276486>

    Quote this.



support/mesos-build/ubuntu-16.04.dockerfile
Lines 18 (patched)
<https://reviews.apache.org/r/65464/#comment276495>

    Just as a heads-up, while We also use this in the `mesos-tidy` image, the 
syntax is actually deprecated upstream by now, 
https://docs.docker.com/engine/reference/builder/#maintainer-deprecated.



support/mesos-build/ubuntu-16.04.dockerfile
Lines 42 (patched)
<https://reviews.apache.org/r/65464/#comment276484>

    We don't seem to need this.



support/mesos-build/ubuntu-16.04.dockerfile
Lines 44 (patched)
<https://reviews.apache.org/r/65464/#comment276481>

    Let's also get rid of the dowloaded lists, i.e., add
    
        rm -rf /var/lib/apt/lists



support/mesos-build/ubuntu-16.04.dockerfile
Lines 49 (patched)
<https://reviews.apache.org/r/65464/#comment276482>

    Let's remove the downloaded script in this layer, i.e., add
    
        rm -f /tmp/install-cmake.sh



support/mesos-build/ubuntu-16.04.dockerfile
Lines 55 (patched)
<https://reviews.apache.org/r/65464/#comment276483>

    Let's use `COPY` here, 
https://docs.docker.com/develop/develop-images/dockerfile_best-practices/#add-or-copy.


- Benjamin Bannier


On Feb. 1, 2018, 8:20 p.m., Michael Park wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65464/
> -----------------------------------------------------------
> 
> (Updated Feb. 1, 2018, 8:20 p.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Introduced `mesos-build`, along with pre-built docker images.
> 
> 
> Diffs
> -----
> 
>   support/jenkins/buildbot.sh 7f78509699b036df025c314fe913158f54402014 
>   support/mesos-build.sh PRE-CREATION 
>   support/mesos-build/centos-7.dockerfile PRE-CREATION 
>   support/mesos-build/entrypoint.sh PRE-CREATION 
>   support/mesos-build/ubuntu-16.04.dockerfile PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/65464/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Reply via email to