You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by haosdent huang <ha...@gmail.com> on 2015/07/04 13:08:54 UTC
Review Request 36179: Fix duplicate "-e" environment variables option
in Docker::run.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36179/
-----------------------------------------------------------
Review request for mesos, Anindya Sinha and Timothy Chen.
Bugs: MESOS-2882
https://issues.apache.org/jira/browse/MESOS-2882
Repository: mesos
Description
-------
Fix duplicate "-e" environment variables option in Docker::run.
Diffs
-----
src/docker/docker.cpp 235ac4a093b2c23a15f2de43780f93c054ddcc4f
Diff: https://reviews.apache.org/r/36179/diff/
Testing
-------
Thanks,
haosdent huang
Re: Review Request 36179: Fix duplicate "-e" environment variables
option in Docker::run.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36179/#review90398
-----------------------------------------------------------
Patch looks great!
Reviews applied: [36179]
All tests passed.
- Mesos ReviewBot
On July 4, 2015, noon, haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36179/
> -----------------------------------------------------------
>
> (Updated July 4, 2015, noon)
>
>
> Review request for mesos, Anindya Sinha and Timothy Chen.
>
>
> Bugs: MESOS-2882
> https://issues.apache.org/jira/browse/MESOS-2882
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix duplicate "-e" environment variables option in Docker::run.
>
>
> Diffs
> -----
>
> src/docker/docker.cpp 235ac4a093b2c23a15f2de43780f93c054ddcc4f
>
> Diff: https://reviews.apache.org/r/36179/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 36179: Fix duplicate "-e" environment variables
option in Docker::run.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36179/#review90393
-----------------------------------------------------------
Ship it!
Ship It!
- Timothy Chen
On July 4, 2015, noon, haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36179/
> -----------------------------------------------------------
>
> (Updated July 4, 2015, noon)
>
>
> Review request for mesos, Anindya Sinha and Timothy Chen.
>
>
> Bugs: MESOS-2882
> https://issues.apache.org/jira/browse/MESOS-2882
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix duplicate "-e" environment variables option in Docker::run.
>
>
> Diffs
> -----
>
> src/docker/docker.cpp 235ac4a093b2c23a15f2de43780f93c054ddcc4f
>
> Diff: https://reviews.apache.org/r/36179/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 36179: Fix duplicate "-e" environment variables
option in Docker::run.
Posted by haosdent huang <ha...@gmail.com>.
> On July 5, 2015, 6:36 a.m., Anindya Sinha wrote:
> > I know this is already merged but can't we just get rid of the foreach (const Environment::Variable...) block since env already contains the contents of commandInfo.environment().variables(). Or are there scenarios when that is not true?
I think add check in "foreach (const Environment::Variable& variable," should be better instead of remove this foreach. Because "src/docker/docker.cpp" is a independence class. When it has contain the interface like
```cpp
Future<Nothing> Docker::run(
const ContainerInfo& containerInfo,
const CommandInfo& commandInfo,
const string& name,
const string& sandboxDirectory,
const string& mappedDirectory,
const Option<Resources>& resources,
const Option<map<string, string>>& env,
const Option<string>& stdoutPath,
const Option<string>& stderrPath)
```
it should contains `commandInfo.environment().variables()` semantically. Other class maybe have some minor mistakes in call this method, but I think we could not remove this foreach because of other classes problem. And add this check
```
if (env.isSome() &&
env.get().find(variable.name()) != env.get().end()) {
// Skip to avoid duplicate environment variables.
continue;
}
```
I think could avoid the problem when other classes call docker->run with duplicate between `env` and `commandInfo.environment()`.
- haosdent
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36179/#review90402
-----------------------------------------------------------
On July 4, 2015, noon, haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36179/
> -----------------------------------------------------------
>
> (Updated July 4, 2015, noon)
>
>
> Review request for mesos, Anindya Sinha and Timothy Chen.
>
>
> Bugs: MESOS-2882
> https://issues.apache.org/jira/browse/MESOS-2882
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix duplicate "-e" environment variables option in Docker::run.
>
>
> Diffs
> -----
>
> src/docker/docker.cpp 235ac4a093b2c23a15f2de43780f93c054ddcc4f
>
> Diff: https://reviews.apache.org/r/36179/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 36179: Fix duplicate "-e" environment variables
option in Docker::run.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36179/#review90402
-----------------------------------------------------------
I know this is already merged but can't we just get rid of the foreach (const Environment::Variable...) block since env already contains the contents of commandInfo.environment().variables(). Or are there scenarios when that is not true?
- Anindya Sinha
On July 4, 2015, noon, haosdent huang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36179/
> -----------------------------------------------------------
>
> (Updated July 4, 2015, noon)
>
>
> Review request for mesos, Anindya Sinha and Timothy Chen.
>
>
> Bugs: MESOS-2882
> https://issues.apache.org/jira/browse/MESOS-2882
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Fix duplicate "-e" environment variables option in Docker::run.
>
>
> Diffs
> -----
>
> src/docker/docker.cpp 235ac4a093b2c23a15f2de43780f93c054ddcc4f
>
> Diff: https://reviews.apache.org/r/36179/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> haosdent huang
>
>
Re: Review Request 36179: Fix duplicate "-e" environment variables
option in Docker::run.
Posted by haosdent huang <ha...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36179/
-----------------------------------------------------------
(Updated July 4, 2015, noon)
Review request for mesos, Anindya Sinha and Timothy Chen.
Bugs: MESOS-2882
https://issues.apache.org/jira/browse/MESOS-2882
Repository: mesos
Description
-------
Fix duplicate "-e" environment variables option in Docker::run.
Diffs (updated)
-----
src/docker/docker.cpp 235ac4a093b2c23a15f2de43780f93c054ddcc4f
Diff: https://reviews.apache.org/r/36179/diff/
Testing
-------
Thanks,
haosdent huang