Re: Review Request 39944: Add prefix option for os::environment.

2015-11-06 Thread haosdent huang 


> On Nov. 5, 2015, 4:40 p.m., Timothy Chen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp, line 34
> > 
> >
> > Where do we need to use this now?
> > I rather not add regex if we never end up using this.

But do we need add manually and keep the sync for every new environment 
variable?


- haosdent


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


On Nov. 5, 2015, 8:25 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39944/
> ---
> 
> (Updated Nov. 5, 2015, 8:25 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add prefix option for os::environment.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp 
> 91d82a8fae27c002458cad0bbdc45b312d2ec70d 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> 5a1da57f7e27cf8154f0d5f6efd47dcee8a430ff 
> 
> Diff: https://reviews.apache.org/r/39944/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39944: Add prefix option for os::environment.

2015-11-06 Thread Timothy Chen 

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



3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp (line 34)


I'm not opposed to include this, I just hope we include some context why we 
need this.

I'm assuming you want to fix the problem of environment variables with 
docker containers around SSL_*.

I think as you mentinoed, having to keep up with new ones we add is a pain, 
but at the same time accidentially including environment variables I believe 
causing more headaches since SSL_ or other regex can potentially match things 
that production systems don't want to be including.


- Timothy Chen


On Nov. 5, 2015, 8:25 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39944/
> ---
> 
> (Updated Nov. 5, 2015, 8:25 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add prefix option for os::environment.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp 
> 91d82a8fae27c002458cad0bbdc45b312d2ec70d 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> 5a1da57f7e27cf8154f0d5f6efd47dcee8a430ff 
> 
> Diff: https://reviews.apache.org/r/39944/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39944: Add prefix option for os::environment.

2015-11-06 Thread haosdent huang 


> On Nov. 6, 2015, 4:22 p.m., Timothy Chen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp, line 34
> > 
> >
> > I'm not opposed to include this, I just hope we include some context 
> > why we need this.
> > 
> > I'm assuming you want to fix the problem of environment variables with 
> > docker containers around SSL_*.
> > 
> > I think as you mentinoed, having to keep up with new ones we add is a 
> > pain, but at the same time accidentially including environment variables I 
> > believe causing more headaches since SSL_ or other regex can potentially 
> > match things that production systems don't want to be including.

hmm, let me discard this and create a patch which excalty include environment 
variables.


- haosdent


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


On Nov. 5, 2015, 8:25 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39944/
> ---
> 
> (Updated Nov. 5, 2015, 8:25 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add prefix option for os::environment.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp 
> 91d82a8fae27c002458cad0bbdc45b312d2ec70d 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> 5a1da57f7e27cf8154f0d5f6efd47dcee8a430ff 
> 
> Diff: https://reviews.apache.org/r/39944/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39944: Add prefix option for os::environment.

2015-11-05 Thread haosdent huang 

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

(Updated Nov. 5, 2015, 8:25 a.m.)


Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.


Changes
---

Fix error in gcc 4.8


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


Repository: mesos


Description
---

Add prefix option for os::environment.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp 
91d82a8fae27c002458cad0bbdc45b312d2ec70d 
  3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
5a1da57f7e27cf8154f0d5f6efd47dcee8a430ff 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 39944: Add prefix option for os::environment.

2015-11-05 Thread Timothy Chen 

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



3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp (line 34)


Where do we need to use this now?
I rather not add regex if we never end up using this.


- Timothy Chen


On Nov. 5, 2015, 8:25 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39944/
> ---
> 
> (Updated Nov. 5, 2015, 8:25 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add prefix option for os::environment.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp 
> 91d82a8fae27c002458cad0bbdc45b312d2ec70d 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> 5a1da57f7e27cf8154f0d5f6efd47dcee8a430ff 
> 
> Diff: https://reviews.apache.org/r/39944/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39944: Add prefix option for os::environment.

2015-11-04 Thread haosdent huang 


> On Nov. 4, 2015, 6:28 p.m., Jojy Varghese wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp, line 25
> > 
> >
> > Do you intend to copy the string each time? Or a const string& will do?
> > 
> > As mentioned in the other review, maybe passing a set of prefixes or 
> > regular expressions would be useful.

Got it, let me update.


- haosdent


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


On Nov. 4, 2015, 6:02 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39944/
> ---
> 
> (Updated Nov. 4, 2015, 6:02 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add prefix option for os::environment.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp 
> 91d82a8fae27c002458cad0bbdc45b312d2ec70d 
> 
> Diff: https://reviews.apache.org/r/39944/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39944: Add prefix option for os::environment.

2015-11-04 Thread haosdent huang 

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

(Updated Nov. 5, 2015, 3:18 a.m.)


Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.


Changes
---

Update according @jojy comments


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


Repository: mesos


Description
---

Add prefix option for os::environment.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp 
91d82a8fae27c002458cad0bbdc45b312d2ec70d 
  3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
5a1da57f7e27cf8154f0d5f6efd47dcee8a430ff 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 39944: Add prefix option for os::environment.

2015-11-04 Thread haosdent huang 


> On Nov. 4, 2015, 6:58 p.m., Timothy Chen wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp, line 25
> > 
> >
> > Why is this needed?
> > And also we prefer Option rather than an empty string default.

Thank you. I replace with Option


- haosdent


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


On Nov. 5, 2015, 3:18 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39944/
> ---
> 
> (Updated Nov. 5, 2015, 3:18 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add prefix option for os::environment.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp 
> 91d82a8fae27c002458cad0bbdc45b312d2ec70d 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> 5a1da57f7e27cf8154f0d5f6efd47dcee8a430ff 
> 
> Diff: https://reviews.apache.org/r/39944/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39944: Add prefix option for os::environment.

2015-11-04 Thread Timothy Chen 

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



3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp (line 25)


Why is this needed?
And also we prefer Option rather than an empty string default.


- Timothy Chen


On Nov. 4, 2015, 6:02 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39944/
> ---
> 
> (Updated Nov. 4, 2015, 6:02 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add prefix option for os::environment.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp 
> 91d82a8fae27c002458cad0bbdc45b312d2ec70d 
> 
> Diff: https://reviews.apache.org/r/39944/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39944: Add prefix option for os::environment.

2015-11-04 Thread Jojy Varghese 

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


I would suggest you add tests for this.

- Jojy Varghese


On Nov. 4, 2015, 6:02 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39944/
> ---
> 
> (Updated Nov. 4, 2015, 6:02 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add prefix option for os::environment.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp 
> 91d82a8fae27c002458cad0bbdc45b312d2ec70d 
> 
> Diff: https://reviews.apache.org/r/39944/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39944: Add prefix option for os::environment.

2015-11-04 Thread Jojy Varghese 

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



3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp (line 40)


I thought the intent here was to check for prefix and not anywhere in the 
string.


- Jojy Varghese


On Nov. 4, 2015, 6:02 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39944/
> ---
> 
> (Updated Nov. 4, 2015, 6:02 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add prefix option for os::environment.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp 
> 91d82a8fae27c002458cad0bbdc45b312d2ec70d 
> 
> Diff: https://reviews.apache.org/r/39944/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 39944: Add prefix option for os::environment.

2015-11-04 Thread Jojy Varghese 

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



3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp (line 25)


Do you intend to copy the string each time? Or a const string& will do?

As mentioned in the other review, maybe passing a set of prefixes or 
regular expressions would be useful.


- Jojy Varghese


On Nov. 4, 2015, 6:02 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/39944/
> ---
> 
> (Updated Nov. 4, 2015, 6:02 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jojy Varghese, and Timothy Chen.
> 
> 
> Bugs: MESOS-3815
> https://issues.apache.org/jira/browse/MESOS-3815
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add prefix option for os::environment.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/environment.hpp 
> 91d82a8fae27c002458cad0bbdc45b312d2ec70d 
> 
> Diff: https://reviews.apache.org/r/39944/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>