You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rojas <al...@mesosphere.io> on 2017/05/22 15:53:28 UTC
Review Request 59453: Renamed RegisterAgent.agent to
RegisterAgent.agents in acls.proto.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59453/
-----------------------------------------------------------
Review request for mesos, Adam B, Greg Mann, Michael Park, and Neil Conway.
Repository: mesos
Description
-------
Renames the field `RegisterAgent.agent` to `RegisterAgent.agents` in
order to come make it consistent with other ACLs.
Diffs
-----
include/mesos/authorizer/acls.proto ae0b1ea2e6417d186b1606542d75f3a20e0811db
src/authorizer/local/authorizer.cpp 89aaf4b712d337d519445c922606789c334e5101
src/tests/authorization_tests.cpp 32aa6ac4db7854507127ea2fb88b3e92daa277c0
src/tests/master_authorization_tests.cpp e4233c19b1d9e3e2734259503d0daec4ce243667
src/tests/script.cpp 791d331d6bdf178098b2ce0dfb185f9128632459
Diff: https://reviews.apache.org/r/59453/diff/1/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 59453: Renamed RegisterAgent.agent to
RegisterAgent.agents in acls.proto.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On May 24, 2017, 10:26 a.m., Jiang Yan Xu wrote:
> > Hmm, is this also violating the convention then?
> >
> > https://github.com/apache/mesos/blob/d225d4d4122e773e2416ba0d0eee653da8ced352/include/mesos/authorizer/acls.proto#L344
> >
> > What if later we use uncountable nouns?
> >
> > FWIW when the rest of the ACLs read pretty natural to me, either the entity is a list of values, or the concept itself is plural (e.g., flags).
>
> Jiang Yan Xu wrote:
> I am not so sure about this "convention".
>
> Entity is either a list of values or ANY/NONE type. If it's a list of values, for sure the field name should be indicative of this. If it's a ANY/NONE, why does it have to be plural?
>
> Alexander Rojas wrote:
> I discussed that one too, but the idea there is that there is only one log, while you have multiple agents.
Alright. I guess it comes down to whether you say "authorzie this action for ANY/No(ne) agent" or "authorzie this action for ANY/No(ne) agent**s**"? I preferred the former but I guess I was wrong about the English grammar.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59453/#review175959
-----------------------------------------------------------
On May 24, 2017, 1:12 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59453/
> -----------------------------------------------------------
>
> (Updated May 24, 2017, 1:12 a.m.)
>
>
> Review request for mesos, Adam B, Greg Mann, Michael Park, and Neil Conway.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Renames the field `RegisterAgent.agent` to `RegisterAgent.agents` in
> order to come make it consistent with other ACLs.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/acls.proto ae0b1ea2e6417d186b1606542d75f3a20e0811db
> src/authorizer/local/authorizer.cpp 89aaf4b712d337d519445c922606789c334e5101
> src/tests/authorization_tests.cpp 32aa6ac4db7854507127ea2fb88b3e92daa277c0
> src/tests/master_authorization_tests.cpp e4233c19b1d9e3e2734259503d0daec4ce243667
> src/tests/script.cpp 791d331d6bdf178098b2ce0dfb185f9128632459
>
>
> Diff: https://reviews.apache.org/r/59453/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 59453: Renamed RegisterAgent.agent to
RegisterAgent.agents in acls.proto.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On May 24, 2017, 10:26 a.m., Jiang Yan Xu wrote:
> > Hmm, is this also violating the convention then?
> >
> > https://github.com/apache/mesos/blob/d225d4d4122e773e2416ba0d0eee653da8ced352/include/mesos/authorizer/acls.proto#L344
> >
> > What if later we use uncountable nouns?
> >
> > FWIW when the rest of the ACLs read pretty natural to me, either the entity is a list of values, or the concept itself is plural (e.g., flags).
I am not so sure about this "convention".
Entity is either a list of values or ANY/NONE type. If it's a list of values, for sure the field name should be indicative of this. If it's a ANY/NONE, why does it have to be plural?
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59453/#review175959
-----------------------------------------------------------
On May 24, 2017, 1:12 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59453/
> -----------------------------------------------------------
>
> (Updated May 24, 2017, 1:12 a.m.)
>
>
> Review request for mesos, Adam B, Greg Mann, Michael Park, and Neil Conway.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Renames the field `RegisterAgent.agent` to `RegisterAgent.agents` in
> order to come make it consistent with other ACLs.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/acls.proto ae0b1ea2e6417d186b1606542d75f3a20e0811db
> src/authorizer/local/authorizer.cpp 89aaf4b712d337d519445c922606789c334e5101
> src/tests/authorization_tests.cpp 32aa6ac4db7854507127ea2fb88b3e92daa277c0
> src/tests/master_authorization_tests.cpp e4233c19b1d9e3e2734259503d0daec4ce243667
> src/tests/script.cpp 791d331d6bdf178098b2ce0dfb185f9128632459
>
>
> Diff: https://reviews.apache.org/r/59453/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 59453: Renamed RegisterAgent.agent to
RegisterAgent.agents in acls.proto.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59453/#review175959
-----------------------------------------------------------
Hmm, is this also violating the convention then?
https://github.com/apache/mesos/blob/d225d4d4122e773e2416ba0d0eee653da8ced352/include/mesos/authorizer/acls.proto#L344
What if later we use uncountable nouns?
FWIW when the rest of the ACLs read pretty natural to me, either the entity is a list of values, or the concept itself is plural (e.g., flags).
- Jiang Yan Xu
On May 24, 2017, 1:12 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59453/
> -----------------------------------------------------------
>
> (Updated May 24, 2017, 1:12 a.m.)
>
>
> Review request for mesos, Adam B, Greg Mann, Michael Park, and Neil Conway.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Renames the field `RegisterAgent.agent` to `RegisterAgent.agents` in
> order to come make it consistent with other ACLs.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/acls.proto ae0b1ea2e6417d186b1606542d75f3a20e0811db
> src/authorizer/local/authorizer.cpp 89aaf4b712d337d519445c922606789c334e5101
> src/tests/authorization_tests.cpp 32aa6ac4db7854507127ea2fb88b3e92daa277c0
> src/tests/master_authorization_tests.cpp e4233c19b1d9e3e2734259503d0daec4ce243667
> src/tests/script.cpp 791d331d6bdf178098b2ce0dfb185f9128632459
>
>
> Diff: https://reviews.apache.org/r/59453/diff/3/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 59453: Renamed RegisterAgent.agent to
RegisterAgent.agents in acls.proto.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59453/
-----------------------------------------------------------
(Updated May 24, 2017, 10:12 a.m.)
Review request for mesos, Adam B, Greg Mann, Michael Park, and Neil Conway.
Repository: mesos
Description
-------
Renames the field `RegisterAgent.agent` to `RegisterAgent.agents` in
order to come make it consistent with other ACLs.
Diffs (updated)
-----
include/mesos/authorizer/acls.proto ae0b1ea2e6417d186b1606542d75f3a20e0811db
src/authorizer/local/authorizer.cpp 89aaf4b712d337d519445c922606789c334e5101
src/tests/authorization_tests.cpp 32aa6ac4db7854507127ea2fb88b3e92daa277c0
src/tests/master_authorization_tests.cpp e4233c19b1d9e3e2734259503d0daec4ce243667
src/tests/script.cpp 791d331d6bdf178098b2ce0dfb185f9128632459
Diff: https://reviews.apache.org/r/59453/diff/3/
Changes: https://reviews.apache.org/r/59453/diff/2-3/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 59453: Renamed RegisterAgent.agent to
RegisterAgent.agents in acls.proto.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59453/
-----------------------------------------------------------
(Updated May 24, 2017, 10:08 a.m.)
Review request for mesos, Adam B, Greg Mann, Michael Park, and Neil Conway.
Repository: mesos
Description (updated)
-------
Renames the field `RegisterAgent.agent` to `RegisterAgent.agents` in
order to come make it consistent with other ACLs.
Diffs (updated)
-----
include/mesos/authorizer/acls.proto ae0b1ea2e6417d186b1606542d75f3a20e0811db
src/authorizer/local/authorizer.cpp 89aaf4b712d337d519445c922606789c334e5101
src/tests/authorization_tests.cpp 32aa6ac4db7854507127ea2fb88b3e92daa277c0
src/tests/master_authorization_tests.cpp e4233c19b1d9e3e2734259503d0daec4ce243667
src/tests/script.cpp 791d331d6bdf178098b2ce0dfb185f9128632459
Diff: https://reviews.apache.org/r/59453/diff/2/
Changes: https://reviews.apache.org/r/59453/diff/1-2/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 59453: Renamed RegisterAgent.agent to
RegisterAgent.agents in acls.proto.
Posted by Neil Conway <ne...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/59453/#review175820
-----------------------------------------------------------
Fix it, then Ship it!
src/authorizer/local/authorizer.cpp
Line 1342 (original), 1342 (patched)
<https://reviews.apache.org/r/59453/#comment249154>
Should be `acls.register_agents.agent` here.
- Neil Conway
On May 22, 2017, 3:53 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59453/
> -----------------------------------------------------------
>
> (Updated May 22, 2017, 3:53 p.m.)
>
>
> Review request for mesos, Adam B, Greg Mann, Michael Park, and Neil Conway.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Renames the field `RegisterAgent.agent` to `RegisterAgent.agents` in
> order to come make it consistent with other ACLs.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/acls.proto ae0b1ea2e6417d186b1606542d75f3a20e0811db
> src/authorizer/local/authorizer.cpp 89aaf4b712d337d519445c922606789c334e5101
> src/tests/authorization_tests.cpp 32aa6ac4db7854507127ea2fb88b3e92daa277c0
> src/tests/master_authorization_tests.cpp e4233c19b1d9e3e2734259503d0daec4ce243667
> src/tests/script.cpp 791d331d6bdf178098b2ce0dfb185f9128632459
>
>
> Diff: https://reviews.apache.org/r/59453/diff/1/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>