> On Oct. 28, 2014, 11:49 p.m., Kapil Arya wrote: > > We added a new module kind but didn't add any new tests. Wouldn't it be > > better to move the authenticator example to the patch where we add the test > > case or the other way around?
Yes, that split is a bit "random" in some cases (there are more). I would still like to leave it as is to allow for smaller chunks in the reviews and to minimize repatching certain areas over multiple commits. All cram-md5 authenticator patches have to be committed or none as noted above. > On Oct. 28, 2014, 11:49 p.m., Kapil Arya wrote: > > src/authentication/authenticator.hpp, line 24 > > <https://reviews.apache.org/r/26857/diff/5/?file=735280#file735280line24> > > > > Shouldn't we also explicitly include <process/pid.h> (although it is > > being included indirectly by process/future.hpp). Yes, well spotted. > On Oct. 28, 2014, 11:49 p.m., Kapil Arya wrote: > > src/authentication/authenticator.hpp, lines 19-20 > > <https://reviews.apache.org/r/26857/diff/5/?file=735280#file735280line19> > > > > Shouldn't this be __AUTHENTICATION_AUTHENTICATOR_HPP__ I would like to leave it as is, following the example of the isolator. I think our current rule does not use the folder name unless we need to disambiguate. > On Oct. 28, 2014, 11:49 p.m., Kapil Arya wrote: > > src/examples/test_authenticator_module.cpp, lines 22-26 > > <https://reviews.apache.org/r/26857/diff/5/?file=735282#file735282line22> > > > > The file module/authenticator.hpp is not part of this patch. Shouldn't > > it be removed from https://reviews.apache.org/r/26859/diff/# and added > > here? > > > > Also, alphabetize please. Yes, it slipped into the wrong RR. - Till ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26857/#review58909 ----------------------------------------------------------- On Oct. 29, 2014, 3:03 p.m., Till Toenshoff wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26857/ > ----------------------------------------------------------- > > (Updated Oct. 29, 2014, 3:03 p.m.) > > > Review request for mesos, Adam B, Benjamin Hindman, and Vinod Kone. > > > Bugs: MESOS-1889 > https://issues.apache.org/jira/browse/MESOS-1889 > > > Repository: mesos-git > > > Description > ------- > > Introducing the CRAM-MD5 SASL authenticator module based on the former > sasl/authenticator. > > The former sasl/authenticator.hpp still remains as part of libmesos but now > is located at authentication/cram_md5. > > > Diffs > ----- > > src/Makefile.am f177d87 > src/authentication/authenticator.hpp PRE-CREATION > src/authentication/cram_md5/authenticator.hpp PRE-CREATION > src/examples/test_authenticator_module.cpp PRE-CREATION > src/module/authenticator.hpp PRE-CREATION > src/module/manager.cpp 7a6c884 > > Diff: https://reviews.apache.org/r/26857/diff/ > > > Testing > ------- > > make check > > NOTE all four CRAM-MD5 authenticator module related RRs need to get applied > before running make check. > > > Thanks, > > Till Toenshoff > >
