> On April 8, 2015, 9:57 p.m., Vinod Kone wrote: > > src/tests/fetcher_tests.cpp, lines 112-116 > > <https://reviews.apache.org/r/32975/diff/1/?file=920880#file920880line112> > > > > Add a note/todo here mentioning why you are setting these fields but > > not doing any asserts/expects on them. > > Jim Klucar wrote: > With the type_utils.cpp change, the EXPECT_EQ on lines 140/141 should be > checking the chown field. Should I still add text regarding how we can't > check this capability without assuming the user running the test has > permission to chown to some other user?
Yes. Because the EXPECT on #140 just checks that the chown field is properly propagated to fetcher environment but not that fetcher actually skips chown if chown is false. If you write a FetcherTest (see examples later in the file) that actually runs a fetcher process, would you able to test the semantics? I'm thinking that you can create a non-existent user (random string) and set chown to false. Once fetcher process finishes, you can check that the owner of the fetched file hasn't changed to the user. Does that make sense? > On April 8, 2015, 9:57 p.m., Vinod Kone wrote: > > include/mesos/mesos.proto, line 208 > > <https://reviews.apache.org/r/32975/diff/1/?file=920878#file920878line208> > > > > please update type_utils.cpp to account for this field when comparing > > CommandInfos. > > Jim Klucar wrote: > done as part of this change to type utils, please make sure that it is backwards compatible (e.g., scheduler using old ExecutorInfo protobuf with no chown field and master using the new ExecutorInfo protobuf with chown field). afaict, it is ok, but it is worth for you to go through that thought exercise. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/32975/#review79433 ----------------------------------------------------------- On April 9, 2015, 4:40 p.m., Jim Klucar wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/32975/ > ----------------------------------------------------------- > > (Updated April 9, 2015, 4:40 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 > >
