mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 46319: Added authorization to agents' `/monitor/statistics` endpoints.
Date Wed, 27 Apr 2016 12:18:50 GMT


> On April 26, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/slave/http.cpp, line 635
> > <https://reviews.apache.org/r/46319/diff/6/?file=1361111#file1361111line635>
> >
> >     Indent the block by 2 spaces, not 4.
> 
> Benjamin Bannier wrote:
>     ditto.

Fixed.


> On April 26, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/slave/http.cpp, line 637
> > <https://reviews.apache.org/r/46319/diff/6/?file=1361111#file1361111line637>
> >
> >     Indent the block by 2 spaces, not 4.
> 
> Benjamin Bannier wrote:
>     ditto.

Fixed.


> On April 26, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/tests/slave_authorization_tests.cpp, lines 172-173
> > <https://reviews.apache.org/r/46319/diff/6/?file=1361113#file1361113line172>
> >
> >     I was about to ask why don't you use parameterized tests, but noticed you do
it in the next review. Any reason you don't want to fix it up in this patch?

Done.


> On April 26, 2016, 6:51 p.m., Alexander Rukletsov wrote:
> > src/slave/http.cpp, line 627
> > <https://reviews.apache.org/r/46319/diff/6/?file=1361111#file1361111line627>
> >
> >     Indent the block by 2 spaces, not 4.
> 
> Benjamin Bannier wrote:
>     Knowing we care a lot about this, I did actually put some care into making this consistent
with the style guide.
>     
>     Here and elsewhere: Our style guide states in the section *Function Definition/Invocation*
that
>     > Newline when calling or defining a function: indent with four spaces.
>     
>     Are you confusing this with the rule on continuations?,
>     > Newline for an assignment statement: indent with two spaces.
>     
>     I believe we are not performing an assignment, but a function invocation here.
>     
>     Note that further down in the style guide there are examples of `.then` inconsistent
with the indention rules.
> 
> Adam B wrote:
>     After a quick grep, it seems that 2 spaces is indeed the de facto standard. Even
in /src/slave/, I can only find two (of 126) instances that indent by 4 spaces.

Indenting by two spaces now.

It would be great if somebody in the know would unambiguously update the style guide to the
latest source of truth.


- Benjamin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/46319/#review130639
-----------------------------------------------------------


On April 27, 2016, 2:18 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46319/
> -----------------------------------------------------------
> 
> (Updated April 27, 2016, 2:18 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jan Schlicht.
> 
> 
> Bugs: MESOS-5164
>     https://issues.apache.org/jira/browse/MESOS-5164
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added authorization to agents' `/monitor/statistics` endpoints.
> 
> 
> Diffs
> -----
> 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46319/diff/
> 
> 
> Testing
> -------
> 
> make check (OS X, clang w/o optimization)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message