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:13:30 UTC
Review Request 55955: Added validation tests to ensure environment
variable value is set.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55955/
-----------------------------------------------------------
Review request for mesos, Jan Schlicht and Vinod Kone.
Bugs: MESOS-6991
https://issues.apache.org/jira/browse/MESOS-6991
Repository: mesos
Description
-------
The `value` field within `Environment::Variable` is being
changed to `optional`, but for the time being we will
enforce that it must be set for backward compatibility.
This patch adds tests to ensure that environment variables
with unset values are correctly rejected.
Diffs
-----
src/tests/health_check_tests.cpp 0a6d2dd295408dcc0434f3573e307e685f9abfe4
src/tests/master_validation_tests.cpp a63178139a5283d6a3fcbe60c271dab1914e5da9
Diff: https://reviews.apache.org/r/55955/diff/
Testing
-------
bin/mesos-tests.sh --gtest_filter="*Validation*"
Thanks,
Greg Mann
Re: Review Request 55955: Added validation tests to ensure environment
variable value is set.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55955/#review163073
-----------------------------------------------------------
Bad patch!
Reviews applied: [55955, 55954]
Failed command: python support/apply-reviews.py -n -r 55954
Error:
2017-01-26 01:40:36 URL:https://reviews.apache.org/r/55954/diff/raw/ [5512/5512] -> "55954.patch" [1]
error: src/health-check/health_checker.cpp: does not exist in index
error: patch failed: src/master/validation.cpp:914
error: src/master/validation.cpp: patch does not apply
Full log: https://builds.apache.org/job/Mesos-Reviewbot/16855/console
- Mesos Reviewbot
On Jan. 25, 2017, 8:13 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55955/
> -----------------------------------------------------------
>
> (Updated Jan. 25, 2017, 8:13 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
> -------
>
> The `value` field within `Environment::Variable` is being
> changed to `optional`, but for the time being we will
> enforce that it must be set for backward compatibility.
> This patch adds tests to ensure that environment variables
> with unset values are correctly rejected.
>
>
> Diffs
> -----
>
> src/tests/health_check_tests.cpp 0a6d2dd295408dcc0434f3573e307e685f9abfe4
> src/tests/master_validation_tests.cpp a63178139a5283d6a3fcbe60c271dab1914e5da9
>
> Diff: https://reviews.apache.org/r/55955/diff/
>
>
> Testing
> -------
>
> bin/mesos-tests.sh --gtest_filter="*Validation*"
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 55955: Added validation tests to ensure environment
variable value is set.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55955/#review163104
-----------------------------------------------------------
Patch looks great!
Reviews applied: [55954, 55955]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker-build.sh
- Mesos Reviewbot
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/55955/
> -----------------------------------------------------------
>
> (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
> -------
>
> The `value` field within `Environment::Variable` is being
> changed to `optional`, but for the time being we will
> enforce that it must be set for backward compatibility.
> This patch adds tests to ensure that environment variables
> with unset values are correctly rejected.
>
>
> Diffs
> -----
>
> src/tests/check_tests.cpp c88cd34fd214f111cff62591aa5fc03eb62567e4
> src/tests/health_check_tests.cpp debbd3c09b7555145aaf3f62a24d795d1423a269
> src/tests/master_validation_tests.cpp edb57407e08cdbd8fbf10a9e1493cab3b4979bb8
> src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
>
> Diff: https://reviews.apache.org/r/55955/diff/
>
>
> Testing
> -------
>
> bin/mesos-tests.sh --gtest_filter="*Validation*"
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 55955: Added validation tests to ensure environment
variable value is set.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55955/
-----------------------------------------------------------
(Updated Jan. 31, 2017, 10:23 p.m.)
Review request for mesos, Jan Schlicht and Vinod Kone.
Changes
-------
Changed indentation.
Bugs: MESOS-6991
https://issues.apache.org/jira/browse/MESOS-6991
Repository: mesos
Description
-------
The `value` field within `Environment::Variable` is being
changed to `optional`, but for the time being we will
enforce that it must be set for backward compatibility.
This patch adds tests to ensure that environment variables
with unset values are correctly rejected.
Diffs (updated)
-----
src/tests/check_tests.cpp c88cd34fd214f111cff62591aa5fc03eb62567e4
src/tests/health_check_tests.cpp 710cb66eff6c4447caa22772f0cdc97cfa582c50
src/tests/master_validation_tests.cpp edb57407e08cdbd8fbf10a9e1493cab3b4979bb8
src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
Diff: https://reviews.apache.org/r/55955/diff/
Testing
-------
bin/mesos-tests.sh --gtest_filter="*Validation*"
Thanks,
Greg Mann
Re: Review Request 55955: Added validation tests to ensure environment
variable value is set.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55955/
-----------------------------------------------------------
(Updated Jan. 31, 2017, 10:12 p.m.)
Review request for mesos, Jan Schlicht and Vinod Kone.
Changes
-------
Addressed comments.
Bugs: MESOS-6991
https://issues.apache.org/jira/browse/MESOS-6991
Repository: mesos
Description
-------
The `value` field within `Environment::Variable` is being
changed to `optional`, but for the time being we will
enforce that it must be set for backward compatibility.
This patch adds tests to ensure that environment variables
with unset values are correctly rejected.
Diffs (updated)
-----
src/tests/check_tests.cpp c88cd34fd214f111cff62591aa5fc03eb62567e4
src/tests/health_check_tests.cpp 710cb66eff6c4447caa22772f0cdc97cfa582c50
src/tests/master_validation_tests.cpp edb57407e08cdbd8fbf10a9e1493cab3b4979bb8
src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
Diff: https://reviews.apache.org/r/55955/diff/
Testing
-------
bin/mesos-tests.sh --gtest_filter="*Validation*"
Thanks,
Greg Mann
Re: Review Request 55955: Added validation tests to ensure environment
variable value is set.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Jan. 31, 2017, 7:58 p.m., Vinod Kone wrote:
> > src/tests/master_validation_tests.cpp, line 2678
> > <https://reviews.apache.org/r/55955/diff/2-3/?file=1616128#file1616128line2678>
> >
> > We stopped using `Times(1)` because that is redundant. You can just do
> >
> > ```
> > EXPECT_CALL(sched, registered(&driver, _, _));
> > ```
> >
> > If there are more instances of that in this file, can you do a sweep in a different review?
You can find the follow-up review here: https://reviews.apache.org/r/56139/
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55955/#review163705
-----------------------------------------------------------
On Jan. 31, 2017, 10:23 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55955/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2017, 10:23 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
> -------
>
> The `value` field within `Environment::Variable` is being
> changed to `optional`, but for the time being we will
> enforce that it must be set for backward compatibility.
> This patch adds tests to ensure that environment variables
> with unset values are correctly rejected.
>
>
> Diffs
> -----
>
> src/tests/check_tests.cpp c88cd34fd214f111cff62591aa5fc03eb62567e4
> src/tests/health_check_tests.cpp 710cb66eff6c4447caa22772f0cdc97cfa582c50
> src/tests/master_validation_tests.cpp edb57407e08cdbd8fbf10a9e1493cab3b4979bb8
> src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
>
> Diff: https://reviews.apache.org/r/55955/diff/
>
>
> Testing
> -------
>
> bin/mesos-tests.sh --gtest_filter="*Validation*"
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 55955: Added validation tests to ensure environment
variable value is set.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55955/#review163705
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/master_validation_tests.cpp (line 2678)
<https://reviews.apache.org/r/55955/#comment235182>
We stopped using `Times(1)` because that is redundant. You can just do
```
EXPECT_CALL(sched, registered(&driver, _, _));
```
If there are more instances of that in this file, can you do a sweep in a different review?
src/tests/master_validation_tests.cpp (line 2783)
<https://reviews.apache.org/r/55955/#comment235183>
ditto.
src/tests/master_validation_tests.cpp (lines 2819 - 2822)
<https://reviews.apache.org/r/55955/#comment235184>
move this to #2800.
- Vinod Kone
On Jan. 28, 2017, 11:35 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55955/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2017, 11:35 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
> -------
>
> The `value` field within `Environment::Variable` is being
> changed to `optional`, but for the time being we will
> enforce that it must be set for backward compatibility.
> This patch adds tests to ensure that environment variables
> with unset values are correctly rejected.
>
>
> Diffs
> -----
>
> src/tests/check_tests.cpp c88cd34fd214f111cff62591aa5fc03eb62567e4
> src/tests/health_check_tests.cpp debbd3c09b7555145aaf3f62a24d795d1423a269
> src/tests/master_validation_tests.cpp edb57407e08cdbd8fbf10a9e1493cab3b4979bb8
> src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
>
> Diff: https://reviews.apache.org/r/55955/diff/
>
>
> Testing
> -------
>
> bin/mesos-tests.sh --gtest_filter="*Validation*"
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 55955: Added validation tests to ensure environment
variable value is set.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55955/#review163484
-----------------------------------------------------------
Ship it!
Ship It!
- Jan Schlicht
On Jan. 29, 2017, 12:35 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55955/
> -----------------------------------------------------------
>
> (Updated Jan. 29, 2017, 12:35 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
> -------
>
> The `value` field within `Environment::Variable` is being
> changed to `optional`, but for the time being we will
> enforce that it must be set for backward compatibility.
> This patch adds tests to ensure that environment variables
> with unset values are correctly rejected.
>
>
> Diffs
> -----
>
> src/tests/check_tests.cpp c88cd34fd214f111cff62591aa5fc03eb62567e4
> src/tests/health_check_tests.cpp debbd3c09b7555145aaf3f62a24d795d1423a269
> src/tests/master_validation_tests.cpp edb57407e08cdbd8fbf10a9e1493cab3b4979bb8
> src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
>
> Diff: https://reviews.apache.org/r/55955/diff/
>
>
> Testing
> -------
>
> bin/mesos-tests.sh --gtest_filter="*Validation*"
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 55955: Added validation tests to ensure environment
variable value is set.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55955/
-----------------------------------------------------------
(Updated Jan. 28, 2017, 11:35 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
-------
The `value` field within `Environment::Variable` is being
changed to `optional`, but for the time being we will
enforce that it must be set for backward compatibility.
This patch adds tests to ensure that environment variables
with unset values are correctly rejected.
Diffs (updated)
-----
src/tests/check_tests.cpp c88cd34fd214f111cff62591aa5fc03eb62567e4
src/tests/health_check_tests.cpp debbd3c09b7555145aaf3f62a24d795d1423a269
src/tests/master_validation_tests.cpp edb57407e08cdbd8fbf10a9e1493cab3b4979bb8
src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
Diff: https://reviews.apache.org/r/55955/diff/
Testing
-------
bin/mesos-tests.sh --gtest_filter="*Validation*"
Thanks,
Greg Mann
Re: Review Request 55955: Added validation tests to ensure environment
variable value is set.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55955/#review163346
-----------------------------------------------------------
Ship it!
Ship It!
- 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/55955/
> -----------------------------------------------------------
>
> (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
> -------
>
> The `value` field within `Environment::Variable` is being
> changed to `optional`, but for the time being we will
> enforce that it must be set for backward compatibility.
> This patch adds tests to ensure that environment variables
> with unset values are correctly rejected.
>
>
> Diffs
> -----
>
> src/tests/check_tests.cpp c88cd34fd214f111cff62591aa5fc03eb62567e4
> src/tests/health_check_tests.cpp debbd3c09b7555145aaf3f62a24d795d1423a269
> src/tests/master_validation_tests.cpp edb57407e08cdbd8fbf10a9e1493cab3b4979bb8
> src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
>
> Diff: https://reviews.apache.org/r/55955/diff/
>
>
> Testing
> -------
>
> bin/mesos-tests.sh --gtest_filter="*Validation*"
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 55955: Added validation tests to ensure environment
variable value is set.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55955/
-----------------------------------------------------------
(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
-------
The `value` field within `Environment::Variable` is being
changed to `optional`, but for the time being we will
enforce that it must be set for backward compatibility.
This patch adds tests to ensure that environment variables
with unset values are correctly rejected.
Diffs (updated)
-----
src/tests/check_tests.cpp c88cd34fd214f111cff62591aa5fc03eb62567e4
src/tests/health_check_tests.cpp debbd3c09b7555145aaf3f62a24d795d1423a269
src/tests/master_validation_tests.cpp edb57407e08cdbd8fbf10a9e1493cab3b4979bb8
src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
Diff: https://reviews.apache.org/r/55955/diff/
Testing
-------
bin/mesos-tests.sh --gtest_filter="*Validation*"
Thanks,
Greg Mann