You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Ben Mahler <be...@gmail.com> on 2015/10/15 21:17:57 UTC
Review Request 39353: Fixed and added tests for docker image name
parsing.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39353/
-----------------------------------------------------------
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
Re: Review Request 39353: Fixed and added tests for docker image name
parsing.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39353/#review102836
-----------------------------------------------------------
Patch looks great!
Reviews applied: [39353]
All tests passed.
- Mesos ReviewBot
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
>
>
Re: Review Request 39353: Fixed and added tests for docker image name
parsing.
Posted by Gilbert Song <gi...@mesoshere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39353/#review102839
-----------------------------------------------------------
Ship it!
Ship It!
- Gilbert Song
On Oct. 15, 2015, 12: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, 12: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
>
>
Re: Review Request 39353: Fixed and added tests for docker image name
parsing.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39353/#review103020
-----------------------------------------------------------
Thanks for adding this! Are you looking to also work on docker store stuff?
- Timothy Chen
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
>
>
Re: Review Request 39353: Fixed and added tests for docker image name
parsing.
Posted by Ben Mahler <be...@gmail.com>.
> 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?
Agreed, did you see the TODO..?
```
// TODO(bmahler): Validate based on docker's validation logic
// and return a Try here.
```
> 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 ?
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.
- Ben
-----------------------------------------------------------
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
>
>
Re: Review Request 39353: Fixed and added tests for docker image name
parsing.
Posted by Jojy Varghese <jo...@mesosphere.io>.
> 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
>
>
Re: Review Request 39353: Fixed and added tests for docker image name
parsing.
Posted by Ben Mahler <be...@gmail.com>.
> 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.
>
> Jojy Varghese wrote:
> Agreed. What do you think about using a RE parser?
Hm.. worth considering for sure, the ambiguity around whether the first component is a registry or a repository component might make it tricky..
- Ben
-----------------------------------------------------------
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
>
>
Re: Review Request 39353: Fixed and added tests for docker image name
parsing.
Posted by Jojy Varghese <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/39353/#review102816
-----------------------------------------------------------
src/slave/containerizer/provisioner/docker/message.hpp (line 46)
<https://reviews.apache.org/r/39353/#comment160586>
How about handling parse errors ? Maybe change this to a Try?
src/slave/containerizer/provisioner/docker/message.hpp (line 51)
<https://reviews.apache.org/r/39353/#comment160592>
Would this also allow @@ or @@@ ? Wondering if we can use a regular expression parser for parsing ?
- Jojy Varghese
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
>
>