You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Greg Mann <gr...@mesosphere.io> on 2017/01/25 20:10:31 UTC
Review Request 55954: Changed 'Environment.Variable.Value' from
required to optional.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55954/
-----------------------------------------------------------
Review request for mesos, Jan Schlicht and Vinod Kone.
Bugs: MESOS-6991
https://issues.apache.org/jira/browse/MESOS-6991
Repository: mesos
Description
-------
To prepare for future work which will enable the
modular fetching of secrets, we should change the
Environment.Variable.Value field from required to
optional. This way, the field can be left empty
and filled in by a secret fetching module.
Diffs
-----
include/mesos/mesos.proto 8f14444d6957a97eff1e0a94817d38e7a7de6d69
include/mesos/v1/mesos.proto 74e7851b147ab821dceeab6e838d34b092f101c3
src/common/validation.hpp fee955139d9699caa651d9bbd03342f2eba8143f
src/common/validation.cpp b2548ad87b4227d6e498c49b5694acb362f6281b
src/health-check/health_checker.cpp a8424b75927d15dc1b897faf0e47cf075c70ff26
src/master/validation.cpp 5f134b781901f2de6a90fa6a10d42cc7629c27a0
Diff: https://reviews.apache.org/r/55954/diff/
Testing
-------
Testing information at the end of this review chain.
Thanks,
Greg Mann
Re: Review Request 55954: Changed 'Environment.Variable.Value' from
required to optional.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55954/#review163704
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On Jan. 28, 2017, 11:34 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55954/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2017, 11:34 p.m.)
>
>
> Review request for mesos, Jan Schlicht and Vinod Kone.
>
>
> Bugs: MESOS-6991
> https://issues.apache.org/jira/browse/MESOS-6991
>
>
> Repository: mesos
>
>
> Description
> -------
>
> To prepare for future work which will enable the
> modular fetching of secrets, we should change the
> Environment.Variable.Value field from required to
> optional. This way, the field can be left empty
> and filled in by a secret fetching module.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto a08b8fb2edf26ae730467557ae0e33e3d4252593
> include/mesos/v1/mesos.proto 4a4609e7db659252ef7f1ca61f8ba079b901b7d0
> src/checks/checker.cpp 8d7285af40d9633608178ec239d3b8d65d3a2725
> src/checks/health_checker.cpp e70bd7936752613a4f92c70c4c61cd7cdf7c4ee5
> src/common/validation.hpp fee955139d9699caa651d9bbd03342f2eba8143f
> src/common/validation.cpp b2548ad87b4227d6e498c49b5694acb362f6281b
> src/master/validation.cpp e3f71be16ff21a1b4e10ad59846d9b06691bc1b9
> src/slave/validation.cpp c30a0ebf044ac81b087a8a599c7476bdc16bef18
>
> Diff: https://reviews.apache.org/r/55954/diff/
>
>
> Testing
> -------
>
> Testing information at the end of this review chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 55954: Changed 'Environment.Variable.Value' from
required to optional.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55954/
-----------------------------------------------------------
(Updated Jan. 28, 2017, 11:34 p.m.)
Review request for mesos, Jan Schlicht and Vinod Kone.
Changes
-------
Addressed comments, added more validation.
Bugs: MESOS-6991
https://issues.apache.org/jira/browse/MESOS-6991
Repository: mesos
Description
-------
To prepare for future work which will enable the
modular fetching of secrets, we should change the
Environment.Variable.Value field from required to
optional. This way, the field can be left empty
and filled in by a secret fetching module.
Diffs (updated)
-----
include/mesos/mesos.proto a08b8fb2edf26ae730467557ae0e33e3d4252593
include/mesos/v1/mesos.proto 4a4609e7db659252ef7f1ca61f8ba079b901b7d0
src/checks/checker.cpp 8d7285af40d9633608178ec239d3b8d65d3a2725
src/checks/health_checker.cpp e70bd7936752613a4f92c70c4c61cd7cdf7c4ee5
src/common/validation.hpp fee955139d9699caa651d9bbd03342f2eba8143f
src/common/validation.cpp b2548ad87b4227d6e498c49b5694acb362f6281b
src/master/validation.cpp e3f71be16ff21a1b4e10ad59846d9b06691bc1b9
src/slave/validation.cpp c30a0ebf044ac81b087a8a599c7476bdc16bef18
Diff: https://reviews.apache.org/r/55954/diff/
Testing
-------
Testing information at the end of this review chain.
Thanks,
Greg Mann
Re: Review Request 55954: Changed 'Environment.Variable.Value' from
required to optional.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55954/#review163128
-----------------------------------------------------------
Ship it!
- Jan Schlicht
On Jan. 26, 2017, 3:36 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55954/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2017, 3:36 a.m.)
>
>
> Review request for mesos, Jan Schlicht and Vinod Kone.
>
>
> Bugs: MESOS-6991
> https://issues.apache.org/jira/browse/MESOS-6991
>
>
> Repository: mesos
>
>
> Description
> -------
>
> To prepare for future work which will enable the
> modular fetching of secrets, we should change the
> Environment.Variable.Value field from required to
> optional. This way, the field can be left empty
> and filled in by a secret fetching module.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 044b7e697f77c9e2cefb938bda4307d328b33766
> include/mesos/v1/mesos.proto 7706de89d91926aedb10e4c882dffcf050f03bb1
> src/checks/checker.cpp 8d7285af40d9633608178ec239d3b8d65d3a2725
> src/checks/health_checker.cpp e70bd7936752613a4f92c70c4c61cd7cdf7c4ee5
> src/common/validation.hpp fee955139d9699caa651d9bbd03342f2eba8143f
> src/common/validation.cpp b2548ad87b4227d6e498c49b5694acb362f6281b
> src/master/validation.cpp e3f71be16ff21a1b4e10ad59846d9b06691bc1b9
> src/slave/validation.cpp 3fd32feb3cd0e6d30a66a917e20fd9ca50b84dc2
>
> Diff: https://reviews.apache.org/r/55954/diff/
>
>
> Testing
> -------
>
> Testing information at the end of this review chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 55954: Changed 'Environment.Variable.Value' from
required to optional.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55954/#review163344
-----------------------------------------------------------
src/slave/validation.cpp (line 175)
<https://reviews.apache.org/r/55954/#comment234749>
put this on the above line? does it not fit?
src/slave/validation.cpp (line 252)
<https://reviews.apache.org/r/55954/#comment234751>
s/launch_nested_container()/launch_nested_container_session()/
src/slave/validation.cpp (line 254)
<https://reviews.apache.org/r/55954/#comment234752>
put this on the above line.
src/slave/validation.cpp (line 255)
<https://reviews.apache.org/r/55954/#comment234753>
ditto. session*
- Vinod Kone
On Jan. 26, 2017, 2:36 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55954/
> -----------------------------------------------------------
>
> (Updated Jan. 26, 2017, 2:36 a.m.)
>
>
> Review request for mesos, Jan Schlicht and Vinod Kone.
>
>
> Bugs: MESOS-6991
> https://issues.apache.org/jira/browse/MESOS-6991
>
>
> Repository: mesos
>
>
> Description
> -------
>
> To prepare for future work which will enable the
> modular fetching of secrets, we should change the
> Environment.Variable.Value field from required to
> optional. This way, the field can be left empty
> and filled in by a secret fetching module.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 044b7e697f77c9e2cefb938bda4307d328b33766
> include/mesos/v1/mesos.proto 7706de89d91926aedb10e4c882dffcf050f03bb1
> src/checks/checker.cpp 8d7285af40d9633608178ec239d3b8d65d3a2725
> src/checks/health_checker.cpp e70bd7936752613a4f92c70c4c61cd7cdf7c4ee5
> src/common/validation.hpp fee955139d9699caa651d9bbd03342f2eba8143f
> src/common/validation.cpp b2548ad87b4227d6e498c49b5694acb362f6281b
> src/master/validation.cpp e3f71be16ff21a1b4e10ad59846d9b06691bc1b9
> src/slave/validation.cpp 3fd32feb3cd0e6d30a66a917e20fd9ca50b84dc2
>
> Diff: https://reviews.apache.org/r/55954/diff/
>
>
> Testing
> -------
>
> Testing information at the end of this review chain.
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 55954: Changed 'Environment.Variable.Value' from
required to optional.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55954/
-----------------------------------------------------------
(Updated Jan. 26, 2017, 2:36 a.m.)
Review request for mesos, Jan Schlicht and Vinod Kone.
Bugs: MESOS-6991
https://issues.apache.org/jira/browse/MESOS-6991
Repository: mesos
Description
-------
To prepare for future work which will enable the
modular fetching of secrets, we should change the
Environment.Variable.Value field from required to
optional. This way, the field can be left empty
and filled in by a secret fetching module.
Diffs (updated)
-----
include/mesos/mesos.proto 044b7e697f77c9e2cefb938bda4307d328b33766
include/mesos/v1/mesos.proto 7706de89d91926aedb10e4c882dffcf050f03bb1
src/checks/checker.cpp 8d7285af40d9633608178ec239d3b8d65d3a2725
src/checks/health_checker.cpp e70bd7936752613a4f92c70c4c61cd7cdf7c4ee5
src/common/validation.hpp fee955139d9699caa651d9bbd03342f2eba8143f
src/common/validation.cpp b2548ad87b4227d6e498c49b5694acb362f6281b
src/master/validation.cpp e3f71be16ff21a1b4e10ad59846d9b06691bc1b9
src/slave/validation.cpp 3fd32feb3cd0e6d30a66a917e20fd9ca50b84dc2
Diff: https://reviews.apache.org/r/55954/diff/
Testing
-------
Testing information at the end of this review chain.
Thanks,
Greg Mann
Re: Review Request 55954: Changed 'Environment.Variable.Value' from
required to optional.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55954/#review163046
-----------------------------------------------------------
Looks like you missed validation in a few other places:
ExecutorInfo.CommandInfo
LaunchNestedContainer.CommandInfo
LaunchNestedContainerSession.CommandInfo
CheckInfo.CommandInfo
src/common/validation.cpp (line 90)
<https://reviews.apache.org/r/55954/#comment234477>
do you need this if statement?
src/common/validation.cpp (lines 94 - 96)
<https://reviews.apache.org/r/55954/#comment234478>
how about:
return Error("Environment variable '" + variable.name() + "' must have a value set");
src/health-check/health_checker.cpp (line 676)
<https://reviews.apache.org/r/55954/#comment234479>
you would also need to add validation for regular checks, not just health checks.
looks like you didn't update your master branch? this file is now under "src/checks".
- Vinod Kone
On Jan. 25, 2017, 8:10 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55954/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2017, 8:10 p.m.)
>
>
> Review request for mesos, Jan Schlicht and Vinod Kone.
>
>
> Bugs: MESOS-6991
> https://issues.apache.org/jira/browse/MESOS-6991
>
>
> Repository: mesos
>
>
> Description
> -------
>
> To prepare for future work which will enable the
> modular fetching of secrets, we should change the
> Environment.Variable.Value field from required to
> optional. This way, the field can be left empty
> and filled in by a secret fetching module.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8f14444d6957a97eff1e0a94817d38e7a7de6d69
> include/mesos/v1/mesos.proto 74e7851b147ab821dceeab6e838d34b092f101c3
> src/common/validation.hpp fee955139d9699caa651d9bbd03342f2eba8143f
> src/common/validation.cpp b2548ad87b4227d6e498c49b5694acb362f6281b
> src/health-check/health_checker.cpp a8424b75927d15dc1b897faf0e47cf075c70ff26
> src/master/validation.cpp 5f134b781901f2de6a90fa6a10d42cc7629c27a0
>
> Diff: https://reviews.apache.org/r/55954/diff/
>
>
> Testing
> -------
>
> Testing information at the end of this review chain.
>
>
> Thanks,
>
> Greg Mann
>
>