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



Overall looks in decent shape, made some suggestions.

Before this is ready to land, please:
- add documentation
- cross-link with the jira ticket


src/main/python/apache/aurora/executor/common/announcer.py (line 128)
<https://reviews.apache.org/r/45042/#comment186991>

    please refactor to make the constructor accept the ACL list, making the 
caller read/parse.  this improves testability and reduces the surface area of 
the class.  it also moves errors like file access and parsing issues closer to 
the source



src/main/python/apache/aurora/executor/common/announcer.py (lines 149 - 151)
<https://reviews.apache.org/r/45042/#comment186994>

    please read the json as a list, to support multiple ACLs (mentioned this in 
the ticket)



src/main/python/apache/aurora/executor/common/announcer.py (line 153)
<https://reviews.apache.org/r/45042/#comment186993>

    -1 on the approach of logging and moving forward without auth when errors 
are encountered.  these should all be fatal



src/main/python/apache/aurora/executor/common/announcer.py (line 162)
<https://reviews.apache.org/r/45042/#comment186992>

    General tip, avoid relying on truthiness when you really mean to compare 
against `None`
    
    From https://www.python.org/dev/peps/pep-0008/#programming-recommendations
    
    > beware of writing if x when you really mean if x is not None -- e.g. when 
testing whether a variable or argument that defaults to None was set to some 
other value. The other value might have a type (such as a container) that could 
be false in a boolean context


- Bill Farner


On March 18, 2016, 2:57 p.m., Kunal Thakar wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45042/
> -----------------------------------------------------------
> 
> (Updated March 18, 2016, 2:57 p.m.)
> 
> 
> Review request for Aurora, Bill Farner and Zameer Manji.
> 
> 
> Repository: aurora
> 
> 
> Description
> -------
> 
> Add ACL support for announcer
> https://issues.apache.org/jira/browse/AURORA-1643
> 
> Adding support for service discovery ZK authentication. ZK authentication 
> secrets should be stored in a file as json (as follows):
> ```json
> {
>   "scheme": "<scheme>",
>   "credential": "<credential>",
>       "permissions": {
>         "read": <bool>,
>         "write": <bool>,
>         "create": <bool>,
>         "delete": <bool>,
>         "admin": <bool>,
>         "all": <bool>
>       }
> }
> ```
> 
> 
> Diffs
> -----
> 
>   src/main/python/apache/aurora/executor/bin/thermos_executor_main.py 
> 6634506108c346f8c23b2da7cc8d20d09d07d590 
>   src/main/python/apache/aurora/executor/common/announcer.py 
> 79a9cfb6ac3a8444f09fb3658e6e859e06941ba4 
>   src/test/python/apache/aurora/executor/common/test_announcer.py 
> 142b58d5e577c9f4b8e2ae8473cffdea94eba21f 
> 
> Diff: https://reviews.apache.org/r/45042/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Kunal Thakar
> 
>

Reply via email to