You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Till Toenshoff <to...@me.com> on 2017/09/26 22:54:24 UTC
Review Request 62592: Added basic HTTP authenticatee implementation.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62592/
-----------------------------------------------------------
Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
Bugs: MESOS-8017
https://issues.apache.org/jira/browse/MESOS-8017
Repository: mesos
Description
-------
Added basic HTTP authenticatee implementation.
Diffs
-----
src/CMakeLists.txt 1a828c4351ded36f51ccbbe67147da2f50b9cdb1
src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900
src/authentication/http/basic_authenticatee.hpp PRE-CREATION
src/authentication/http/basic_authenticatee.cpp PRE-CREATION
src/common/http.hpp 0e6b1c59860e75c04e2e3be2520ed2b39c84ac90
Diff: https://reviews.apache.org/r/62592/diff/1/
Testing
-------
Integration test in external project. Further tests upcoming.
Thanks,
Till Toenshoff
Re: Review Request 62592: Added basic HTTP authenticatee
implementation.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62592/#review187385
-----------------------------------------------------------
Ship it!
Ship It!
- Greg Mann
On Oct. 2, 2017, 10:12 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62592/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2017, 10:12 a.m.)
>
>
> Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
>
>
> Bugs: MESOS-8017
> https://issues.apache.org/jira/browse/MESOS-8017
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added basic HTTP authenticatee implementation.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 1a0dff3604c288d2af8ecc23f1ec73f5a350bc31
> src/Makefile.am da8af916652a80303f37b65056920676726f8dfe
> src/authentication/http/basic_authenticatee.hpp PRE-CREATION
> src/authentication/http/basic_authenticatee.cpp PRE-CREATION
> src/common/http.hpp 0e6b1c59860e75c04e2e3be2520ed2b39c84ac90
>
>
> Diff: https://reviews.apache.org/r/62592/diff/4/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62592: Added basic HTTP authenticatee
implementation.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62592/
-----------------------------------------------------------
(Updated Oct. 10, 2017, 6:07 p.m.)
Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
Changes
-------
comments.
Bugs: MESOS-8017
https://issues.apache.org/jira/browse/MESOS-8017
Repository: mesos
Description
-------
Moves the hardcoded basic HTTP authentication code from within the
scheduler library into the modularized HTTP authenticatee interface.
Diffs (updated)
-----
src/CMakeLists.txt 1a0dff3604c288d2af8ecc23f1ec73f5a350bc31
src/Makefile.am da8af916652a80303f37b65056920676726f8dfe
src/authentication/http/basic_authenticatee.hpp PRE-CREATION
src/authentication/http/basic_authenticatee.cpp PRE-CREATION
src/common/http.hpp 0e6b1c59860e75c04e2e3be2520ed2b39c84ac90
Diff: https://reviews.apache.org/r/62592/diff/5/
Changes: https://reviews.apache.org/r/62592/diff/4-5/
Testing
-------
Integration test in external project. Further tests upcoming.
Thanks,
Till Toenshoff
Re: Review Request 62592: Added basic HTTP authenticatee
implementation.
Posted by Armand Grillet <ag...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62592/#review186846
-----------------------------------------------------------
LGTM.
- Armand Grillet
On Oct. 2, 2017, 10:12 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62592/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2017, 10:12 a.m.)
>
>
> Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
>
>
> Bugs: MESOS-8017
> https://issues.apache.org/jira/browse/MESOS-8017
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added basic HTTP authenticatee implementation.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 1a828c4351ded36f51ccbbe67147da2f50b9cdb1
> src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900
> src/authentication/http/basic_authenticatee.hpp PRE-CREATION
> src/authentication/http/basic_authenticatee.cpp PRE-CREATION
> src/common/http.hpp 0e6b1c59860e75c04e2e3be2520ed2b39c84ac90
>
>
> Diff: https://reviews.apache.org/r/62592/diff/2/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62592: Added basic HTTP authenticatee
implementation.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62592/#review187320
-----------------------------------------------------------
src/authentication/http/basic_authenticatee.cpp
Lines 87-88 (patched)
<https://reviews.apache.org/r/62592/#comment264232>
Fits on one line.
- Greg Mann
On Oct. 2, 2017, 10:12 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62592/
> -----------------------------------------------------------
>
> (Updated Oct. 2, 2017, 10:12 a.m.)
>
>
> Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
>
>
> Bugs: MESOS-8017
> https://issues.apache.org/jira/browse/MESOS-8017
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added basic HTTP authenticatee implementation.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 1a0dff3604c288d2af8ecc23f1ec73f5a350bc31
> src/Makefile.am da8af916652a80303f37b65056920676726f8dfe
> src/authentication/http/basic_authenticatee.hpp PRE-CREATION
> src/authentication/http/basic_authenticatee.cpp PRE-CREATION
> src/common/http.hpp 0e6b1c59860e75c04e2e3be2520ed2b39c84ac90
>
>
> Diff: https://reviews.apache.org/r/62592/diff/3/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62592: Added basic HTTP authenticatee
implementation.
Posted by Till Toenshoff <to...@me.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62592/
-----------------------------------------------------------
(Updated Oct. 2, 2017, 10:12 a.m.)
Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
Changes
-------
addressed comments and fixed oversight in Makefile.am
Bugs: MESOS-8017
https://issues.apache.org/jira/browse/MESOS-8017
Repository: mesos
Description
-------
Added basic HTTP authenticatee implementation.
Diffs (updated)
-----
src/CMakeLists.txt 1a828c4351ded36f51ccbbe67147da2f50b9cdb1
src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900
src/authentication/http/basic_authenticatee.hpp PRE-CREATION
src/authentication/http/basic_authenticatee.cpp PRE-CREATION
src/common/http.hpp 0e6b1c59860e75c04e2e3be2520ed2b39c84ac90
Diff: https://reviews.apache.org/r/62592/diff/2/
Changes: https://reviews.apache.org/r/62592/diff/1-2/
Testing
-------
Integration test in external project. Further tests upcoming.
Thanks,
Till Toenshoff
Re: Review Request 62592: Added basic HTTP authenticatee
implementation.
Posted by Till Toenshoff <to...@me.com>.
> On Sept. 27, 2017, 9:21 p.m., Greg Mann wrote:
> > src/authentication/http/basic_authenticatee.cpp
> > Lines 75-76 (patched)
> > <https://reviews.apache.org/r/62592/diff/1/?file=1836080#file1836080line75>
> >
> > Hmm wouldn't we usually indent two spaces after the linebreak here?
>
> Till Toenshoff wrote:
> Again clangformat things otherwise - unsure actually bout these cases of a return type already needing a linebreak.
https://google.github.io/styleguide/cppguide.html#Function_Declarations_and_Definitions
> If you break after the return type of a function declaration or definition, do not indent.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62592/#review186476
-----------------------------------------------------------
On Sept. 26, 2017, 10:54 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62592/
> -----------------------------------------------------------
>
> (Updated Sept. 26, 2017, 10:54 p.m.)
>
>
> Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
>
>
> Bugs: MESOS-8017
> https://issues.apache.org/jira/browse/MESOS-8017
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added basic HTTP authenticatee implementation.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 1a828c4351ded36f51ccbbe67147da2f50b9cdb1
> src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900
> src/authentication/http/basic_authenticatee.hpp PRE-CREATION
> src/authentication/http/basic_authenticatee.cpp PRE-CREATION
> src/common/http.hpp 0e6b1c59860e75c04e2e3be2520ed2b39c84ac90
>
>
> Diff: https://reviews.apache.org/r/62592/diff/1/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62592: Added basic HTTP authenticatee
implementation.
Posted by Till Toenshoff <to...@me.com>.
> On Sept. 27, 2017, 9:21 p.m., Greg Mann wrote:
> > src/authentication/http/basic_authenticatee.cpp
> > Lines 41-42 (patched)
> > <https://reviews.apache.org/r/62592/diff/1/?file=1836080#file1836080line41>
> >
> > I think we usually put the `{}` on the same line as the initialization list?
Not me not me :) ... clangformat is to blame :D .... yes, you are right.
> On Sept. 27, 2017, 9:21 p.m., Greg Mann wrote:
> > src/authentication/http/basic_authenticatee.cpp
> > Lines 75-76 (patched)
> > <https://reviews.apache.org/r/62592/diff/1/?file=1836080#file1836080line75>
> >
> > Hmm wouldn't we usually indent two spaces after the linebreak here?
Again clangformat things otherwise - unsure actually bout these cases of a return type already needing a linebreak.
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62592/#review186476
-----------------------------------------------------------
On Sept. 26, 2017, 10:54 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62592/
> -----------------------------------------------------------
>
> (Updated Sept. 26, 2017, 10:54 p.m.)
>
>
> Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
>
>
> Bugs: MESOS-8017
> https://issues.apache.org/jira/browse/MESOS-8017
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added basic HTTP authenticatee implementation.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 1a828c4351ded36f51ccbbe67147da2f50b9cdb1
> src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900
> src/authentication/http/basic_authenticatee.hpp PRE-CREATION
> src/authentication/http/basic_authenticatee.cpp PRE-CREATION
> src/common/http.hpp 0e6b1c59860e75c04e2e3be2520ed2b39c84ac90
>
>
> Diff: https://reviews.apache.org/r/62592/diff/1/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>
Re: Review Request 62592: Added basic HTTP authenticatee
implementation.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/62592/#review186476
-----------------------------------------------------------
src/authentication/http/basic_authenticatee.cpp
Lines 41-42 (patched)
<https://reviews.apache.org/r/62592/#comment263038>
I think we usually put the `{}` on the same line as the initialization list?
src/authentication/http/basic_authenticatee.cpp
Lines 48 (patched)
<https://reviews.apache.org/r/62592/#comment263043>
We use trailing underscores for similar variables within a scope, but the style guide explicitly suggests moving away from this pattern:
"Some trailing underscores are used to distinguish between similar variables in the same scope (think prime symbols), but this should be avoided as much as possible, including removing existing instances in the code base."
Maybe use a new variable name, something like `decoratedRequest` maybe?
src/authentication/http/basic_authenticatee.cpp
Lines 75-76 (patched)
<https://reviews.apache.org/r/62592/#comment263048>
Hmm wouldn't we usually indent two spaces after the linebreak here?
- Greg Mann
On Sept. 26, 2017, 10:54 p.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/62592/
> -----------------------------------------------------------
>
> (Updated Sept. 26, 2017, 10:54 p.m.)
>
>
> Review request for mesos, Armand Grillet, Benjamin Bannier, Greg Mann, and Kapil Arya.
>
>
> Bugs: MESOS-8017
> https://issues.apache.org/jira/browse/MESOS-8017
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Added basic HTTP authenticatee implementation.
>
>
> Diffs
> -----
>
> src/CMakeLists.txt 1a828c4351ded36f51ccbbe67147da2f50b9cdb1
> src/Makefile.am 93ed2bf55447e3e470d9bea8a0b61ce78aad1900
> src/authentication/http/basic_authenticatee.hpp PRE-CREATION
> src/authentication/http/basic_authenticatee.cpp PRE-CREATION
> src/common/http.hpp 0e6b1c59860e75c04e2e3be2520ed2b39c84ac90
>
>
> Diff: https://reviews.apache.org/r/62592/diff/1/
>
>
> Testing
> -------
>
> Integration test in external project. Further tests upcoming.
>
>
> Thanks,
>
> Till Toenshoff
>
>