----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44319/#review122543 -----------------------------------------------------------
src/authorizer/authorizer.cpp (line 44) <https://reviews.apache.org/r/44319/#comment184603> s/Using/Creating/ src/authorizer/authorizer.cpp (line 62) <https://reviews.apache.org/r/44319/#comment184600> new line. src/authorizer/authorizer.cpp (line 76) <https://reviews.apache.org/r/44319/#comment184601> don't we need to pass params here? src/examples/test_authorizer_module.cpp (lines 41 - 61) <https://reviews.apache.org/r/44319/#comment184617> why can't you use Authorizer::create() here instead of repeating this flags extracting/parsing logic? src/examples/test_authorizer_module.cpp (line 74) <https://reviews.apache.org/r/44319/#comment184613> s/createAuthorizer/createLocalAuthorizer/ ? looking at how CRAMMD5 test module was called. src/local/local.cpp (lines 225 - 233) <https://reviews.apache.org/r/44319/#comment184632> I'm a little confused. Does this mean we allow specifying non-local authorizer as "flags.authorizers" but only allow "acls" parameter to it? What if the module writer want to send extra parameters to their module? src/master/main.cpp (lines 363 - 369) <https://reviews.apache.org/r/44319/#comment184639> I see the repetition now. How about we create an overload Authorizer::create(const ACLs& acls) that calls Authorizer::create(const string& name, const Parameters& parameters). Add a TODO saying that this overload should be killed once ACLs can be input as a module param instead of top level "--acls" flag. src/tests/authorization_tests.cpp (lines 56 - 75) <https://reviews.apache.org/r/44319/#comment184644> why not use Authorizer::create() instead of the repetition here? src/tests/authorization_tests.cpp (lines 97 - 100) <https://reviews.apache.org/r/44319/#comment184645> why not make this a test fixture method? ``` template <typename T> class AuthorizationTest : public MesosTest { protected: Parameters convert(const ACLs& acls) { Parameters parameters; auto *parameter = parameters.add_parameter(); parameter->set_key("acls"); parameter->set_value(string(jsonify(JSON::Protobuf(acls)))); return parameters; } }; ``` - Vinod Kone On March 8, 2016, 4:43 p.m., Alexander Rojas wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44319/ > ----------------------------------------------------------- > > (Updated March 8, 2016, 4:43 p.m.) > > > Review request for mesos, Adam B, Joerg Schad, and Vinod Kone. > > > Bugs: MESOS-2950 > https://issues.apache.org/jira/browse/MESOS-2950 > > > Repository: mesos > > > Description > ------- > > Removed initialize method from the authorizer interface. > > > Diffs > ----- > > include/mesos/authorizer/authorizer.hpp > ec6c9928c55c3096c7de634f900419abbdd00886 > src/authorizer/authorizer.cpp 54278b022118c40d3b976794fd472ce8d8b6a5e2 > src/authorizer/local/authorizer.hpp > 96baf77709cf721caf46b6c2c096a843c1b5d9c0 > src/authorizer/local/authorizer.cpp > 4e5c2c2869823ec957735cebfc80dc850d40f9eb > src/examples/test_authorizer_module.cpp > 95d77fbff0cdfdb360a8597fbba28404b59d0042 > src/local/local.cpp 359fc54d7c4081f536a8de8b1dfcde413d75c9a9 > src/master/main.cpp 4263110c9b889984ef74eb94fed629958f2abd79 > src/tests/authorization_tests.cpp 2b2297036550412a955ff479f6ec9d7dad8cb0e3 > src/tests/cluster.cpp 084fb1ce37a315c561c4587c4761c870f54c8625 > > Diff: https://reviews.apache.org/r/44319/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Alexander Rojas > >
