mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rojas <alexan...@mesosphere.io>
Subject Re: Review Request 46260: Added a MetricsTest with authentication to libprocess.
Date Mon, 18 Apr 2016 08:55:30 GMT


> On April 15, 2016, 1:22 p.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/src/tests/metrics_tests.cpp, line 551
> > <https://reviews.apache.org/r/46260/diff/1/?file=1346281#file1346281line551>
> >
> >     Can you please add a test where authentication actually works? (for saftey)
> 
> Greg Mann wrote:
>     I wonder if we really need this? I've written some previous tests along those lines
(which test full endpoint functionality both with and without authentication), but after some
conversations with bmahler recently I'm not sure this is a good idea. Since we don't currently
use the authenticated `principal` at all in the endpoint handler, we really just need to test
here that unauthenticated requests receive an `Unauthorized` response. The `HttpAuthenticationTest.Authenticated`
test in libprocess already tests that a properly authenticated request will succeed in reaching
the HTTP endpoint and passing the correct principal to the handler, so we don't need to test
that again. Once authorization is enabled on this endpoint, it will make sense to have successfully
authenticated test cases in order to test the authorization code, but at this point I think
it would introduce unnecessary tests and increase the code maintenance burden. What do you
think?

Fair enough


- Alexander


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


On April 15, 2016, 8:58 a.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46260/
> -----------------------------------------------------------
> 
> (Updated April 15, 2016, 8:58 a.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-4902
>     https://issues.apache.org/jira/browse/MESOS-4902
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The test `MetricsTest.SnapshotAuthenticationEnabled`
> is added to the libprocess tests in this patch.
> 
> 
> Diffs
> -----
> 
>   3rdparty/libprocess/src/tests/metrics_tests.cpp b84dc8d858f58bc9f52b218b7153510417cf34c2

> 
> Diff: https://reviews.apache.org/r/46260/diff/
> 
> 
> Testing
> -------
> 
> `sudo make check` on OSX.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


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