Re: Review Request 59000: Added environment secret isolator.

[Kapil Arya]

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

(Updated May 22, 2017, 4:54 p.m.)


Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, and Vinod Kone.


Changes
---

Addressed Vinod's comment


Repository: mesos


Description
---

Added environment secret isolator.


Diffs (updated)
-

  src/CMakeLists.txt a038c0b919cb7ac46c99b37bcfb839217b4ab9b4 
  src/Makefile.am 3e71393c4a8e50c9c6f703bbef7f7bdc6a53e6ee 
  src/launcher/executor.cpp 9971a67d75b33002127fd1decad59371fdf1e719 
  src/slave/containerizer/mesos/containerizer.cpp 
50a63b58b4960729316b0b5685793ce18ee5ce93 
  src/slave/containerizer/mesos/isolators/environment_secret.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/environment_secret.cpp PRE-CREATION 
  src/tests/CMakeLists.txt 9f2af9cdd1cf50485f4cd84ce67bcceba64b9328 
  src/tests/containerizer/environment_secret_isolator_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/59000/diff/8/

Changes: https://reviews.apache.org/r/59000/diff/7-8/


Testing
---

Added a new test and ran `make check`.


Thanks,

Kapil Arya

Re: Review Request 59000: Added environment secret isolator.

[Vinod Kone]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/environment_secret.cpp
Lines 93-108 (original), 86-110 (patched)


can't we just use `common::validation::validateEnvironment()`?


- Vinod Kone


On May 16, 2017, 7:22 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59000/
> ---
> 
> (Updated May 16, 2017, 7:22 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added environment secret isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt eef718d95b5d8e051a5094369dc9b4532bc307ff 
>   src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a 
>   src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 97837c83cc223950750e4cd088f4da067023c96c 
>   src/slave/containerizer/mesos/isolators/environment_secret.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/environment_secret.cpp PRE-CREATION 
>   src/tests/CMakeLists.txt 9f2af9cdd1cf50485f4cd84ce67bcceba64b9328 
>   src/tests/containerizer/environment_secret_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/59000/diff/7/
> 
> 
> Testing
> ---
> 
> Added a new test and ran `make check`.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 59000: Added environment secret isolator.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On May 16, 2017, 12:22 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59000/
> ---
> 
> (Updated May 16, 2017, 12:22 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added environment secret isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt eef718d95b5d8e051a5094369dc9b4532bc307ff 
>   src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a 
>   src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 97837c83cc223950750e4cd088f4da067023c96c 
>   src/slave/containerizer/mesos/isolators/environment_secret.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/environment_secret.cpp PRE-CREATION 
>   src/tests/CMakeLists.txt 9f2af9cdd1cf50485f4cd84ce67bcceba64b9328 
>   src/tests/containerizer/environment_secret_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/59000/diff/7/
> 
> 
> Testing
> ---
> 
> Added a new test and ran `make check`.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 59000: Added environment secret isolator.

[Gilbert Song]

> On May 15, 2017, 6:42 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/environment_secret.cpp
> > Lines 137-138 (patched)
> > 
> >
> > How about custom executor/default executor? `task_environment` is for 
> > command task specific.
> 
> Kapil Arya wrote:
> We already set the `launchInfo.environment`; are you suggesting to not 
> set task_environment?

ah, I see.


- Gilbert


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


On May 16, 2017, 12:22 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59000/
> ---
> 
> (Updated May 16, 2017, 12:22 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added environment secret isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt eef718d95b5d8e051a5094369dc9b4532bc307ff 
>   src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a 
>   src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 97837c83cc223950750e4cd088f4da067023c96c 
>   src/slave/containerizer/mesos/isolators/environment_secret.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/environment_secret.cpp PRE-CREATION 
>   src/tests/CMakeLists.txt 9f2af9cdd1cf50485f4cd84ce67bcceba64b9328 
>   src/tests/containerizer/environment_secret_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/59000/diff/7/
> 
> 
> Testing
> ---
> 
> Added a new test and ran `make check`.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 59000: Added environment secret isolator.

[Kapil Arya]

> On May 15, 2017, 9:42 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/isolators/environment_secret.cpp
> > Lines 137-138 (patched)
> > 
> >
> > How about custom executor/default executor? `task_environment` is for 
> > command task specific.

We already set the `launchInfo.environment`; are you suggesting to not set 
task_environment?


- Kapil


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


On May 16, 2017, 3:22 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59000/
> ---
> 
> (Updated May 16, 2017, 3:22 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added environment secret isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt eef718d95b5d8e051a5094369dc9b4532bc307ff 
>   src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a 
>   src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 97837c83cc223950750e4cd088f4da067023c96c 
>   src/slave/containerizer/mesos/isolators/environment_secret.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/environment_secret.cpp PRE-CREATION 
>   src/tests/CMakeLists.txt 9f2af9cdd1cf50485f4cd84ce67bcceba64b9328 
>   src/tests/containerizer/environment_secret_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/59000/diff/7/
> 
> 
> Testing
> ---
> 
> Added a new test and ran `make check`.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 59000: Added environment secret isolator.

[Kapil Arya]

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

(Updated May 16, 2017, 3:22 p.m.)


Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, and Vinod Kone.


Changes
---

Addressed Gilbert's comments.


Repository: mesos


Description
---

Added environment secret isolator.


Diffs (updated)
-

  src/CMakeLists.txt eef718d95b5d8e051a5094369dc9b4532bc307ff 
  src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a 
  src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
  src/slave/containerizer/mesos/containerizer.cpp 
97837c83cc223950750e4cd088f4da067023c96c 
  src/slave/containerizer/mesos/isolators/environment_secret.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/environment_secret.cpp PRE-CREATION 
  src/tests/CMakeLists.txt 9f2af9cdd1cf50485f4cd84ce67bcceba64b9328 
  src/tests/containerizer/environment_secret_isolator_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/59000/diff/7/

Changes: https://reviews.apache.org/r/59000/diff/6-7/


Testing
---

Added a new test and ran `make check`.


Thanks,

Kapil Arya

Re: Review Request 59000: Added environment secret isolator.

[Gilbert Song]

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




src/slave/containerizer/mesos/isolators/environment_secret.hpp
Lines 43-45 (patched)


No need recover().



src/slave/containerizer/mesos/isolators/environment_secret.cpp
Lines 77-82 (patched)


Ditto.



src/slave/containerizer/mesos/isolators/environment_secret.cpp
Lines 98-101 (patched)


I would move this part above `secretResolver->resolve()`.



src/slave/containerizer/mesos/isolators/environment_secret.cpp
Lines 99-100 (patched)


Let's log the `Environment::Variable::name`.



src/slave/containerizer/mesos/isolators/environment_secret.cpp
Lines 103 (patched)


this is a bug? should check `variable.has_secret()` first?



src/slave/containerizer/mesos/isolators/environment_secret.cpp
Lines 137-138 (patched)


How about custom executor/default executor? `task_environment` is for 
command task specific.



src/tests/containerizer/environment_secret_isolator_tests.cpp
Lines 50 (patched)


s/FetchSecret/ResolveSecret/g ?



src/tests/containerizer/environment_secret_isolator_tests.cpp
Lines 86-91 (patched)


use the helper from test/mesos.hpp `createTask()`.



src/tests/containerizer/environment_secret_isolator_tests.cpp
Lines 95-97 (patched)


Could we use of `strings::format()` to make this part more readable?



src/tests/containerizer/environment_secret_isolator_tests.cpp
Lines 100-107 (patched)


Let's add a `TODO` here to refactor the helper  `createEnvironment()` to 
support value based/secret based env var respectively.


- Gilbert Song


On May 12, 2017, 10:52 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59000/
> ---
> 
> (Updated May 12, 2017, 10:52 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added environment secret isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 40d921ee7026f5ac47efbf0243e1cafab57825f9 
>   src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a 
>   src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 97837c83cc223950750e4cd088f4da067023c96c 
>   src/slave/containerizer/mesos/isolators/environment_secret.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/environment_secret.cpp PRE-CREATION 
>   src/tests/CMakeLists.txt 9f2af9cdd1cf50485f4cd84ce67bcceba64b9328 
>   src/tests/containerizer/environment_secret_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/59000/diff/6/
> 
> 
> Testing
> ---
> 
> Added a new test and ran `make check`.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 59000: Added environment secret isolator.

[Kapil Arya]

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

(Updated May 12, 2017, 1:52 p.m.)


Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, and Vinod Kone.


Changes
---

Rebased


Repository: mesos


Description
---

Added environment secret isolator.


Diffs (updated)
-

  src/CMakeLists.txt 40d921ee7026f5ac47efbf0243e1cafab57825f9 
  src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a 
  src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
  src/slave/containerizer/mesos/containerizer.cpp 
97837c83cc223950750e4cd088f4da067023c96c 
  src/slave/containerizer/mesos/isolators/environment_secret.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/environment_secret.cpp PRE-CREATION 
  src/tests/CMakeLists.txt 9f2af9cdd1cf50485f4cd84ce67bcceba64b9328 
  src/tests/containerizer/environment_secret_isolator_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/59000/diff/6/

Changes: https://reviews.apache.org/r/59000/diff/5-6/


Testing
---

Added a new test and ran `make check`.


Thanks,

Kapil Arya

Re: Review Request 59000: Added environment secret isolator.

[Kapil Arya]

> On May 8, 2017, 8:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 226 (patched)
> > 
> >
> > should this be "environment/secret" instead? i'm assuming there might 
> > be other env based isolators in the future.

Jie suggested that we keep it at the top-level. This is also an special case 
where this isolator is not visible outside and is turned on by default.


- Kapil


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


On May 9, 2017, 2:11 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59000/
> ---
> 
> (Updated May 9, 2017, 2:11 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added environment secret isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 40d921ee7026f5ac47efbf0243e1cafab57825f9 
>   src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a 
>   src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 58ab74571fb14c6dbb1907151dc421f93e324bb5 
>   src/slave/containerizer/mesos/isolators/environment_secret.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/environment_secret.cpp PRE-CREATION 
>   src/tests/CMakeLists.txt 9f2af9cdd1cf50485f4cd84ce67bcceba64b9328 
>   src/tests/containerizer/environment_secret_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/59000/diff/5/
> 
> 
> Testing
> ---
> 
> Added a new test and ran `make check`.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 59000: Added environment secret isolator.

[Kapil Arya]

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

(Updated May 9, 2017, 2:11 p.m.)


Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, and Vinod Kone.


Changes
---

Addressed Vinod's comments!


Repository: mesos


Description
---

Added environment secret isolator.


Diffs (updated)
-

  src/CMakeLists.txt 40d921ee7026f5ac47efbf0243e1cafab57825f9 
  src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a 
  src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
  src/slave/containerizer/mesos/containerizer.cpp 
58ab74571fb14c6dbb1907151dc421f93e324bb5 
  src/slave/containerizer/mesos/isolators/environment_secret.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/environment_secret.cpp PRE-CREATION 
  src/tests/CMakeLists.txt 9f2af9cdd1cf50485f4cd84ce67bcceba64b9328 
  src/tests/containerizer/environment_secret_isolator_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/59000/diff/5/

Changes: https://reviews.apache.org/r/59000/diff/4-5/


Testing
---

Added a new test and ran `make check`.


Thanks,

Kapil Arya

Re: Review Request 59000: Added environment secret isolator.

[Vinod Kone]

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




src/slave/containerizer/mesos/containerizer.cpp
Lines 226 (patched)


should this be "environment/secret" instead? i'm assuming there might be 
other env based isolators in the future.



src/slave/containerizer/mesos/containerizer.cpp
Lines 1460 (patched)


s/have been/should have been/



src/slave/containerizer/mesos/isolators/environment_secret.cpp
Lines 98 (patched)


s/environment/Enviornment/

log the variable name?



src/slave/containerizer/mesos/isolators/environment_secret.cpp
Lines 107 (patched)


log variable name.



src/slave/containerizer/mesos/isolators/environment_secret.cpp
Lines 122 (patched)


can you use `collect` instead?



src/tests/containerizer/environment_secret_isolator_tests.cpp
Lines 52 (patched)


kill the comment.



src/tests/containerizer/environment_secret_isolator_tests.cpp
Lines 76 (patched)


no need for `Times(1)`


- Vinod Kone


On May 8, 2017, 10:19 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59000/
> ---
> 
> (Updated May 8, 2017, 10:19 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added environment secret isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 40d921ee7026f5ac47efbf0243e1cafab57825f9 
>   src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a 
>   src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 58ab74571fb14c6dbb1907151dc421f93e324bb5 
>   src/slave/containerizer/mesos/isolators/environment_secret.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/environment_secret.cpp PRE-CREATION 
>   src/tests/CMakeLists.txt 9f2af9cdd1cf50485f4cd84ce67bcceba64b9328 
>   src/tests/containerizer/environment_secret_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/59000/diff/4/
> 
> 
> Testing
> ---
> 
> Added a new test and ran `make check`.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 59000: Added environment secret isolator.

[Kapil Arya]

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

(Updated May 8, 2017, 6:19 p.m.)


Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, and Vinod Kone.


Changes
---

Rebased


Repository: mesos


Description
---

Added environment secret isolator.


Diffs (updated)
-

  src/CMakeLists.txt 40d921ee7026f5ac47efbf0243e1cafab57825f9 
  src/Makefile.am 6bb81fd49b4564a0afa993b2cef6baa9d370ee7a 
  src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
  src/slave/containerizer/mesos/containerizer.cpp 
58ab74571fb14c6dbb1907151dc421f93e324bb5 
  src/slave/containerizer/mesos/isolators/environment_secret.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/environment_secret.cpp PRE-CREATION 
  src/tests/CMakeLists.txt 9f2af9cdd1cf50485f4cd84ce67bcceba64b9328 
  src/tests/containerizer/environment_secret_isolator_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/59000/diff/4/

Changes: https://reviews.apache.org/r/59000/diff/3-4/


Testing
---

Added a new test and ran `make check`.


Thanks,

Kapil Arya

Re: Review Request 59000: Added environment secret isolator.

[Kapil Arya]

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

(Updated May 5, 2017, 8:33 p.m.)


Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, and Vinod Kone.


Changes
---

Added cmake changes


Repository: mesos


Description
---

Added environment secret isolator.


Diffs (updated)
-

  src/CMakeLists.txt 89cbd3f5a93f4891e8272d3b1136059ab1069d01 
  src/Makefile.am 29da17bee13226e18757e2ad3a7a091427fd35f4 
  src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 
  src/slave/containerizer/mesos/isolators/environment_secret.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/environment_secret.cpp PRE-CREATION 
  src/tests/CMakeLists.txt 9f2af9cdd1cf50485f4cd84ce67bcceba64b9328 
  src/tests/containerizer/environment_secret_isolator_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/59000/diff/3/

Changes: https://reviews.apache.org/r/59000/diff/2-3/


Testing
---

Added a new test and ran `make check`.


Thanks,

Kapil Arya

Re: Review Request 59000: Added environment secret isolator.

[Kapil Arya]

> On May 4, 2017, 6:32 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/environment_secret.cpp
> > Lines 37-44 (patched)
> > 
> >
> > Why you need this? Can you make sure to avoid such small nits while 
> > copying pasting code? Please remove all unncessary headers, and do a sweep 
> > for other reviews as well!

Sorry for the noise - that was really stupid copy-paste. Fixed now.


- Kapil


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


On May 5, 2017, 6:43 a.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59000/
> ---
> 
> (Updated May 5, 2017, 6:43 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added environment secret isolator.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 29da17bee13226e18757e2ad3a7a091427fd35f4 
>   src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/slave/containerizer/mesos/isolators/environment_secret.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/environment_secret.cpp PRE-CREATION 
>   src/tests/containerizer/environment_secret_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/59000/diff/2/
> 
> 
> Testing
> ---
> 
> Added a new test and ran `make check`.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 59000: Added environment secret isolator.

[Kapil Arya]

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

(Updated May 5, 2017, 6:43 a.m.)


Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, and Vinod Kone.


Changes
---

Rebased.


Repository: mesos


Description (updated)
---

Added environment secret isolator.


Diffs (updated)
-

  src/Makefile.am 29da17bee13226e18757e2ad3a7a091427fd35f4 
  src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
  src/slave/containerizer/mesos/containerizer.cpp 
b58baed64480e22f640a4852537f85922ed382ae 
  src/slave/containerizer/mesos/isolators/environment_secret.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/environment_secret.cpp PRE-CREATION 
  src/tests/containerizer/environment_secret_isolator_tests.cpp PRE-CREATION 


Diff: https://reviews.apache.org/r/59000/diff/2/

Changes: https://reviews.apache.org/r/59000/diff/1-2/


Testing
---

Added a new test and ran `make check`.


Thanks,

Kapil Arya

Re: Review Request 59000: Added environment secret isolator.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/environment_secret.cpp
Lines 37-44 (patched)


Why you need this? Can you make sure to avoid such small nits while copying 
pasting code? Please remove all unncessary headers, and do a sweep for other 
reviews as well!



src/slave/containerizer/mesos/isolators/environment_secret.cpp
Lines 57-58 (patched)


Do you use those in this file? Ditto my other comments.



src/slave/containerizer/mesos/isolators/environment_secret.cpp
Lines 100 (patched)


Please remove std:: prefix here. Please do a sweep to fix all such issues 
in other reviews!


- Jie Yu


On May 4, 2017, 8:08 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59000/
> ---
> 
> (Updated May 4, 2017, 8:08 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This isolator resolves Secret-type environment variables using a provided 
> SecretFetcher module.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 29da17bee13226e18757e2ad3a7a091427fd35f4 
>   src/launcher/executor.cpp b05f73e539c22d4d40f07df76168a06373b818d4 
>   src/slave/containerizer/mesos/containerizer.cpp 
> b58baed64480e22f640a4852537f85922ed382ae 
>   src/slave/containerizer/mesos/isolators/environment_secret.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/environment_secret.cpp PRE-CREATION 
>   src/tests/containerizer/environment_secret_isolator_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/59000/diff/1/
> 
> 
> Testing
> ---
> 
> Added a new test and ran `make check`.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>