----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/33109/#review80660 -----------------------------------------------------------
Thanks for jumping on this. Great start, but we need to rethink the format/delimiters to account for environment variables like PATH that might contain `:` or `;`. I assume you manually tested this with mesos-execute --env? Please describe your manual test in the testing section as well. src/cli/execute.cpp <https://reviews.apache.org/r/33109/#comment130789> Any reason not to just name this 'env' or 'environment'? src/cli/execute.cpp <https://reviews.apache.org/r/33109/#comment130792> Please `#include <stout/hashmap.hpp>` src/cli/execute.cpp <https://reviews.apache.org/r/33109/#comment130790> Style nit: Please bring the opening brace up to the previous line, just like the rest of the the 'if' blocks in this file. src/cli/execute.cpp <https://reviews.apache.org/r/33109/#comment130791> Instead of using an index, since you're just adding new variables as you iterate through commandEnv, try `Environment_Variable* envVar = env->mutable_variables()->add_variables();` which will append a new `variable` to the list and return a pointer to you. src/cli/execute.cpp <https://reviews.apache.org/r/33109/#comment130793> How is this going to work with environment variables like PATH that expect :'s inside their values? Probably need to choose another delimiter (even `;` and `=` could be tricky), or pass in a newline-delimited file, or go all the way to json lists. - Adam B On April 13, 2015, 9:42 a.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/33109/ > ----------------------------------------------------------- > > (Updated April 13, 2015, 9:42 a.m.) > > > Review request for mesos and Adam B. > > > Bugs: MESOS-2023 > https://issues.apache.org/jira/browse/MESOS-2023 > > > Repository: mesos > > > Description > ------- > > Allow setting environment variables in mesos-execute > > > Diffs > ----- > > src/cli/execute.cpp 84f70dccbc2c5dd43f68105d967f4488c82f582b > > Diff: https://reviews.apache.org/r/33109/diff/ > > > Testing > ------- > > make check > > > Thanks, > > haosdent huang > >
