> On Aug. 31, 2015, 5:50 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/provisioners/backends/copy.cpp, line 19 > > <https://reviews.apache.org/r/37921/diff/1/?file=1059333#file1059333line19> > > > > Move this below. > > > > ``` > > #include "common/status_utils.hpp" > > > > #include "slave/containerizer/provisioners/backends/copy.hpp" > > ```
I think our newest style guide points that we include the cpp's main header file on the top, but I'm fine moving this down. > On Aug. 31, 2015, 5:50 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/provisioners/backends/copy.cpp, lines 127-132 > > <https://reviews.apache.org/r/37921/diff/1/?file=1059333#file1059333line127> > > > > Using shell expansion here has others issues: > > > > 1) Spaces (or other $IFS) in the paths > > 2) Not sure if filename expansions works the same way across shells. > > > > GNU `cp` has a `--no-target-directory` flag that's handy in this > > situation but Mac `cp` doesn't have it. > > > > So I think we should probably just rely on a precondition check: return > > Failure if os::exists(rootfs) already exists and remove the mkdir in > > `provision()`. This allows us to just use the *argv* form of `subprocess` > > with `{"cp", "-a", layer, rootfs}`. > > > > AppcProvisioner already expects the Backend to create the `rootfs` and > > DockerProvisioner can do the same. > > > > We can emphaize this by adding a note to `Backend::provision()` > > documentation. > > > > Thoughts? Ok as long we can make sure provisioners output the same layout I definitely prefer just doing a straight copy! I'll make sure the basename of all layers are the same. - Timothy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/37921/#review97009 ----------------------------------------------------------- On Aug. 29, 2015, 7:59 a.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/37921/ > ----------------------------------------------------------- > > (Updated Aug. 29, 2015, 7:59 a.m.) > > > Review request for mesos, Jie Yu and Jiang Yan Xu. > > > Repository: mesos > > > Description > ------- > > Add Copy backend for provisioners. > > > Diffs > ----- > > src/Makefile.am 7b620ff66856b3f0adac121b3297d55ed71a3d99 > src/slave/containerizer/provisioners/backend.cpp > 2f7c335f62fdeb27526ab9a38a07c097422ae92b > src/slave/containerizer/provisioners/backends/copy.hpp PRE-CREATION > src/slave/containerizer/provisioners/backends/copy.cpp PRE-CREATION > src/tests/containerizer/provisioner_backend_tests.cpp > d321850613223a2357ca1646a9d988d05171772c > > Diff: https://reviews.apache.org/r/37921/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
