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 60439: Added scripts to automate website publishing.
Date Tue, 04 Jul 2017 12:02:05 GMT

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



This is a great start, but I would prefer if we wouldn't directly mount an existing source
directory into the container and bootstrap, configure and build in it (this is also broken
for e.g., in-source builds).

What about instead modify mounting the Mesos source tree into the container, then inside the
container cloning from it, and building in the clean checkout? This woudl not only minimize
the risk of destructive side-effects outside the container, but also e.g., make sure that
we do not by accident pick up uncommitted changes. This is the approach we also followed in
`support/mesos-tidy`.

I left some first comments, but will look again once the handling of the source tree is under
control.


support/jenkins/websitebot.sh
Lines 24 (patched)
<https://reviews.apache.org/r/60439/#comment254355>

    Let's quote,
    
        : "${WORKSPACE:?"Environment variable 'WORKSPACE' must be set"}"



support/jenkins/websitebot.sh
Lines 26-27 (patched)
<https://reviews.apache.org/r/60439/#comment254358>

    Let's quote,
    
        export MESOS_DIR="$WORKSPACE/mesos"
        export MESOS_SITE_DIR="$WORKSPACE/mesos-site"



support/jenkins/websitebot.sh
Lines 29 (patched)
<https://reviews.apache.org/r/60439/#comment254356>

    Let's quote,
    
        pushd "$MESOS_DIR"



support/jenkins/websitebot.sh
Lines 31 (patched)
<https://reviews.apache.org/r/60439/#comment254354>

    If the source tree contains uncommitted changes, the output will not reproducibly correspond
to this SHA1.
    
    I'd suggest to instead clone the source tree instead.



support/jenkins/websitebot.sh
Lines 38 (patched)
<https://reviews.apache.org/r/60439/#comment254359>

    Let's quote,
    
        pushd "$MESOS_SITE_DIR"



support/mesos-website.sh
Lines 25 (patched)
<https://reviews.apache.org/r/60439/#comment253995>

    Let's put this into quotes,
    
        : "${MESOS_SITE_DIR:?"Environment variable 'MESOS_SITE_DIR' must be set"}"



support/mesos-website.sh
Lines 27 (patched)
<https://reviews.apache.org/r/60439/#comment253993>

    Let's quote here,
    
        pushd "$MESOS_DIR"



support/mesos-website.sh
Lines 33 (patched)
<https://reviews.apache.org/r/60439/#comment253992>

    Not crucial here right now, but let's put this string into single quotes, see https://github.com/koalaman/shellcheck/wiki/SC2064.
    
    Also please consider working on getting this image published in the Mesos dockerhub org
so we can pull from there instead of having the rebuild the same image over and over again.
See e.g., `support/mesos-tidy.sh` how we already do this for other images.



support/mesos-website.sh
Lines 43 (patched)
<https://reviews.apache.org/r/60439/#comment253994>

    Let's be consistent with other command substitutions in this file, and also add quotes,
    
        docker run \
          --rm \
          -e LOCAL_USER_ID="$(id -u "$USER")" \
          -v "$MESOS_DIR":/SRC \
          -v "$MESOS_SITE_DIR"/content:/mesos/site/publish \
          $TAG



support/mesos-website/Dockerfile
Lines 1-11 (patched)
<https://reviews.apache.org/r/60439/#comment254000>

    We now have one Docker image to run tests in CI (via `docker_build.sh`), one to run clang-tidy
checks (`mesos-tidy/Dockerfile`), and are adding another one  here to build the website.
    
    We should try to reduce the number of these images or remove duplication, e.g., this image
could be in principle implemented as a small addon on top of `mesos/mesos-tidy`
    
        FROM mesos/mesos-tidy:latest
        LABEL Description="This image is used for generating Mesos web site."
        
        # Install ruby and doxygen tools ...



support/mesos-website/build.sh
Lines 35 (patched)
<https://reviews.apache.org/r/60439/#comment253955>

    Could we make this consistent with other CI scripts?
    
    In `docker_build.sh` we use `make -j6` w/o e.g., hitting a memory limit, in `mesos-tidy/entrypoint.sh`
where we do not build the most expensive parts of Mesos we even pick `make -j $(nproc)`.


- Benjamin Bannier


On June 29, 2017, 11:59 p.m., Vinod Kone wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60439/
> -----------------------------------------------------------
> 
> (Updated June 29, 2017, 11:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and haosdent huang.
> 
> 
> Bugs: MESOS-7625
>     https://issues.apache.org/jira/browse/MESOS-7625
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> These scripts are expected to be run by ASF CI on any commit to
> the mesos repo. The directory layout is similar to tidybot CI job.
> 
> 
> Diffs
> -----
> 
>   support/jenkins/websitebot.sh PRE-CREATION 
>   support/mesos-website.sh PRE-CREATION 
>   support/mesos-website/Dockerfile PRE-CREATION 
>   support/mesos-website/build.sh PRE-CREATION 
>   support/mesos-website/entrypoint.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60439/diff/1/
> 
> 
> Testing
> -------
> 
> Tested by running a CI job pointing to a branch containing this patch.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>


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