mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam B" <a...@mesosphere.io>
Subject Re: Review Request 35711: Disallow special characters in role name.
Date Mon, 29 Jun 2015 11:17:26 GMT

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


Looks great. I think you're covering all the places where roles are specified now. But I'd
like to see the validate() method move onto a Role/Roles class somewhere like Resources, rather
than freestanding in an internal namespace.


src/common/validation.hpp (lines 27 - 34)
<https://reviews.apache.org/r/35711/#comment142465>

    I'm not convinced this is the right namespace/scope for a freestanding validate() function.
Maybe, like we have a Resource protobuf message and a Resources class with parse() and validate(),
we should also have a Roles::validate().



src/common/validation.cpp (lines 20 - 26)
<https://reviews.apache.org/r/35711/#comment142489>

    Unused?



src/common/validation.cpp (lines 43 - 44)
<https://reviews.apache.org/r/35711/#comment142285>

    I suppose role name ".." would also be invalid by similar logic (cannot `mkdir ..`)



src/common/validation.cpp (lines 50 - 55)
<https://reviews.apache.org/r/35711/#comment142468>

    s/space characters/whitespace/



src/common/validation.cpp (line 58)
<https://reviews.apache.org/r/35711/#comment142491>

    What is `\x20`? You didn't comment it.



src/common/validation.cpp (line 61)
<https://reviews.apache.org/r/35711/#comment142467>

    s/space/whitespace/



src/master/flags.cpp (line 189)
<https://reviews.apache.org/r/35711/#comment142493>

    This could be a quick Roles::parse()



src/master/flags.cpp (lines 194 - 196)
<https://reviews.apache.org/r/35711/#comment142471>

    Check for the `*` first, then the other check can just `return Role::validate(role);`.



src/slave/flags.cpp (line 52)
<https://reviews.apache.org/r/35711/#comment142474>

    s/value/resourcesString/ since 'value' is referenced as something else in the help string
above.



src/slave/flags.cpp (lines 61 - 62)
<https://reviews.apache.org/r/35711/#comment142477>

    How about `return Resources::validate(parsed.get());`



src/tests/common/validation_tests.cpp (lines 38 - 40)
<https://reviews.apache.org/r/35711/#comment142482>

    Where are these being used?



src/tests/common/validation_tests.cpp (line 50)
<https://reviews.apache.org/r/35711/#comment142484>

    s/name/names/
    s/contains/contain/



src/tests/common/validation_tests.cpp (lines 59 - 60)
<https://reviews.apache.org/r/35711/#comment142480>

    Please add tests for strings starting with and strings containing a `-`.



src/tests/common/validation_tests.cpp (line 63)
<https://reviews.apache.org/r/35711/#comment142481>

    Test `\n` as well?


- Adam B


On June 27, 2015, 4:35 a.m., haosdent huang wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35711/
> -----------------------------------------------------------
> 
> (Updated June 27, 2015, 4:35 a.m.)
> 
> 
> Review request for mesos, Adam B and Jie Yu.
> 
> 
> Bugs: MESOS-2210
>     https://issues.apache.org/jira/browse/MESOS-2210
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Disallow special characters in role name.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am a064d17a6b62e6e3c8e190135bcc8cbbb0051ed5 
>   src/common/resources.cpp eb5476a0365fe65f474afd0ab7a52ad7f1e04521 
>   src/common/validation.hpp PRE-CREATION 
>   src/common/validation.cpp PRE-CREATION 
>   src/master/flags.cpp 4377715029878cfee36f067e9c53c42b522b79d8 
>   src/slave/flags.cpp 6ba5a1bdc9f91aa1977f13b3aeec0fe0604687a0 
>   src/tests/common/validation_tests.cpp PRE-CREATION 
>   src/tests/resources_tests.cpp 30c10f3246b807800985f20f8835090657a2d56e 
> 
> Diff: https://reviews.apache.org/r/35711/diff/
> 
> 
> Testing
> -------
> 
> make -j8 check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>


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