You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Isabel Jimenez <co...@isabeljimenez.com> on 2014/05/12 19:02:29 UTC
Review Request 21324: Unauthorized HTTP Response + tests
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21324/
-----------------------------------------------------------
Review request for mesos, Adam B and Benjamin Hindman.
Bugs: MESOS-1131
https://issues.apache.org/jira/browse/MESOS-1131
Repository: mesos-git
Description
-------
2nd Phase following Ben's comments on https://reviews.apache.org/r/19575/
Diffs
-----
3rdparty/libprocess/include/process/http.hpp 06f2596
3rdparty/libprocess/src/tests/http_tests.cpp f58a129
Diff: https://reviews.apache.org/r/21324/diff/
Testing
-------
Thanks,
Isabel Jimenez
Re: Review Request 21324: Unauthorized HTTP Response + tests
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21324/#review42753
-----------------------------------------------------------
3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/21324/#comment76630>
brace on previous line, indent 2 within braces.
3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/21324/#comment76631>
drop brace on function definitions.
3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/21324/#comment76632>
EXPECT_EQ would be better as ASSERT_EQ will stop the test.
- Dominic Hamon
On May 12, 2014, 10:02 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21324/
> -----------------------------------------------------------
>
> (Updated May 12, 2014, 10:02 a.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1131
> https://issues.apache.org/jira/browse/MESOS-1131
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> 2nd Phase following Ben's comments on https://reviews.apache.org/r/19575/
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 06f2596
> 3rdparty/libprocess/src/tests/http_tests.cpp f58a129
>
> Diff: https://reviews.apache.org/r/21324/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 21324: Unauthorized HTTP Response + tests
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On May 19, 2014, 7:44 p.m., Benjamin Hindman wrote:
> > 3rdparty/libprocess/src/tests/http_tests.cpp, line 84
> > <https://reviews.apache.org/r/21324/diff/2/?file=579238#file579238line84>
> >
> > Did you want "" or None() for the third (i.e., 'query') argument? Same for the call to http::get below too.
I changed to None() to be more explicit, I think a query is not necessary for this test.
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21324/#review43405
-----------------------------------------------------------
On May 19, 2014, 8:11 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21324/
> -----------------------------------------------------------
>
> (Updated May 19, 2014, 8:11 p.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1131
> https://issues.apache.org/jira/browse/MESOS-1131
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> 2nd Phase following Ben's comments on https://reviews.apache.org/r/19575/
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 06f2596
> 3rdparty/libprocess/src/tests/http_tests.cpp f58a129
>
> Diff: https://reviews.apache.org/r/21324/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 21324: Unauthorized HTTP Response + tests
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21324/#review43405
-----------------------------------------------------------
Ship it!
Sweet! Very minor cleanup then we'll get this committed.
3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/21324/#comment77501>
Prefer full names when applicable. I.e., 'request' is better than 'r' here, but for example you'll sometimes see us take a 'const string& s' just because there isn't a better identifier for the variable.
3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/21324/#comment77502>
Oops, bad indentation here.
3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/21324/#comment77503>
We try and use two newlines between top-level definitions/declarations please.
3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/21324/#comment77504>
To be consistent let's use 'HTTP' instead of 'http' to label these tests please.
3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/21324/#comment77505>
Did you want "" or None() for the third (i.e., 'query') argument? Same for the call to http::get below too.
- Benjamin Hindman
On May 12, 2014, 10:53 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21324/
> -----------------------------------------------------------
>
> (Updated May 12, 2014, 10:53 p.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1131
> https://issues.apache.org/jira/browse/MESOS-1131
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> 2nd Phase following Ben's comments on https://reviews.apache.org/r/19575/
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 06f2596
> 3rdparty/libprocess/src/tests/http_tests.cpp f58a129
>
> Diff: https://reviews.apache.org/r/21324/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 21324: Unauthorized HTTP Response + tests
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21324/
-----------------------------------------------------------
(Updated May 19, 2014, 8:11 p.m.)
Review request for mesos, Adam B and Benjamin Hindman.
Bugs: MESOS-1131
https://issues.apache.org/jira/browse/MESOS-1131
Repository: mesos-git
Description
-------
2nd Phase following Ben's comments on https://reviews.apache.org/r/19575/
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp 06f2596
3rdparty/libprocess/src/tests/http_tests.cpp f58a129
Diff: https://reviews.apache.org/r/21324/diff/
Testing
-------
Thanks,
Isabel Jimenez
Re: Review Request 21324: Unauthorized HTTP Response + tests
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21324/
-----------------------------------------------------------
(Updated May 21, 2014, 11:54 p.m.)
Review request for mesos, Adam B and Benjamin Hindman.
Bugs: MESOS-1131
https://issues.apache.org/jira/browse/MESOS-1131
Repository: mesos-git
Description
-------
2nd Phase following Ben's comments on https://reviews.apache.org/r/19575/
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp 06f2596
3rdparty/libprocess/src/tests/http_tests.cpp f58a129
Diff: https://reviews.apache.org/r/21324/diff/
Testing
-------
Thanks,
Isabel Jimenez
Re: Review Request 21324: Unauthorized HTTP Response + tests
Posted by Benjamin Hindman <be...@berkeley.edu>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21324/#review43666
-----------------------------------------------------------
3rdparty/libprocess/src/tests/http_tests.cpp
<https://reviews.apache.org/r/21324/#comment77958>
Hmmm, I don't think that we should expect to get a realm from the query. The realm will be set by the actual code implementing the callback (i.e., the "origin server") when it creates an Unauthorized (IIUC). Speaking of which, we'll always want the Unauthorized constructor to take a realm so I think we'll want:
Unauthorized(const std::string& realm)
and
Unauthorized(const std::string& realm, const std::string& body)
That is, you always have to specify the realm.
- Benjamin Hindman
On May 21, 2014, 3:39 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21324/
> -----------------------------------------------------------
>
> (Updated May 21, 2014, 3:39 a.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1131
> https://issues.apache.org/jira/browse/MESOS-1131
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> 2nd Phase following Ben's comments on https://reviews.apache.org/r/19575/
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 06f2596
> 3rdparty/libprocess/src/tests/http_tests.cpp f58a129
>
> Diff: https://reviews.apache.org/r/21324/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 21324: Unauthorized HTTP Response + tests
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21324/
-----------------------------------------------------------
(Updated May 21, 2014, 3:39 a.m.)
Review request for mesos, Adam B and Benjamin Hindman.
Bugs: MESOS-1131
https://issues.apache.org/jira/browse/MESOS-1131
Repository: mesos-git
Description
-------
2nd Phase following Ben's comments on https://reviews.apache.org/r/19575/
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp 06f2596
3rdparty/libprocess/src/tests/http_tests.cpp f58a129
Diff: https://reviews.apache.org/r/21324/diff/
Testing
-------
Thanks,
Isabel Jimenez
Re: Review Request 21324: Unauthorized HTTP Response + tests
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
> On May 20, 2014, 5:21 p.m., Dominic Hamon wrote:
> > 3rdparty/libprocess/include/process/http.hpp, line 240
> > <https://reviews.apache.org/r/21324/diff/3/?file=584469#file584469line240>
> >
> > do we want to set 'realm' here to give a friendly message to users?
I think you're right I updated Unauthorized.
- Isabel
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21324/#review43498
-----------------------------------------------------------
On May 21, 2014, 3:39 a.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21324/
> -----------------------------------------------------------
>
> (Updated May 21, 2014, 3:39 a.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1131
> https://issues.apache.org/jira/browse/MESOS-1131
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> 2nd Phase following Ben's comments on https://reviews.apache.org/r/19575/
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 06f2596
> 3rdparty/libprocess/src/tests/http_tests.cpp f58a129
>
> Diff: https://reviews.apache.org/r/21324/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 21324: Unauthorized HTTP Response + tests
Posted by Dominic Hamon <dh...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21324/#review43498
-----------------------------------------------------------
3rdparty/libprocess/include/process/http.hpp
<https://reviews.apache.org/r/21324/#comment77670>
do we want to set 'realm' here to give a friendly message to users?
- Dominic Hamon
On May 19, 2014, 1:11 p.m., Isabel Jimenez wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/21324/
> -----------------------------------------------------------
>
> (Updated May 19, 2014, 1:11 p.m.)
>
>
> Review request for mesos, Adam B and Benjamin Hindman.
>
>
> Bugs: MESOS-1131
> https://issues.apache.org/jira/browse/MESOS-1131
>
>
> Repository: mesos-git
>
>
> Description
> -------
>
> 2nd Phase following Ben's comments on https://reviews.apache.org/r/19575/
>
>
> Diffs
> -----
>
> 3rdparty/libprocess/include/process/http.hpp 06f2596
> 3rdparty/libprocess/src/tests/http_tests.cpp f58a129
>
> Diff: https://reviews.apache.org/r/21324/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Isabel Jimenez
>
>
Re: Review Request 21324: Unauthorized HTTP Response + tests
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21324/
-----------------------------------------------------------
(Updated May 19, 2014, 8:11 p.m.)
Review request for mesos, Adam B and Benjamin Hindman.
Bugs: MESOS-1131
https://issues.apache.org/jira/browse/MESOS-1131
Repository: mesos-git
Description
-------
2nd Phase following Ben's comments on https://reviews.apache.org/r/19575/
Diffs
-----
3rdparty/libprocess/include/process/http.hpp 06f2596
3rdparty/libprocess/src/tests/http_tests.cpp f58a129
Diff: https://reviews.apache.org/r/21324/diff/
Testing
-------
Thanks,
Isabel Jimenez
Re: Review Request 21324: Unauthorized HTTP Response + tests
Posted by Isabel Jimenez <co...@isabeljimenez.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/21324/
-----------------------------------------------------------
(Updated May 12, 2014, 10:53 p.m.)
Review request for mesos, Adam B and Benjamin Hindman.
Bugs: MESOS-1131
https://issues.apache.org/jira/browse/MESOS-1131
Repository: mesos-git
Description
-------
2nd Phase following Ben's comments on https://reviews.apache.org/r/19575/
Diffs (updated)
-----
3rdparty/libprocess/include/process/http.hpp 06f2596
3rdparty/libprocess/src/tests/http_tests.cpp f58a129
Diff: https://reviews.apache.org/r/21324/diff/
Testing
-------
Thanks,
Isabel Jimenez