> On March 17, 2016, 7:48 a.m., Guangya Liu wrote: > > src/cli/execute.cpp, line 241 > > <https://reviews.apache.org/r/44934/diff/2/?file=1302214#file1302214line241> > > > > The default value of `containerizer` is `mesos`, do we need to check > > this? > > > > What about make the logic as this? This will involve less code change > > and the logic may be more clear? > > > > // Do not touch old dockerImage logic. > > if (dockerImage.isSome()) { > > old logic here; > > } > > > > if (appcImage.isSome() && containerizer == "mesos") { > > ContainerInfo containerInfo; > > containerInfo.set_type(ContainerInfo::MESOS); > > ContainerInfo::MesosInfo mesosInfo; > > > > Image::Appc appc; > > > > appc.set_name(appcImage.get()); > > > > // TODO(jojy): Labels are hard coded right now. > > // Consider adding label flags for customization. > > Label arch; > > arch.set_key("arch"); > > arch.set_value("amd64"); > > > > Label os; > > os.set_key("os"); > > os.set_value("linux"); > > > > Labels labels; > > labels.add_labels()->CopyFrom(os); > > labels.add_labels()->CopyFrom(arch); > > > > appc.mutable_labels()->CopyFrom(labels); > > > > Image mesosImage; > > > > mesosImage.set_type(Image::APPC); > > mesosImage.mutable_appc()->CopyFrom(appc); > > > > mesosInfo.mutable_image()->CopyFrom(mesosImage); > > > > containerInfo.mutable_mesos()->CopyFrom(mesosInfo); > > > > task.mutable_container()->CopyFrom(containerInfo); > > } > > Jojy Varghese wrote: > Then we will have to repeat the `if (containerizer == "mesos")` for both > dockerImage case and appcImage case. By seperating the logic by cotainerizer, > we now can add more image types into the "mesos" containerizer block (say OCI > image). > > Guangya Liu wrote: > Yes, that's also my concern when I open this issue, I think it is ok to > keep your current logic. > > The logic of checking `containerizer.empty()` should be removed?
We could remove that but the idea is that the block can be cleanly separated out now to a method (say `handleContainerizer`) that handles the containerizers. - Jojy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44934/#review123991 ----------------------------------------------------------- On March 17, 2016, 6:54 a.m., Jojy Varghese wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44934/ > ----------------------------------------------------------- > > (Updated March 17, 2016, 6:54 a.m.) > > > Review request for mesos and Jie Yu. > > > Repository: mesos > > > Description > ------- > > Updated mesos-execute to add support for Appc. > > > Diffs > ----- > > src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e > > Diff: https://reviews.apache.org/r/44934/diff/ > > > Testing > ------- > > Tested with various Appc images. > > > Thanks, > > Jojy Varghese > >
