----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32975/#review79898 -----------------------------------------------------------
src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/32975/#comment129485> s/archive/archived/ ? src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/32975/#comment129477> kill extraneous space. s/tar gzip/tar gzip/ src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/32975/#comment129486> I know you just followed the example of other tests in the file, but this test (and other tests in this file) should really ensure that any temp files are properly cleaned up in even if the test fails. Current if the test fails for whatever reason, the cleanup on #756 won't be executed leaking this file. You could use 'environment->mkdtemp()' instead of 'os::mktemp()' to get the cleanup for free in the short term. s/os::mktemp()/environment->mkdtemp()/ In the long term, TemporaryDirectoryTest fixture should really have a mkdtemp() method on it that deletes temporary directories on TearDown. If you want to do this and fix the tests in this file, please do that in another review. src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/32975/#comment129478> unused? src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/32975/#comment129476> s/chowntestuser/'chowntestuser'/ s/returned/returns/ src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/32975/#comment129482> s/cleanup/Cleanup/ s/file/file./ src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/32975/#comment129484> s/archive/archived/ ? src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/32975/#comment129483> s/tar gzip/tar gzip/ src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/32975/#comment129481> unused? src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/32975/#comment129479> s/despite/Despite/ s/chowntestuser/'chowntestuser'/ s/process/fetching/ src/tests/fetcher_tests.cpp <https://reviews.apache.org/r/32975/#comment129480> s/cleanup/Cleanup/ s/file/file./ - Vinod Kone On April 13, 2015, 4:55 p.m., Jim Klucar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32975/ > ----------------------------------------------------------- > > (Updated April 13, 2015, 4:55 p.m.) > > > Review request for mesos. > > > Bugs: MESOS-1790 > https://issues.apache.org/jira/browse/MESOS-1790 > > > Repository: mesos > > > Description > ------- > > Added chown to CommandInfo.URI protocol buffer as an optional > boolean that defaults to true, the current chown behavior. > > The fetcher was updated to skip the os::chown operation if the chown > boolean is set to false. > > No documentation was updated. > > > Diffs > ----- > > include/mesos/mesos.proto 3c592d5ab3092ecbeddfaff95e0c1addc3ac58f8 > src/common/type_utils.cpp e92f6f36de0955784619029a016667b46bbe221b > src/launcher/fetcher.cpp 796526f59c25898ef6db2b828b0e2bb7b172ba25 > src/tests/fetcher_tests.cpp 4549e6a631e2c17cec3766efaa556593eeac9a1e > > Diff: https://reviews.apache.org/r/32975/diff/ > > > Testing > ------- > > Unit testing this functionality is difficult because it would require that > the user running the test to have permission to chown a file to someone other > than themselves. I didn't want to add that as a requirement to build. I added > the new field to the existing test cases just to see that they populate. > > > Thanks, > > Jim Klucar > >
