mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 65464: WIP: Introduced `mesos-build`, along with pre-built docker images.
Date Fri, 02 Feb 2018 10:00:00 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message