Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 14, 2016, 1:33 a.m., Adam B wrote: > > Looks great! Just a few minor items that I can fix myself. I'll commit this > > shortly. > > haosdent huang wrote: > Thank you very much! No sir, thank you! - Adam --- This is an au

Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 14, 2016, 9:33 a.m., Adam B wrote: > > Looks great! Just a few minor items that I can fix myself. I'll commit this > > shortly. Thank you very much! - haosdent --- This is an automatically generated e-mail. To reply, visit: h

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review114435 --- Ship it! Looks great! Just a few minor items that I can fix myself

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review114439 --- Patch looks great! Reviews applied: [35711] Passed command: expor

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Jan. 14, 2016, 6 a.m.) Review request for mesos, Adam B, Jie Yu, and M

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review114367 --- Ship it! Ship It! - Klaus Ma On Jan. 14, 2016, 1:12 a.m., haosd

Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 10, 2016, 7:44 a.m., Klaus Ma wrote: > > @klaus1982 @adam-mesos Thank you very much for your detail reviews. Could you help review again? Thank you very much. - haosdent --- This is an automatically generated e-mail. To reply

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Jan. 13, 2016, 5:12 p.m.) Review request for mesos, Adam B, Jie Yu, an

Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 10, 2016, 3:44 p.m., Klaus Ma wrote: > > src/common/roles.cpp, line 34 > > > > > > Do we need to check whether duplicated roles here? > > Adam B wrote: > Not necessary here, since they're de-duped when

Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 10, 2016, 7:44 a.m., Klaus Ma wrote: > > src/common/roles.cpp, lines 56-63 > > > > > > Just another question about the role: do we support other language, > > e.g. Chinese? If not, I'd suggest also to high

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review113644 --- Ship it! Looks good to me. We can commit this once you've resolved

Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 9, 2016, 11:44 p.m., Klaus Ma wrote: > > src/common/roles.cpp, line 32 > > > > > > Do we need to define a const value `REOLE_SPLITOR` to replace ','? > > Adam B wrote: > I think it's unnecessary, since

Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 9, 2016, 11:44 p.m., Klaus Ma wrote: > > src/common/roles.cpp, line 32 > > > > > > Do we need to define a const value `REOLE_SPLITOR` to replace ','? I think it's unnecessary, since this is the only place

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review113639 --- include/mesos/roles.hpp (line 51)

Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 7, 2016, 10:51 p.m., Adam B wrote: > > Still wondering if `class Roles` is any better than `namespace roles`, and > > unsure about the need for the CMake change. Thank you very much for your great reviews! Could you help review again? I change to use namespaces roles. - haosdent

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Jan. 9, 2016, 11:15 a.m.) Review request for mesos, Adam B, Jie Yu, an

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review113287 --- Still wondering if `class Roles` is any better than `namespace role

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review112878 --- Patch looks great! Reviews applied: [35711] Passed command: expor

Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 5, 2016, 8:16 a.m., Adam B wrote: > > Looks good. My biggest question is whether `class Roles` needs to exist, or > > whether we can just use freestanding `roles::validate()` functions. Thank you very much. I use vector instead. Could you help review again? - haosdent

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Jan. 5, 2016, 3:19 p.m.) Review request for mesos, Adam B, Jie Yu, and

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review112788 --- Looks good. My biggest question is whether `class Roles` needs to e

Re: Review Request 35711: Disallow special characters in role name.

> On 一月 5, 2016, 5:51 a.m., Guangya Liu wrote: > > include/mesos/roles.hpp, lines 29-49 > > > > > > It is better use /**/ for comments in a header file. > > haosdent huang wrote: > According resources.hpp, seems

Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 5, 2016, 5:51 a.m., Guangya Liu wrote: > > include/mesos/roles.hpp, lines 29-49 > > > > > > It is better use /**/ for comments in a header file. > > haosdent huang wrote: > According resources.hpp, see

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Jan. 5, 2016, 6:58 a.m.) Review request for mesos, Adam B, Jie Yu, and

Re: Review Request 35711: Disallow special characters in role name.

> On 一月 5, 2016, 5:51 a.m., Guangya Liu wrote: > > include/mesos/roles.hpp, lines 29-49 > > > > > > It is better use /**/ for comments in a header file. > > haosdent huang wrote: > According resources.hpp, seems

Re: Review Request 35711: Disallow special characters in role name.

> On Jan. 5, 2016, 5:51 a.m., Guangya Liu wrote: > > include/mesos/roles.hpp, lines 29-49 > > > > > > It is better use /**/ for comments in a header file. According resources.hpp, seems it is OK here. https://githu

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review112771 --- include/mesos/roles.hpp (lines 29 - 49)

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Jan. 5, 2016, 2:48 a.m.) Review request for mesos, Adam B, Jie Yu, and

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Jan. 5, 2016, 2:42 a.m.) Review request for mesos, Adam B, Jie Yu, and

Re: Review Request 35711: Disallow special characters in role name.

> On Dec. 15, 2015, 2:45 a.m., Adam B wrote: > > Thanks for reviving this, and sorry it's taken so long to get back to it. > > Adam B wrote: > You'll also need to add some documentation about what the valid/invalid > role names are, and when they'll be rejected. Could you add this info to

Re: Review Request 35711: Disallow special characters in role name.

> On Dec. 15, 2015, 2:45 a.m., Adam B wrote: > > Thanks for reviving this, and sorry it's taken so long to get back to it. You'll also need to add some documentation about what the valid/invalid role names are, and when they'll be rejected. - Adam ---

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review110461 --- Thanks for reviving this, and sorry it's taken so long to get back

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review110341 --- Looking good! Minor nit below. src/tests/roles_tests.cpp (line 55

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Dec. 12, 2015, 2:33 p.m.) Review request for mesos, Adam B, Jie Yu, an

Re: Review Request 35711: Disallow special characters in role name.

> On Nov. 9, 2015, 11:26 p.m., Greg Mann wrote: > > src/master/master.cpp, line 568 > > > > > > Should we add a comment here saying that the validation of roles occurs > > in flags.cpp? I put off the check to `Mas

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Dec. 12, 2015, 2:29 p.m.) Review request for mesos, Adam B, Jie Yu, an

Re: Review Request 35711: Disallow special characters in role name.

> On Nov. 5, 2015, 11:59 p.m., Neil Conway wrote: > > include/mesos/roles.hpp, line 51 > > > > > > Should this be a member function? i.e., not static. When I write this, I refer to the static validate method in Reso

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review105756 --- include/mesos/roles.hpp (lines 40 - 41)

Re: Review Request 35711: Disallow special characters in role name.

> On Nov. 5, 2015, 3:59 p.m., Neil Conway wrote: > > Can we add an end-to-end unit test that verifies that attempting to > > register as a framework with an invalid role name results in an error? Attempting to register with an unrecognized role name will fail long before we get to creating a d

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review105316 --- Can we add an end-to-end unit test that verifies that attempting to

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review102158 --- src/Makefile.am (line 467)

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review101411 --- Patch looks great! Reviews applied: [35711] All tests passed. -

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Oct. 3, 2015, 4:11 p.m.) Review request for mesos, Adam B, Jie Yu, and

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review94631 --- Ship it! Ship It! - Guangya Liu On 八月 7, 2015, 6:50 p.m., haosde

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review94576 --- Patch looks great! Reviews applied: [35711] All tests passed. - M

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Aug. 7, 2015, 6:50 p.m.) Review request for mesos, Adam B, Jie Yu, and

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review94571 --- Bad patch! Reviews applied: [35711] Failed command: make -j3 distc

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Aug. 7, 2015, 3:48 p.m.) Review request for mesos, Adam B, Jie Yu, and

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated Aug. 7, 2015, 3:40 p.m.) Review request for mesos, Adam B, Jie Yu, and

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated June 30, 2015, 4:56 p.m.) Review request for mesos, Adam B and Jie Yu.

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated June 30, 2015, 8:30 a.m.) Review request for mesos, Adam B and Jie Yu.

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated June 30, 2015, 8:29 a.m.) Review request for mesos, Adam B and Jie Yu.

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated June 30, 2015, 7:53 a.m.) Review request for mesos, Adam B and Jie Yu.

Re: Review Request 35711: Disallow special characters in role name.

> On June 29, 2015, 11:17 a.m., Adam B wrote: > > src/common/validation.hpp, lines 27-34 > > > > > > I'm not convinced this is the right namespace/scope for a freestanding > > validate() function. Maybe, like we have a

Re: Review Request 35711: Disallow special characters in role name.

> On June 29, 2015, 11:17 a.m., Adam B wrote: > > src/common/validation.hpp, lines 27-34 > > > > > > I'm not convinced this is the right namespace/scope for a freestanding > > validate() function. Maybe, like we have a

Re: Review Request 35711: Disallow special characters in role name.

--- 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

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review89627 --- src/Makefile.am (line 1489)

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review89626 --- src/Makefile.am (line 362)

Re: Review Request 35711: Disallow special characters in role name.

> On June 24, 2015, 6:34 p.m., Jie Yu wrote: > > src/master/master.hpp, lines 630-651 > > > > > > Can you move this function to src/master/validation.hpp|cpp > > > > ``` > > namespace role { > > Option

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated June 27, 2015, 11:35 a.m.) Review request for mesos, Adam B and Jie Yu

Re: Review Request 35711: Disallow special characters in role name.

> On June 24, 2015, 9:30 a.m., Adam B wrote: > > Looks great! We recently added validation lambdas to flag definitions, so > > you may be able to take advantage of that. > > We probably also want to do validation in the slave for the --resources > > flag, since resources can be declared as rese

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated June 27, 2015, 11:32 a.m.) Review request for mesos, Adam B and Jie Yu

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated June 27, 2015, 8:36 a.m.) Review request for mesos, Adam B and Jie Yu.

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review89222 --- +1 on Adam's comments. src/master/master.hpp (lines 630 - 651)

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/#review89140 --- Looks great! We recently added validation lambdas to flag definition

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated June 21, 2015, 2:21 p.m.) Review request for mesos, Adam B and Jie Yu.

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated June 21, 2015, 2:16 p.m.) Review request for mesos, Adam B and Jie Yu.

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated June 21, 2015, 2:15 p.m.) Review request for mesos, Adam B and Jie Yu.

Re: Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- (Updated June 21, 2015, 2:09 p.m.) Review request for mesos, Adam B and Jie Yu.

Review Request 35711: Disallow special characters in role name.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35711/ --- Review request for mesos, Adam B and Jie Yu. Repository: mesos Description --