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/28 23:39:27 UTC
Review Request 56055: Added validation for
'LAUNCH_NESTED_CONTAINER_SESSION'.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56055/
-----------------------------------------------------------
Review request for mesos, Jan Schlicht and Vinod Kone.
Repository: mesos
Description
-------
This patch adds a validation test for the
`LAUNCH_NESTED_CONTAINER_SESSION` call.
Diffs
-----
src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
Diff: https://reviews.apache.org/r/56055/diff/
Testing
-------
`bin/mesos-tests.sh --gtest_filter="*Validation*"`
Thanks,
Greg Mann
Re: Review Request 56055: Added validation for
'LAUNCH_NESTED_CONTAINER_SESSION'.
Posted by Mesos Reviewbot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56055/#review163422
-----------------------------------------------------------
Patch looks great!
Reviews applied: [55954, 55955, 56055]
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. 28, 2017, 11:39 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56055/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2017, 11:39 p.m.)
>
>
> Review request for mesos, Jan Schlicht and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a validation test for the
> `LAUNCH_NESTED_CONTAINER_SESSION` call.
>
>
> Diffs
> -----
>
> src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
>
> Diff: https://reviews.apache.org/r/56055/diff/
>
>
> Testing
> -------
>
> `bin/mesos-tests.sh --gtest_filter="*Validation*"`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56055: Added validation for
'LAUNCH_NESTED_CONTAINER_SESSION'.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56055/#review163709
-----------------------------------------------------------
Fix it, then Ship it!
src/tests/slave_validation_tests.cpp (line 220)
<https://reviews.apache.org/r/56055/#comment235188>
Can you attach https://issues.apache.org/jira/browse/MESOS-6887 as the bug id?
- Vinod Kone
On Jan. 28, 2017, 11:39 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56055/
> -----------------------------------------------------------
>
> (Updated Jan. 28, 2017, 11:39 p.m.)
>
>
> Review request for mesos, Jan Schlicht and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a validation test for the
> `LAUNCH_NESTED_CONTAINER_SESSION` call.
>
>
> Diffs
> -----
>
> src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
>
> Diff: https://reviews.apache.org/r/56055/diff/
>
>
> Testing
> -------
>
> `bin/mesos-tests.sh --gtest_filter="*Validation*"`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56055: Added validation for
'LAUNCH_NESTED_CONTAINER_SESSION'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56055/
-----------------------------------------------------------
(Updated Jan. 31, 2017, 10:19 p.m.)
Review request for mesos, Jan Schlicht and Vinod Kone.
Bugs: MESOS-6887
https://issues.apache.org/jira/browse/MESOS-6887
Repository: mesos
Description
-------
This patch adds a validation test for the
`LAUNCH_NESTED_CONTAINER_SESSION` call.
Diffs (updated)
-----
src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
Diff: https://reviews.apache.org/r/56055/diff/
Testing
-------
`bin/mesos-tests.sh --gtest_filter="*Validation*"`
Thanks,
Greg Mann
Re: Review Request 56055: Added validation for
'LAUNCH_NESTED_CONTAINER_SESSION'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56055/
-----------------------------------------------------------
(Updated Jan. 31, 2017, 10:17 p.m.)
Review request for mesos, Jan Schlicht and Vinod Kone.
Bugs: MESOS-6887
https://issues.apache.org/jira/browse/MESOS-6887
Repository: mesos
Description
-------
This patch adds a validation test for the
`LAUNCH_NESTED_CONTAINER_SESSION` call.
Diffs (updated)
-----
src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
Diff: https://reviews.apache.org/r/56055/diff/
Testing
-------
`bin/mesos-tests.sh --gtest_filter="*Validation*"`
Thanks,
Greg Mann
Re: Review Request 56055: Added validation for
'LAUNCH_NESTED_CONTAINER_SESSION'.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56055/
-----------------------------------------------------------
(Updated Jan. 31, 2017, 10:13 p.m.)
Review request for mesos, Jan Schlicht and Vinod Kone.
Changes
-------
Addressed comments.
Bugs: MESOS-6887
https://issues.apache.org/jira/browse/MESOS-6887
Repository: mesos
Description
-------
This patch adds a validation test for the
`LAUNCH_NESTED_CONTAINER_SESSION` call.
Diffs (updated)
-----
src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
Diff: https://reviews.apache.org/r/56055/diff/
Testing
-------
`bin/mesos-tests.sh --gtest_filter="*Validation*"`
Thanks,
Greg Mann
Re: Review Request 56055: Added validation for
'LAUNCH_NESTED_CONTAINER_SESSION'.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Jan. 30, 2017, 9:24 a.m., Jan Schlicht wrote:
> > src/tests/slave_validation_tests.cpp, line 282
> > <https://reviews.apache.org/r/56055/diff/1/?file=1618348#file1618348line282>
> >
> > Indent with 4 spaces.
Unfortunately, I think our style guide is ambiguous on this point? And it seems that within the Mesos codebase we see about equal numbers of both cases, indenting 2 and indenting 4 spaces after a linebreak like this. A little playing around with `clang-format` seems to suggest that it prefers indenting 2 spaces in this case. I don't have a strong opinion, happy to do whatever is the "right" thing here. Local consistency is probably the most important, and since all such occurrences in this file are part of the new tests added in this review chain, I can set them all to be either 2 or 4.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56055/#review163486
-----------------------------------------------------------
On Jan. 31, 2017, 10:19 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56055/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2017, 10:19 p.m.)
>
>
> Review request for mesos, Jan Schlicht and Vinod Kone.
>
>
> Bugs: MESOS-6887
> https://issues.apache.org/jira/browse/MESOS-6887
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a validation test for the
> `LAUNCH_NESTED_CONTAINER_SESSION` call.
>
>
> Diffs
> -----
>
> src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
>
> Diff: https://reviews.apache.org/r/56055/diff/
>
>
> Testing
> -------
>
> `bin/mesos-tests.sh --gtest_filter="*Validation*"`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56055: Added validation for
'LAUNCH_NESTED_CONTAINER_SESSION'.
Posted by Jan Schlicht <ja...@mesosphere.io>.
> On Jan. 30, 2017, 10:24 a.m., Jan Schlicht wrote:
> > src/tests/slave_validation_tests.cpp, line 282
> > <https://reviews.apache.org/r/56055/diff/1/?file=1618348#file1618348line282>
> >
> > Indent with 4 spaces.
>
> Greg Mann wrote:
> Unfortunately, I think our style guide is ambiguous on this point? And it seems that within the Mesos codebase we see about equal numbers of both cases, indenting 2 and indenting 4 spaces after a linebreak like this. A little playing around with `clang-format` seems to suggest that it prefers indenting 2 spaces in this case. I don't have a strong opinion, happy to do whatever is the "right" thing here. Local consistency is probably the most important, and since all such occurrences in this file are part of the new tests added in this review chain, I can set them all to be either 2 or 4.
>
> Greg Mann wrote:
> I also changed the newline pattern for one of these occurrences to something I prefer a bit more (no opinion on 2 vs. 4 spaces in the indent, I'm fine with either):
> ```
> Environment::Variable* variable = launch
> ->mutable_command()
> ->mutable_environment()
> ->mutable_variables()
> ->Add();
> ```
>
> Greg Mann wrote:
> I'm gonna drop this issue for now; if there's a case to be made for 4 spaces, I'm happy to change it.
I also don't have an opinion on 2 vs. 4 spaces. Just wanted it to be consistent with https://reviews.apache.org/r/55955 where similar occurrences are indented with 4 spaces.
- Jan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56055/#review163486
-----------------------------------------------------------
On Jan. 31, 2017, 11:19 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56055/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2017, 11:19 p.m.)
>
>
> Review request for mesos, Jan Schlicht and Vinod Kone.
>
>
> Bugs: MESOS-6887
> https://issues.apache.org/jira/browse/MESOS-6887
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a validation test for the
> `LAUNCH_NESTED_CONTAINER_SESSION` call.
>
>
> Diffs
> -----
>
> src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
>
> Diff: https://reviews.apache.org/r/56055/diff/
>
>
> Testing
> -------
>
> `bin/mesos-tests.sh --gtest_filter="*Validation*"`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56055: Added validation for
'LAUNCH_NESTED_CONTAINER_SESSION'.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Jan. 30, 2017, 9:24 a.m., Jan Schlicht wrote:
> > src/tests/slave_validation_tests.cpp, line 282
> > <https://reviews.apache.org/r/56055/diff/1/?file=1618348#file1618348line282>
> >
> > Indent with 4 spaces.
>
> Greg Mann wrote:
> Unfortunately, I think our style guide is ambiguous on this point? And it seems that within the Mesos codebase we see about equal numbers of both cases, indenting 2 and indenting 4 spaces after a linebreak like this. A little playing around with `clang-format` seems to suggest that it prefers indenting 2 spaces in this case. I don't have a strong opinion, happy to do whatever is the "right" thing here. Local consistency is probably the most important, and since all such occurrences in this file are part of the new tests added in this review chain, I can set them all to be either 2 or 4.
I also changed the newline pattern for one of these occurrences to something I prefer a bit more (no opinion on 2 vs. 4 spaces in the indent, I'm fine with either):
```
Environment::Variable* variable = launch
->mutable_command()
->mutable_environment()
->mutable_variables()
->Add();
```
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56055/#review163486
-----------------------------------------------------------
On Jan. 31, 2017, 10:19 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56055/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2017, 10:19 p.m.)
>
>
> Review request for mesos, Jan Schlicht and Vinod Kone.
>
>
> Bugs: MESOS-6887
> https://issues.apache.org/jira/browse/MESOS-6887
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a validation test for the
> `LAUNCH_NESTED_CONTAINER_SESSION` call.
>
>
> Diffs
> -----
>
> src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
>
> Diff: https://reviews.apache.org/r/56055/diff/
>
>
> Testing
> -------
>
> `bin/mesos-tests.sh --gtest_filter="*Validation*"`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56055: Added validation for
'LAUNCH_NESTED_CONTAINER_SESSION'.
Posted by Greg Mann <gr...@mesosphere.io>.
> On Jan. 30, 2017, 9:24 a.m., Jan Schlicht wrote:
> > src/tests/slave_validation_tests.cpp, line 282
> > <https://reviews.apache.org/r/56055/diff/1/?file=1618348#file1618348line282>
> >
> > Indent with 4 spaces.
>
> Greg Mann wrote:
> Unfortunately, I think our style guide is ambiguous on this point? And it seems that within the Mesos codebase we see about equal numbers of both cases, indenting 2 and indenting 4 spaces after a linebreak like this. A little playing around with `clang-format` seems to suggest that it prefers indenting 2 spaces in this case. I don't have a strong opinion, happy to do whatever is the "right" thing here. Local consistency is probably the most important, and since all such occurrences in this file are part of the new tests added in this review chain, I can set them all to be either 2 or 4.
>
> Greg Mann wrote:
> I also changed the newline pattern for one of these occurrences to something I prefer a bit more (no opinion on 2 vs. 4 spaces in the indent, I'm fine with either):
> ```
> Environment::Variable* variable = launch
> ->mutable_command()
> ->mutable_environment()
> ->mutable_variables()
> ->Add();
> ```
I'm gonna drop this issue for now; if there's a case to be made for 4 spaces, I'm happy to change it.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56055/#review163486
-----------------------------------------------------------
On Jan. 31, 2017, 10:19 p.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56055/
> -----------------------------------------------------------
>
> (Updated Jan. 31, 2017, 10:19 p.m.)
>
>
> Review request for mesos, Jan Schlicht and Vinod Kone.
>
>
> Bugs: MESOS-6887
> https://issues.apache.org/jira/browse/MESOS-6887
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a validation test for the
> `LAUNCH_NESTED_CONTAINER_SESSION` call.
>
>
> Diffs
> -----
>
> src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
>
> Diff: https://reviews.apache.org/r/56055/diff/
>
>
> Testing
> -------
>
> `bin/mesos-tests.sh --gtest_filter="*Validation*"`
>
>
> Thanks,
>
> Greg Mann
>
>
Re: Review Request 56055: Added validation for
'LAUNCH_NESTED_CONTAINER_SESSION'.
Posted by Jan Schlicht <ja...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/56055/#review163486
-----------------------------------------------------------
src/tests/slave_validation_tests.cpp (line 282)
<https://reviews.apache.org/r/56055/#comment234960>
Indent with 4 spaces.
- Jan Schlicht
On Jan. 29, 2017, 12:39 a.m., Greg Mann wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56055/
> -----------------------------------------------------------
>
> (Updated Jan. 29, 2017, 12:39 a.m.)
>
>
> Review request for mesos, Jan Schlicht and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds a validation test for the
> `LAUNCH_NESTED_CONTAINER_SESSION` call.
>
>
> Diffs
> -----
>
> src/tests/slave_validation_tests.cpp 5de771114982751e7796f55dcacd4384c6989efb
>
> Diff: https://reviews.apache.org/r/56055/diff/
>
>
> Testing
> -------
>
> `bin/mesos-tests.sh --gtest_filter="*Validation*"`
>
>
> Thanks,
>
> Greg Mann
>
>