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 49394: Added support for VIEW_FLAGS authorization action in HTTP API.
Date Thu, 30 Jun 2016 10:00:00 GMT

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



Left a few comments. You introduced an `ErrorFlags::Type` enum which currently can only be
in a single state. Will you expand this later? Otherwise I am not sure this is as simple as
it can be.


src/master/http.cpp (line 1440)
<https://reviews.apache.org/r/49394/#comment205439>

    Please explicitly include `errorbase.hpp`.



src/master/http.cpp (line 1443)
<https://reviews.apache.org/r/49394/#comment205433>

    Make this an `enum class` to curb implicit conversions to integer types.



src/master/http.cpp (line 1446)
<https://reviews.apache.org/r/49394/#comment205438>

    `UNKNOWN` appears unused. Not sure why you added it.



src/master/http.cpp (line 1449)
<https://reviews.apache.org/r/49394/#comment205435>

    Make this `explicit`.



src/master/http.cpp (line 1453)
<https://reviews.apache.org/r/49394/#comment205436>

    Please provide conversions of `Type` for `stringify`. Currently and until you use an `enum
class` this will just output some integer value which is not super useful.



src/master/http.cpp (lines 1455 - 1456)
<https://reviews.apache.org/r/49394/#comment205434>

    Please make this `const` (the base class does the same).



src/master/http.cpp (line 1490)
<https://reviews.apache.org/r/49394/#comment205432>

    Since you already used an enum for `Type` you could use a `switch` here so we can catch
unhandled cases at compile time.



src/master/http.cpp (line 1561)
<https://reviews.apache.org/r/49394/#comment205437>

    `switch`.


- Benjamin Bannier


On June 30, 2016, 11:49 a.m., Alexander Rojas wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49394/
> -----------------------------------------------------------
> 
> (Updated June 30, 2016, 11:49 a.m.)
> 
> 
> Review request for mesos, Adam B and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Adds an intermediate function `Master::http::_flags()` which performs
> authorization and it is called by both the endpoint `/flags` handler
> and the HTTP API v1 call `flags` handler.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp e5acdb8e0bbcd7a2b7e8a8bc7f4bbeaae2c4fea1 
>   src/master/master.hpp e2ab2110fe5a287ab16ac9ef4222fed633e02ebe 
> 
> Diff: https://reviews.apache.org/r/49394/diff/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>


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