> On June 24, 2014, 9:58 a.m., Adam B wrote: > > src/credentials/credentials.hpp, line 34 > > <https://reviews.apache.org/r/22222/diff/3/?file=614273#file614273line34> > > > > To quote Vinod's comment on my original RR (r/20301): > > "with the name space change you can > > s/readCredentials/read/" > > since credentials::readCredentials() sounds redundant.
Thanks for the comments, I was trying to differentiate read for one credential from multiple. I kept just read for multiple credentials read. > On June 24, 2014, 9:58 a.m., Adam B wrote: > > src/credentials/credentials.hpp, lines 58-63 > > <https://reviews.apache.org/r/22222/diff/3/?file=614273#file614273line58> > > > > So, if the credentials file is valid JSON, but not a valid Credentials > > message, you will fall through and treat it like a plain-text file? Yes and in that case an error will also be returned since it's not a valid text file either. > On June 24, 2014, 9:58 a.m., Adam B wrote: > > src/credentials/credentials.hpp, line 82 > > <https://reviews.apache.org/r/22222/diff/3/?file=614273#file614273line82> > > > > Why create a separate readCredential() method as well? That's what I > > had in my original r/18381 before Vinod suggested I "Just use > > readCredentials and let the caller check the returned vector is of size 1." I think it is cleaner to make a read a single credential function, we gain taking away that size check. > On June 24, 2014, 9:58 a.m., Adam B wrote: > > src/tests/mesos.cpp, line 146 > > <https://reviews.apache.org/r/22222/diff/3/?file=614282#file614282line146> > > > > Why does this need to be turned JSON? Ben make a good comment about making things easier if instead of a string miming the json format we just built it from the original message. - Isabel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/22222/#review46508 ----------------------------------------------------------- On June 26, 2014, 5:50 p.m., Isabel Jimenez wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/22222/ > ----------------------------------------------------------- > > (Updated June 26, 2014, 5:50 p.m.) > > > Review request for mesos, Benjamin Hindman, Dominic Hamon, and Till Toenshoff. > > > Bugs: MESOS-1391 > https://issues.apache.org/jira/browse/MESOS-1391 > > > Repository: mesos-git > > > Description > ------- > > After Ben's comments, regroup of all types of authentication in one common > definition, this is a draft for this Issue, here are some questions: > > - Do I replace credentials flag completely or if this slight duplication will > suffice for now ? > - If I don't replace it, for now as it is in this patch, the flag is useless, > I should give it to the sasl/authenticator but I would really appreciate some > comments for that part. > > > Diffs > ----- > > include/mesos/mesos.proto 2f6be05 > src/Makefile.am b1b7d2d > src/credentials/credentials.hpp 98b9088 > src/master/flags.hpp 47bb0dc > src/master/master.hpp b56e9f4 > src/master/master.cpp dcf28ad > src/sasl/authenticator.hpp 365db5f > src/slave/flags.hpp 3b8ba08 > src/slave/slave.cpp ed3483f > src/tests/credentials_tests.cpp PRE-CREATION > src/tests/mesos.cpp 1037420 > > Diff: https://reviews.apache.org/r/22222/diff/ > > > Testing > ------- > > > Thanks, > > Isabel Jimenez > >
