You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Qian Zhang <zh...@gmail.com> on 2017/01/03 14:11:20 UTC
Review Request 55139: Implemented parse methods for OCI image spec.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55139/
-----------------------------------------------------------
Review request for mesos, Gilbert Song and Jie Yu.
Bugs: MESOS-6681
https://issues.apache.org/jira/browse/MESOS-6681
Repository: mesos
Description
-------
Implemented parse methods for OCI image spec.
Diffs
-----
include/mesos/oci/spec.hpp PRE-CREATION
src/CMakeLists.txt c8d4260c03d8cdee1951a50d293e9fdabcd2cf84
src/Makefile.am 6d0f77be37af9bc4e22199418796d6d0c5b6c462
src/oci/spec.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/55139/diff/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 55139: Implemented parse methods for OCI image
spec.
Posted by Qian Zhang <zh...@gmail.com>.
> On Feb. 5, 2017, 12:02 p.m., Jie Yu wrote:
> > src/oci/spec.cpp, lines 163-166
> > <https://reviews.apache.org/r/55139/diff/3/?file=1621004#file1621004line163>
> >
> > what if `digest.isNone()`?
`digest` is a required field, so it can not be `None()` here.
> On Feb. 5, 2017, 12:02 p.m., Jie Yu wrote:
> > src/oci/spec.cpp, lines 182-185
> > <https://reviews.apache.org/r/55139/diff/3/?file=1621004#file1621004line182>
> >
> > What if `platform.isNone()`?
Ditto.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55139/#review164262
-----------------------------------------------------------
On Feb. 1, 2017, 9:13 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55139/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2017, 9:13 a.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented parse methods for OCI image spec.
>
>
> Diffs
> -----
>
> include/mesos/oci/spec.hpp PRE-CREATION
> src/CMakeLists.txt 09ef1aee680c6b91476092dbf61d4ca3a8d256bb
> src/Makefile.am 6c9be54fe0ce3faaa26e2f090773d1b77bb6e430
> src/oci/spec.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/55139/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 55139: Implemented parse methods for OCI image
spec.
Posted by Jie Yu <yu...@gmail.com>.
> On Feb. 5, 2017, 4:02 a.m., Jie Yu wrote:
> > src/oci/spec.cpp, lines 163-166
> > <https://reviews.apache.org/r/55139/diff/3/?file=1621004#file1621004line163>
> >
> > what if `digest.isNone()`?
>
> Qian Zhang wrote:
> `digest` is a required field, so it can not be `None()` here.
I'd prefer to be a bit more defensive here, and self contained. Either add a CHECK here, or doing explicit check of `isNone()`. I prefer the latter.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55139/#review164262
-----------------------------------------------------------
On Feb. 6, 2017, 2:43 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55139/
> -----------------------------------------------------------
>
> (Updated Feb. 6, 2017, 2:43 p.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented parse methods for OCI image spec.
>
>
> Diffs
> -----
>
> include/mesos/oci/spec.hpp PRE-CREATION
> src/CMakeLists.txt c09bcde3c14a73ab0b296c400f79f6a775da2470
> src/Makefile.am 78d55d86b1e1238beda3163d3240c85609ea982d
> src/oci/spec.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/55139/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 55139: Implemented parse methods for OCI image
spec.
Posted by Qian Zhang <zh...@gmail.com>.
> On Feb. 5, 2017, 12:02 p.m., Jie Yu wrote:
> > src/oci/spec.cpp, lines 163-166
> > <https://reviews.apache.org/r/55139/diff/3/?file=1621004#file1621004line163>
> >
> > what if `digest.isNone()`?
>
> Qian Zhang wrote:
> `digest` is a required field, so it can not be `None()` here.
>
> Jie Yu wrote:
> I'd prefer to be a bit more defensive here, and self contained. Either add a CHECK here, or doing explicit check of `isNone()`. I prefer the latter.
OK, let me do an explicit check of `isNone()`.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55139/#review164262
-----------------------------------------------------------
On Feb. 6, 2017, 10:43 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55139/
> -----------------------------------------------------------
>
> (Updated Feb. 6, 2017, 10:43 p.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented parse methods for OCI image spec.
>
>
> Diffs
> -----
>
> include/mesos/oci/spec.hpp PRE-CREATION
> src/CMakeLists.txt c09bcde3c14a73ab0b296c400f79f6a775da2470
> src/Makefile.am 78d55d86b1e1238beda3163d3240c85609ea982d
> src/oci/spec.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/55139/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 55139: Implemented parse methods for OCI image
spec.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55139/#review164262
-----------------------------------------------------------
Fix it, then Ship it!
src/oci/spec.cpp (line 33)
<https://reviews.apache.org/r/55139/#comment235915>
`const string&`
src/oci/spec.cpp (line 76)
<https://reviews.apache.org/r/55139/#comment235921>
insert extra blank line above.
src/oci/spec.cpp (line 156)
<https://reviews.apache.org/r/55139/#comment235918>
Any reason you need this temp var?
src/oci/spec.cpp (lines 163 - 166)
<https://reviews.apache.org/r/55139/#comment235924>
what if `digest.isNone()`?
src/oci/spec.cpp (lines 182 - 185)
<https://reviews.apache.org/r/55139/#comment235923>
What if `platform.isNone()`?
src/oci/spec.cpp (line 226)
<https://reviews.apache.org/r/55139/#comment235925>
replace .get() with `->`
src/oci/spec.cpp (line 271)
<https://reviews.apache.org/r/55139/#comment235920>
You can use `!annotations->is<JSON::Null>()`
src/oci/spec.cpp (line 319)
<https://reviews.apache.org/r/55139/#comment235926>
kill this line
src/oci/spec.cpp (line 325)
<https://reviews.apache.org/r/55139/#comment235927>
no need for this temp var?
src/oci/spec.cpp (line 338)
<https://reviews.apache.org/r/55139/#comment235928>
Ditto. No need for this temp var.
src/oci/spec.cpp (line 351)
<https://reviews.apache.org/r/55139/#comment235929>
No need for this temp var?
- Jie Yu
On Feb. 1, 2017, 1:13 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55139/
> -----------------------------------------------------------
>
> (Updated Feb. 1, 2017, 1:13 a.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented parse methods for OCI image spec.
>
>
> Diffs
> -----
>
> include/mesos/oci/spec.hpp PRE-CREATION
> src/CMakeLists.txt 09ef1aee680c6b91476092dbf61d4ca3a8d256bb
> src/Makefile.am 6c9be54fe0ce3faaa26e2f090773d1b77bb6e430
> src/oci/spec.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/55139/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 55139: Implemented parse methods for OCI image
spec.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55139/
-----------------------------------------------------------
(Updated Feb. 7, 2017, 4:41 p.m.)
Review request for mesos, Gilbert Song and Jie Yu.
Changes
-------
Addressed Jie's comments.
Bugs: MESOS-6681
https://issues.apache.org/jira/browse/MESOS-6681
Repository: mesos
Description
-------
Implemented parse methods for OCI image spec.
Diffs (updated)
-----
include/mesos/oci/spec.hpp PRE-CREATION
src/CMakeLists.txt c09bcde3c14a73ab0b296c400f79f6a775da2470
src/Makefile.am 78d55d86b1e1238beda3163d3240c85609ea982d
src/oci/spec.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/55139/diff/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 55139: Implemented parse methods for OCI image
spec.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55139/
-----------------------------------------------------------
(Updated Feb. 6, 2017, 10:43 p.m.)
Review request for mesos, Gilbert Song and Jie Yu.
Changes
-------
Addressed Jie's comments.
Bugs: MESOS-6681
https://issues.apache.org/jira/browse/MESOS-6681
Repository: mesos
Description
-------
Implemented parse methods for OCI image spec.
Diffs (updated)
-----
include/mesos/oci/spec.hpp PRE-CREATION
src/CMakeLists.txt c09bcde3c14a73ab0b296c400f79f6a775da2470
src/Makefile.am 78d55d86b1e1238beda3163d3240c85609ea982d
src/oci/spec.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/55139/diff/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 55139: Implemented parse methods for OCI image
spec.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55139/
-----------------------------------------------------------
(Updated Feb. 1, 2017, 9:13 a.m.)
Review request for mesos, Gilbert Song and Jie Yu.
Changes
-------
Added internal validate method.
Bugs: MESOS-6681
https://issues.apache.org/jira/browse/MESOS-6681
Repository: mesos
Description
-------
Implemented parse methods for OCI image spec.
Diffs (updated)
-----
include/mesos/oci/spec.hpp PRE-CREATION
src/CMakeLists.txt 09ef1aee680c6b91476092dbf61d4ca3a8d256bb
src/Makefile.am 6c9be54fe0ce3faaa26e2f090773d1b77bb6e430
src/oci/spec.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/55139/diff/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 55139: Implemented parse methods for OCI image
spec.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55139/
-----------------------------------------------------------
(Updated Jan. 9, 2017, 3:25 p.m.)
Review request for mesos, Gilbert Song and Jie Yu.
Changes
-------
Addressed Jie's comments.
Bugs: MESOS-6681
https://issues.apache.org/jira/browse/MESOS-6681
Repository: mesos
Description
-------
Implemented parse methods for OCI image spec.
Diffs (updated)
-----
include/mesos/oci/spec.hpp PRE-CREATION
src/CMakeLists.txt c8d4260c03d8cdee1951a50d293e9fdabcd2cf84
src/Makefile.am 6d0f77be37af9bc4e22199418796d6d0c5b6c462
src/oci/spec.cpp PRE-CREATION
Diff: https://reviews.apache.org/r/55139/diff/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 55139: Implemented parse methods for OCI image
spec.
Posted by Jie Yu <yu...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55139/#review160843
-----------------------------------------------------------
include/mesos/oci/spec.hpp (lines 27 - 48)
<https://reviews.apache.org/r/55139/#comment232080>
I would use template and template specification here:
```
template <typename T>
Try<T> parse(const string& s);
template <>
Try<Descriptor> parse(const string& s);
...
```
At callsite, you can do:
```
Try<Descriptor> descriptor = parse<Descriptor>(s);
```
- Jie Yu
On Jan. 3, 2017, 2:11 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55139/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2017, 2:11 p.m.)
>
>
> Review request for mesos, Gilbert Song and Jie Yu.
>
>
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Implemented parse methods for OCI image spec.
>
>
> Diffs
> -----
>
> include/mesos/oci/spec.hpp PRE-CREATION
> src/CMakeLists.txt c8d4260c03d8cdee1951a50d293e9fdabcd2cf84
> src/Makefile.am 6d0f77be37af9bc4e22199418796d6d0c5b6c462
> src/oci/spec.cpp PRE-CREATION
>
> Diff: https://reviews.apache.org/r/55139/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>