-----------------------------------------------------------
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)
<https://reviews.apache.org/r/59000/#comment248233>

    No need recover().



src/slave/containerizer/mesos/isolators/environment_secret.cpp
Lines 77-82 (patched)
<https://reviews.apache.org/r/59000/#comment248234>

    Ditto.



src/slave/containerizer/mesos/isolators/environment_secret.cpp
Lines 98-101 (patched)
<https://reviews.apache.org/r/59000/#comment248238>

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



src/slave/containerizer/mesos/isolators/environment_secret.cpp
Lines 99-100 (patched)
<https://reviews.apache.org/r/59000/#comment248236>

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



src/slave/containerizer/mesos/isolators/environment_secret.cpp
Lines 103 (patched)
<https://reviews.apache.org/r/59000/#comment248237>

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



src/slave/containerizer/mesos/isolators/environment_secret.cpp
Lines 137-138 (patched)
<https://reviews.apache.org/r/59000/#comment248239>

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



src/tests/containerizer/environment_secret_isolator_tests.cpp
Lines 50 (patched)
<https://reviews.apache.org/r/59000/#comment248240>

    s/FetchSecret/ResolveSecret/g ?



src/tests/containerizer/environment_secret_isolator_tests.cpp
Lines 86-91 (patched)
<https://reviews.apache.org/r/59000/#comment248241>

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



src/tests/containerizer/environment_secret_isolator_tests.cpp
Lines 95-97 (patched)
<https://reviews.apache.org/r/59000/#comment248244>

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



src/tests/containerizer/environment_secret_isolator_tests.cpp
Lines 100-107 (patched)
<https://reviews.apache.org/r/59000/#comment248242>

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