mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jie Yu <yujie....@gmail.com>
Subject Re: Review Request 47266: Implemented provisioner removing docker whitelist files.
Date Thu, 26 May 2016 22:07:26 GMT

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


Fix it, then Ship it!





src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 312)
<https://reviews.apache.org/r/47266/#comment200052>

    is _currently_ docker image specific.



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 324)
<https://reviews.apache.org/r/47266/#comment200100>

    No need for this check



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 330)
<https://reviews.apache.org/r/47266/#comment200048>

    Remove space before 'rootfs' and after 'dockerManifest'



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 333)
<https://reviews.apache.org/r/47266/#comment200056>

    Ditto. No spaces around `{` and `}`



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 337)
<https://reviews.apache.org/r/47266/#comment200070>

    Can you include the errno? os::strerror(errno)?



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 340)
<https://reviews.apache.org/r/47266/#comment200096>

    s/whitelist/whiteout/



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 345)
<https://reviews.apache.org/r/47266/#comment200091>

    Can you make `.wh.` a constant in spec.hpp? E.g. `WHITEOUT_PREFIX`



src/slave/containerizer/mesos/provisioner/provisioner.cpp (lines 348 - 349)
<https://reviews.apache.org/r/47266/#comment200097>

    Do you need this temp variable?



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 349)
<https://reviews.apache.org/r/47266/#comment200092>

    please use strlen(WHILEOUT_PREFIX) here.



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 370)
<https://reviews.apache.org/r/47266/#comment200098>

    Can you include the errno? Ditto above.



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 378)
<https://reviews.apache.org/r/47266/#comment200101>

    please use whiteout consistently



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 391)
<https://reviews.apache.org/r/47266/#comment200102>

    Ditto on extra spaces.


- Jie Yu


On May 13, 2016, 10:16 p.m., Gilbert Song wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47266/
> -----------------------------------------------------------
> 
> (Updated May 13, 2016, 10:16 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-5277
>     https://issues.apache.org/jira/browse/MESOS-5277
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Implemented provisioner removing docker whitelist files.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 5c96e9f6603d39889e6bc807874d35d0cb3556be

>   src/slave/containerizer/mesos/provisioner/provisioner.cpp dcbbbaf797d4467bfd0bb1ee91ee9ce843e7d546

> 
> Diff: https://reviews.apache.org/r/47266/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> Tested with docker image containing `.wh.` files.
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>


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