Re: Review Request 46260: Added a MetricsTest with authentication to libprocess.

2016-04-25 Thread Greg Mann 

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

(Updated April 25, 2016, 8:47 p.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 (updated)
-

  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

Re: Review Request 46260: Added a MetricsTest with authentication to libprocess.

2016-04-25 Thread Kapil Arya 

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


Ship it!




Ship It!

- Kapil Arya


On April 22, 2016, 3:14 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46260/
> ---
> 
> (Updated April 22, 2016, 3:14 p.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
> 
>

Re: Review Request 46260: Added a MetricsTest with authentication to libprocess.

2016-04-22 Thread Greg Mann 

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

(Updated April 22, 2016, 7:14 p.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 (updated)
-

  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

Re: Review Request 46260: Added a MetricsTest with authentication to libprocess.

2016-04-18 Thread Alexander Rojas 


> On April 15, 2016, 1:22 p.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/src/tests/metrics_tests.cpp, line 551
> > 
> >
> > 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
> 
>

Re: Review Request 46260: Added a MetricsTest with authentication to libprocess.

2016-04-15 Thread Greg Mann 


> On April 15, 2016, 11:22 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/src/tests/metrics_tests.cpp, line 551
> > 
> >
> > Can you please add a test where authentication actually works? (for 
> > saftey)

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?


- Greg


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


On April 15, 2016, 6: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, 6: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
> 
>

Re: Review Request 46260: Added a MetricsTest with authentication to libprocess.

2016-04-15 Thread Alexander Rojas 

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


Fix it, then Ship it!





3rdparty/libprocess/src/tests/metrics_tests.cpp (line 551)


Can you please add a test where authentication actually works? (for saftey)


- Alexander Rojas


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