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