> On July 31, 2018, 3:38 a.m., Chun-Hung Hsiao wrote: > > src/slave/http.cpp > > Lines 1821-1822 (patched) > > <https://reviews.apache.org/r/68104/diff/1/?file=2065137#file2065137line1821> > > > > The following can be fit into 80 characters: > > ``` > > [this, acceptType](const Owned<ObjectApprover>& approvers) -> > > Response { > > ``` > > Or for here it seems harmless to just use `[=]`. I'm fine with either > > though. > > Benjamin Bannier wrote: > That line seems to be exactly 81 characters long, so it looks like we do > need to split somewhere. I now manually adjusted this to not split the > capture list (the previous version was formated by our `clang-format`). > > I don't think that implicitly capturing pointers to not immediately > invoked lambdas is safe style, much less when capturing `this` like here. > I'll keep the explicit capture.
You're right about the line length. I must have made a mistake. It seems that it's more common to do the following indentation in our codebase: ``` [this, acceptType]( const Owned<ObjectApprovers>& approvers) -> Response { ``` > On July 31, 2018, 3:38 a.m., Chun-Hung Hsiao wrote: > > src/tests/api_tests.cpp > > Lines 7002-7004 (original), 7002-7021 (patched) > > <https://reviews.apache.org/r/68104/diff/1/?file=2065138#file2065138line7002> > > > > How about the following: > > ``` > > slave::Flags slaveFlags = CreateSlaveFlags(); > > slaveFlags.authenticate_http_readwrite = true; > > > > { > > // `DEFAULT_CREDENTIAL_2` is not allowed to view any resource > > provider. > > mesos::ACL::ViewResourceProvider* acl = > > slaveFlags.acls->add_view_resource_providers(); > > > > acl->mutable_principals()->add_values(DEFAULT_CREDENTIAL_2.principal()); > > acl->mutable_resource_providers()->set_type(mesos::ACL::Entity::NONE); > > { > > > > Try<Owned<cluster::Slave>> slave = StartSlave(&detector, slaveFlags); > > ``` > > Benjamin Bannier wrote: > Great suggestion, applied. > > I did not set `authenticate_http_readwrite` though since it is not > required. Are you sure about this? The test won't pass if I don't turn this flag on: ``` [ RUN ] ContentType/AgentAPITest.GetResourceProviders/0 ../src/tests/api_tests.cpp:7091: Failure Expected: 0 To be equal to: v1Response->get_resource_providers().resource_providers_size() Which is: 1 [ FAILED ] ContentType/AgentAPITest.GetResourceProviders/0, where GetParam() = application/x-protobuf (192 ms) [ RUN ] ContentType/AgentAPITest.GetResourceProviders/1 ../src/tests/api_tests.cpp:7091: Failure Expected: 0 To be equal to: v1Response->get_resource_providers().resource_providers_size() Which is: 1 [ FAILED ] ContentType/AgentAPITest.GetResourceProviders/1, where GetParam() = application/json (164 ms) ``` Also, we don't need to set up the ACL for `DEFAULT_CREDENTIAL` since the permissive mode is turned on by default. - Chun-Hung ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/68104/#review206637 ----------------------------------------------------------- On July 31, 2018, 10:19 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/68104/ > ----------------------------------------------------------- > > (Updated July 31, 2018, 10:19 a.m.) > > > Review request for mesos, Chun-Hung Hsiao and Jan Schlicht. > > > Bugs: MESOS-8314 > https://issues.apache.org/jira/browse/MESOS-8314 > > > Repository: mesos > > > Description > ------- > > Implemented authorization for agent `GET_RESOURCE_PROVIDER` calls. > > > Diffs > ----- > > src/slave/http.cpp ab5864d9fd2fde478ed7da2ca7ed8abedc72c7c5 > src/tests/api_tests.cpp 182622a62d350ebefc891a385de3f2d35a7c0243 > > > Diff: https://reviews.apache.org/r/68104/diff/2/ > > > Testing > ------- > > `make check` > > > Thanks, > > Benjamin Bannier > >