-----------------------------------------------------------
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
> 
>

Reply via email to