Re: Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Greg Mann]

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

(Updated March 20, 2016, 4:48 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Addressed comments.


Bugs: MESOS-4849
https://issues.apache.org/jira/browse/MESOS-4849


Repository: mesos


Description
---

Added Doxygen docs for basic HTTP authenticator.

The structure of the module parameters for the basic HTTP authenticator has 
changed, so Doxygen comments were added to the module's header file to provide 
an example.


Diffs (updated)
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 

Diff: https://reviews.apache.org/r/44703/diff/


Testing
---


Thanks,

Greg Mann

Re: Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Greg Mann]

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

(Updated March 18, 2016, 6:13 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Addressed comments.


Bugs: MESOS-4849
https://issues.apache.org/jira/browse/MESOS-4849


Repository: mesos


Description
---

Added Doxygen docs for basic HTTP authenticator.

The structure of the module parameters for the basic HTTP authenticator has 
changed, so Doxygen comments were added to the module's header file to provide 
an example.


Diffs (updated)
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 

Diff: https://reviews.apache.org/r/44703/diff/


Testing
---


Thanks,

Greg Mann

Re: Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Adam B]

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



Please be careful how you use "should" vs. "must" vs. "may". See 
http://tools.ietf.org/html/rfc2119


include/mesos/authentication/http/basic_authenticator_factory.hpp (line 40)


s/should/must/? Which ones are required vs. optional?



include/mesos/authentication/http/basic_authenticator_factory.hpp (line 42)


s/should/must/?



include/mesos/authentication/http/basic_authenticator_factory.hpp (lines 56 - 
57)


Why are these quotes escaped, but the others aren't? Because they're inside 
a string "value"? Seems odd.



include/mesos/authentication/http/basic_authenticator_factory.hpp (line 64)


s/should/must/ or s/should/may/?



include/mesos/authentication/http/basic_authenticator_factory.hpp (line 66)


s/which contains/containing/
s/the authenticator/a new authenticator/


- Adam B


On March 17, 2016, 12:40 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44703/
> ---
> 
> (Updated March 17, 2016, 12:40 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4849
> https://issues.apache.org/jira/browse/MESOS-4849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Doxygen docs for basic HTTP authenticator.
> 
> The structure of the module parameters for the basic HTTP authenticator has 
> changed, so Doxygen comments were added to the module's header file to 
> provide an example.
> 
> 
> Diffs
> -
> 
>   include/mesos/authentication/http/basic_authenticator_factory.hpp 
> c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 
> 
> Diff: https://reviews.apache.org/r/44703/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Greg Mann]

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

(Updated March 17, 2016, 7:40 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Rebase.


Bugs: MESOS-4849
https://issues.apache.org/jira/browse/MESOS-4849


Repository: mesos


Description
---

Added Doxygen docs for basic HTTP authenticator.

The structure of the module parameters for the basic HTTP authenticator has 
changed, so Doxygen comments were added to the module's header file to provide 
an example.


Diffs (updated)
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 

Diff: https://reviews.apache.org/r/44703/diff/


Testing
---


Thanks,

Greg Mann

Re: Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Joerg Schad]

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


Ship it!




- Joerg Schad


On March 18, 2016, 6:13 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44703/
> ---
> 
> (Updated March 18, 2016, 6:13 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4849
> https://issues.apache.org/jira/browse/MESOS-4849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Doxygen docs for basic HTTP authenticator.
> 
> The structure of the module parameters for the basic HTTP authenticator has 
> changed, so Doxygen comments were added to the module's header file to 
> provide an example.
> 
> 
> Diffs
> -
> 
>   include/mesos/authentication/http/basic_authenticator_factory.hpp 
> c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 
> 
> Diff: https://reviews.apache.org/r/44703/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Adam B]

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


Fix it, then Ship it!




Unnecessary summaries, but shippable once we remove those.


include/mesos/authentication/http/basic_authenticator_factory.hpp (lines 38 - 
40)


No need for the summary line above. Our doxygen config will auto-create a 
summary from the first sentence in the description, which says essentially the 
same thing.



include/mesos/authentication/http/basic_authenticator_factory.hpp (lines 74 - 
75)


Remove



include/mesos/authentication/http/basic_authenticator_factory.hpp (lines 90 - 
91)


Remove


- Adam B


On March 18, 2016, 11:13 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44703/
> ---
> 
> (Updated March 18, 2016, 11:13 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4849
> https://issues.apache.org/jira/browse/MESOS-4849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Doxygen docs for basic HTTP authenticator.
> 
> The structure of the module parameters for the basic HTTP authenticator has 
> changed, so Doxygen comments were added to the module's header file to 
> provide an example.
> 
> 
> Diffs
> -
> 
>   include/mesos/authentication/http/basic_authenticator_factory.hpp 
> c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 
> 
> Diff: https://reviews.apache.org/r/44703/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Greg Mann]

> On March 18, 2016, 9:07 a.m., Adam B wrote:
> > include/mesos/authentication/http/basic_authenticator_factory.hpp, lines 
> > 56-57
> > 
> >
> > Why are these quotes escaped, but the others aren't? Because they're 
> > inside a string "value"? Seems odd.

Yep, it's because they're inside this JSON string. It's a bit messy, but 
necessary if the credentials are going to be specified as JSON.


- Greg


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


On March 18, 2016, 6:13 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44703/
> ---
> 
> (Updated March 18, 2016, 6:13 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4849
> https://issues.apache.org/jira/browse/MESOS-4849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Doxygen docs for basic HTTP authenticator.
> 
> The structure of the module parameters for the basic HTTP authenticator has 
> changed, so Doxygen comments were added to the module's header file to 
> provide an example.
> 
> 
> Diffs
> -
> 
>   include/mesos/authentication/http/basic_authenticator_factory.hpp 
> c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 
> 
> Diff: https://reviews.apache.org/r/44703/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Greg Mann]

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

(Updated March 14, 2016, 6:16 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Addressed comments.


Bugs: MESOS-4849
https://issues.apache.org/jira/browse/MESOS-4849


Repository: mesos


Description
---

Added Doxygen docs for basic HTTP authenticator.

The structure of the module parameters for the basic HTTP authenticator has 
changed, so Doxygen comments were added to the module's header file to provide 
an example.


Diffs (updated)
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 

Diff: https://reviews.apache.org/r/44703/diff/


Testing
---


Thanks,

Greg Mann

Re: Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Adam B]

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




include/mesos/authentication/http/basic_authenticator_factory.hpp (line 64)


This is a useless @param comment. I'd think some of the content from above 
would be more appropriate in this @param description.



include/mesos/authentication/http/basic_authenticator_factory.hpp (line 77)


No @param for `credentials`?


- Adam B


On March 13, 2016, 9:17 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44703/
> ---
> 
> (Updated March 13, 2016, 9:17 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4849
> https://issues.apache.org/jira/browse/MESOS-4849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Doxygen docs for basic HTTP authenticator.
> 
> The structure of the module parameters for the basic HTTP authenticator has 
> changed, so Doxygen comments were added to the module's header file to 
> provide an example.
> 
> 
> Diffs
> -
> 
>   include/mesos/authentication/http/basic_authenticator_factory.hpp 
> c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 
> 
> Diff: https://reviews.apache.org/r/44703/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Greg Mann]

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

(Updated March 14, 2016, 4:17 a.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Rebase.


Bugs: MESOS-4849
https://issues.apache.org/jira/browse/MESOS-4849


Repository: mesos


Description
---

Added Doxygen docs for basic HTTP authenticator.

The structure of the module parameters for the basic HTTP authenticator has 
changed, so Doxygen comments were added to the module's header file to provide 
an example.


Diffs (updated)
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 

Diff: https://reviews.apache.org/r/44703/diff/


Testing
---


Thanks,

Greg Mann

Re: Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Greg Mann]

> On March 11, 2016, 4:22 p.m., Alexander Rojas wrote:
> > include/mesos/authentication/http/basic_authenticator_factory.hpp, lines 
> > 54-61
> > 
> >
> > s/"value:"/"value":/
> > 
> > also, the contents of value are free strings.

Ah yea, since the value has to be a string it looks a bit messy with a string 
representing a JSON array as its value. I think I still like the current 
format, since we'll be deprecating the space-delimited format. I changed the 
comments to include the JSON array in a string representation.


- Greg


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


On March 11, 2016, 4:41 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44703/
> ---
> 
> (Updated March 11, 2016, 4:41 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4849
> https://issues.apache.org/jira/browse/MESOS-4849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Doxygen docs for basic HTTP authenticator.
> 
> The structure of the module parameters for the basic HTTP authenticator has 
> changed, so Doxygen comments were added to the module's header file to 
> provide an example.
> 
> 
> Diffs
> -
> 
>   include/mesos/authentication/http/basic_authenticator_factory.hpp 
> c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 
> 
> Diff: https://reviews.apache.org/r/44703/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Greg Mann]

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

(Updated March 11, 2016, 4:41 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Addressed comment.


Bugs: MESOS-4849
https://issues.apache.org/jira/browse/MESOS-4849


Repository: mesos


Description
---

Added Doxygen docs for basic HTTP authenticator.

The structure of the module parameters for the basic HTTP authenticator has 
changed, so Doxygen comments were added to the module's header file to provide 
an example.


Diffs (updated)
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 

Diff: https://reviews.apache.org/r/44703/diff/


Testing
---


Thanks,

Greg Mann

Re: Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Alexander Rojas]

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




include/mesos/authentication/http/basic_authenticator_factory.hpp (lines 54 - 
61)


s/"value:"/"value":/

also, the contents of value are free strings.


- Alexander Rojas


On March 11, 2016, 5:04 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44703/
> ---
> 
> (Updated March 11, 2016, 5:04 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4849
> https://issues.apache.org/jira/browse/MESOS-4849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Doxygen docs for basic HTTP authenticator.
> 
> The structure of the module parameters for the basic HTTP authenticator has 
> changed, so Doxygen comments were added to the module's header file to 
> provide an example.
> 
> 
> Diffs
> -
> 
>   include/mesos/authentication/http/basic_authenticator_factory.hpp 
> c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 
> 
> Diff: https://reviews.apache.org/r/44703/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Greg Mann]

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

(Updated March 11, 2016, 4:04 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Addressed comment.


Bugs: MESOS-4849
https://issues.apache.org/jira/browse/MESOS-4849


Repository: mesos


Description
---

Added Doxygen docs for basic HTTP authenticator.

The structure of the module parameters for the basic HTTP authenticator has 
changed, so Doxygen comments were added to the module's header file to provide 
an example.


Diffs (updated)
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 

Diff: https://reviews.apache.org/r/44703/diff/


Testing
---


Thanks,

Greg Mann

Re: Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Alexander Rojas]

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




include/mesos/authentication/http/basic_authenticator_factory.hpp (lines 46 - 
54)


I think this might be a bit confusing because this json is not a valid 
parameter value, while one gains in the sense that it reflects the intention, I 
can imagine people using this as an example, feeding it into the `--modules` 
flag, and then failing. So I rather use the actual json users have to write 
even if it looks worse.


- Alexander Rojas


On March 11, 2016, 10:47 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44703/
> ---
> 
> (Updated March 11, 2016, 10:47 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4849
> https://issues.apache.org/jira/browse/MESOS-4849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Doxygen docs for basic HTTP authenticator.
> 
> The structure of the module parameters for the basic HTTP authenticator has 
> changed, so Doxygen comments were added to the module's header file to 
> provide an example.
> 
> 
> Diffs
> -
> 
>   include/mesos/authentication/http/basic_authenticator_factory.hpp 
> c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 
> 
> Diff: https://reviews.apache.org/r/44703/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Greg Mann]

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

Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Bugs: MESOS-4849
https://issues.apache.org/jira/browse/MESOS-4849


Repository: mesos


Description
---

Added Doxygen docs for basic HTTP authenticator.

The structure of the module parameters for the basic HTTP authenticator has 
changed, so Doxygen comments were added to the module's header file to provide 
an example.


Diffs
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 

Diff: https://reviews.apache.org/r/44703/diff/


Testing
---


Thanks,

Greg Mann