> On Oct. 15, 2015, 8:28 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioner/docker/message.hpp, line 46 > > <https://reviews.apache.org/r/39353/diff/1/?file=1099046#file1099046line46> > > > > How about handling parse errors ? Maybe change this to a Try? > > Ben Mahler wrote: > Agreed, did you see the TODO..? > > ``` > // TODO(bmahler): Validate based on docker's validation logic > // and return a Try here. > ```
Ah my bad. > On Oct. 15, 2015, 8:28 p.m., Jojy Varghese wrote: > > src/slave/containerizer/provisioner/docker/message.hpp, line 51 > > <https://reviews.apache.org/r/39353/diff/1/?file=1099046#file1099046line51> > > > > Would this also allow @@ or @@@ ? Wondering if we can use a regular > > expression parser for parsing ? > > Ben Mahler wrote: > Yes, this won't reject invalid input (given this doesn't return a Try). > > The first step (this change) is to fix our code to accept valid input > (which we weren't doing for all cases!). I've left the TODO for validation > and rejecting bad input. Agreed. What do you think about using a RE parser? - Jojy ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/39353/#review102816 ----------------------------------------------------------- On Oct. 15, 2015, 7:17 p.m., Ben Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/39353/ > ----------------------------------------------------------- > > (Updated Oct. 15, 2015, 7:17 p.m.) > > > Review request for mesos, Jojy Varghese and Timothy Chen. > > > Repository: mesos > > > Description > ------- > > The existing docker image parsing did not work in many cases: if a digest is > present, a registry port is included. And it could not resolve the ambiguity > that exists between the registry and the repository. > > This follows docker's approach to parsing, which is a bit hacky due to the > way the registry and repository are disambiguated, see the comments. > > > Diffs > ----- > > include/mesos/mesos.proto f2ea4fc62507ce30b7a7981fd6a8c4749eb97190 > src/slave/containerizer/provisioner/docker/message.hpp > 6368bf4caec6f8c3ac97282f41c55381f920bce9 > src/slave/containerizer/provisioner/docker/message.proto > bbac2e6c1f40a7ca3f9227baca56a44cd43f58c6 > src/slave/containerizer/provisioner/docker/store.cpp > 637c97c0973bda492826803a962c99635647692d > src/tests/containerizer/provisioner_docker_tests.cpp > 9c3c45a81be6398722a37911788e347a4e91cce8 > > Diff: https://reviews.apache.org/r/39353/diff/ > > > Testing > ------- > > Added a test. > > > Thanks, > > Ben Mahler > >
