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 2016/12/03 01:50:59 UTC
Re: Review Request 52349: Add protobuf messages for OCI image spec.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52349/
-----------------------------------------------------------
(Updated Dec. 3, 2016, 9:50 a.m.)
Review request for mesos and Jie Yu.
Bugs: MESOS-6681
https://issues.apache.org/jira/browse/MESOS-6681
Repository: mesos
Description
-------
Add protobuf messages for OCI image spec.
Diffs
-----
include/mesos/oci/spec.hpp PRE-CREATION
include/mesos/oci/spec.proto PRE-CREATION
src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb
Diff: https://reviews.apache.org/r/52349/diff/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 52349: Add protobuf messages for OCI image spec.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52349/#review158194
-----------------------------------------------------------
include/mesos/oci/spec.proto (line 24)
<https://reviews.apache.org/r/52349/#comment228961>
Might want to add a comment highlgihting the possible media types:
https://github.com/opencontainers/image-spec/blob/master/media-types.md
include/mesos/oci/spec.proto (line 59)
<https://reviews.apache.org/r/52349/#comment228962>
This represents a media-type. In the comments we should highlight the OCI MIME type of this message. Will be easier for readers to correlate the message.
include/mesos/oci/spec.proto (line 93)
<https://reviews.apache.org/r/52349/#comment228963>
Ditto on highlighting the OCI MIME type.
include/mesos/oci/spec.proto (line 106)
<https://reviews.apache.org/r/52349/#comment228964>
Ditto on highlighting the OCI MIME type.
- Avinash sridharan
On Dec. 3, 2016, 1:50 a.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52349/
> -----------------------------------------------------------
>
> (Updated Dec. 3, 2016, 1:50 a.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add protobuf messages for OCI image spec.
>
>
> Diffs
> -----
>
> include/mesos/oci/spec.hpp PRE-CREATION
> include/mesos/oci/spec.proto PRE-CREATION
> src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb
>
> Diff: https://reviews.apache.org/r/52349/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 52349: Add protobuf messages for OCI image spec.
Posted by Jie Yu <yu...@gmail.com>.
> On Dec. 16, 2016, 12:32 a.m., Jie Yu wrote:
> > include/mesos/oci/spec.proto, line 64
> > <https://reviews.apache.org/r/52349/diff/6/?file=1581544#file1581544line64>
> >
> > No need for the `Image` prefix here.
>
> Qian Zhang wrote:
> Do you mean we name it as `ManifestList`? But it seems not consistent with what we did for Appc and Docker, we have the name like `ImageManifest` in both Appc spec and Docker and spec.
image is in the package namespace already. We don't do that for docker or appc
> On Dec. 16, 2016, 12:32 a.m., Jie Yu wrote:
> > include/mesos/oci/spec.proto, line 90
> > <https://reviews.apache.org/r/52349/diff/6/?file=1581544#file1581544line90>
> >
> > How do we support parsing `annotations`?
> >
> > Should we use a different name so we can do manual parsing?
> >
> > Also, i'd like to introduce a common `Label` message instead of `EntryWithStringValue`
>
> Qian Zhang wrote:
> > How do we support parsing `annotations`?
>
> That's the suggested way to handle `map` in protobuf, see https://developers.google.com/protocol-buffers/docs/proto#maps (the backwards compatible way) for details, I think `annotations` will be automatically parsed in this way.
>
> > Also, i'd like to introduce a common `Label` message instead of `EntryWithStringValue`
>
> Do you mean we rename `EntryWithStringValue` to `Label` and define it in a common place rather than in this file?
Ah, didn't realize that Map is already supported in proto2. We definitely need to add parsing support for that (i.e., json -> protobuf). Not sure how backwards compatibility will be handled. However, if it is used in agent only, it should not be a problem.
- Jie
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52349/#review159391
-----------------------------------------------------------
On Dec. 11, 2016, 2:15 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52349/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2016, 2:15 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add protobuf messages for OCI image spec.
>
>
> Diffs
> -----
>
> include/mesos/oci/spec.hpp PRE-CREATION
> include/mesos/oci/spec.proto PRE-CREATION
> src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47
>
> Diff: https://reviews.apache.org/r/52349/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 52349: Add protobuf messages for OCI image spec.
Posted by Qian Zhang <zh...@gmail.com>.
> On Dec. 16, 2016, 8:32 a.m., Jie Yu wrote:
> > include/mesos/oci/spec.proto, line 90
> > <https://reviews.apache.org/r/52349/diff/6/?file=1581544#file1581544line90>
> >
> > How do we support parsing `annotations`?
> >
> > Should we use a different name so we can do manual parsing?
> >
> > Also, i'd like to introduce a common `Label` message instead of `EntryWithStringValue`
>
> Qian Zhang wrote:
> > How do we support parsing `annotations`?
>
> That's the suggested way to handle `map` in protobuf, see https://developers.google.com/protocol-buffers/docs/proto#maps (the backwards compatible way) for details, I think `annotations` will be automatically parsed in this way.
>
> > Also, i'd like to introduce a common `Label` message instead of `EntryWithStringValue`
>
> Do you mean we rename `EntryWithStringValue` to `Label` and define it in a common place rather than in this file?
>
> Jie Yu wrote:
> Ah, didn't realize that Map is already supported in proto2. We definitely need to add parsing support for that (i.e., json -> protobuf). Not sure how backwards compatibility will be handled. However, if it is used in agent only, it should not be a problem.
The protobuf that we are using (protobuf-2.6.1) does not support `map`, as MPark mentioned in the dev list, we need to upgrade to protobuf 3.0.0 to have `map` support. Do you think we need to do the upgrade right away, or we can use the backwards compatible way mentioned in the above link first in this project and do the upgrade later?
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52349/#review159391
-----------------------------------------------------------
On Dec. 11, 2016, 10:15 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52349/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2016, 10:15 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add protobuf messages for OCI image spec.
>
>
> Diffs
> -----
>
> include/mesos/oci/spec.hpp PRE-CREATION
> include/mesos/oci/spec.proto PRE-CREATION
> src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47
>
> Diff: https://reviews.apache.org/r/52349/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 52349: Add protobuf messages for OCI image spec.
Posted by Qian Zhang <zh...@gmail.com>.
> On Dec. 16, 2016, 8:32 a.m., Jie Yu wrote:
> > include/mesos/oci/spec.proto, line 90
> > <https://reviews.apache.org/r/52349/diff/6/?file=1581544#file1581544line90>
> >
> > How do we support parsing `annotations`?
> >
> > Should we use a different name so we can do manual parsing?
> >
> > Also, i'd like to introduce a common `Label` message instead of `EntryWithStringValue`
>
> Qian Zhang wrote:
> > How do we support parsing `annotations`?
>
> That's the suggested way to handle `map` in protobuf, see https://developers.google.com/protocol-buffers/docs/proto#maps (the backwards compatible way) for details, I think `annotations` will be automatically parsed in this way.
>
> > Also, i'd like to introduce a common `Label` message instead of `EntryWithStringValue`
>
> Do you mean we rename `EntryWithStringValue` to `Label` and define it in a common place rather than in this file?
>
> Jie Yu wrote:
> Ah, didn't realize that Map is already supported in proto2. We definitely need to add parsing support for that (i.e., json -> protobuf). Not sure how backwards compatibility will be handled. However, if it is used in agent only, it should not be a problem.
>
> Qian Zhang wrote:
> The protobuf that we are using (protobuf-2.6.1) does not support `map`, as MPark mentioned in the dev list, we need to upgrade to protobuf 3.0.0 to have `map` support. Do you think we need to do the upgrade right away, or we can use the backwards compatible way mentioned in the above link first in this project and do the upgrade later?
Let's follow the similar way of https://reviews.apache.org/r/47199/ to do the manual parsing for now, and in future when we upgrade the protobuf to a newer version in Mesos to support `Map`, we can remove that manual parsing logic and just use the `Map` keyword.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52349/#review159391
-----------------------------------------------------------
On Jan. 3, 2017, 9:53 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52349/
> -----------------------------------------------------------
>
> (Updated Jan. 3, 2017, 9:53 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add protobuf messages for OCI image spec.
>
>
> Diffs
> -----
>
> include/mesos/oci/spec.hpp PRE-CREATION
> include/mesos/oci/spec.proto PRE-CREATION
> src/Makefile.am 6d0f77be37af9bc4e22199418796d6d0c5b6c462
>
> Diff: https://reviews.apache.org/r/52349/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 52349: Add protobuf messages for OCI image spec.
Posted by Qian Zhang <zh...@gmail.com>.
> On Dec. 16, 2016, 8:32 a.m., Jie Yu wrote:
> > include/mesos/oci/spec.proto, line 64
> > <https://reviews.apache.org/r/52349/diff/6/?file=1581544#file1581544line64>
> >
> > No need for the `Image` prefix here.
Do you mean we name it as `ManifestList`? But it seems not consistent with what we did for Appc and Docker, we have the name like `ImageManifest` in both Appc spec and Docker and spec.
> On Dec. 16, 2016, 8:32 a.m., Jie Yu wrote:
> > include/mesos/oci/spec.proto, line 90
> > <https://reviews.apache.org/r/52349/diff/6/?file=1581544#file1581544line90>
> >
> > How do we support parsing `annotations`?
> >
> > Should we use a different name so we can do manual parsing?
> >
> > Also, i'd like to introduce a common `Label` message instead of `EntryWithStringValue`
> How do we support parsing `annotations`?
That's the suggested way to handle `map` in protobuf, see https://developers.google.com/protocol-buffers/docs/proto#maps (the backwards compatible way) for details, I think `annotations` will be automatically parsed in this way.
> Also, i'd like to introduce a common `Label` message instead of `EntryWithStringValue`
Do you mean we rename `EntryWithStringValue` to `Label` and define it in a common place rather than in this file?
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52349/#review159391
-----------------------------------------------------------
On Dec. 11, 2016, 10:15 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52349/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2016, 10:15 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add protobuf messages for OCI image spec.
>
>
> Diffs
> -----
>
> include/mesos/oci/spec.hpp PRE-CREATION
> include/mesos/oci/spec.proto PRE-CREATION
> src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47
>
> Diff: https://reviews.apache.org/r/52349/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 52349: Add protobuf messages 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/52349/#review159391
-----------------------------------------------------------
include/mesos/oci/spec.proto (line 17)
<https://reviews.apache.org/r/52349/#comment230386>
+1 using oci.spec.image.
Also, looks like they start to prefix with v1. Let's use that as well:
```
oci.spec.image.v1
```
include/mesos/oci/spec.proto (line 64)
<https://reviews.apache.org/r/52349/#comment230387>
No need for the `Image` prefix here.
include/mesos/oci/spec.proto (line 90)
<https://reviews.apache.org/r/52349/#comment230384>
How do we support parsing `annotations`?
Should we use a different name so we can do manual parsing?
Also, i'd like to introduce a common `Label` message instead of `EntryWithStringValue`
- Jie Yu
On Dec. 11, 2016, 2:15 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52349/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2016, 2:15 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add protobuf messages for OCI image spec.
>
>
> Diffs
> -----
>
> include/mesos/oci/spec.hpp PRE-CREATION
> include/mesos/oci/spec.proto PRE-CREATION
> src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47
>
> Diff: https://reviews.apache.org/r/52349/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 52349: Add protobuf messages 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/52349/
-----------------------------------------------------------
(Updated Jan. 3, 2017, 9:53 p.m.)
Review request for mesos and Jie Yu.
Changes
-------
Addressed Jie and Avinash's comments.
Bugs: MESOS-6681
https://issues.apache.org/jira/browse/MESOS-6681
Repository: mesos
Description
-------
Add protobuf messages for OCI image spec.
Diffs (updated)
-----
include/mesos/oci/spec.hpp PRE-CREATION
include/mesos/oci/spec.proto PRE-CREATION
src/Makefile.am 6d0f77be37af9bc4e22199418796d6d0c5b6c462
Diff: https://reviews.apache.org/r/52349/diff/
Testing
-------
Thanks,
Qian Zhang
Re: Review Request 52349: Add protobuf messages for OCI image spec.
Posted by Avinash sridharan <av...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/52349/#review158819
-----------------------------------------------------------
Fix it, then Ship it!
Ship It!
include/mesos/oci/spec.proto (line 17)
<https://reviews.apache.org/r/52349/#comment229635>
Should have pointed this out earlier. Sorry. We probably should have `oci.spec.image` since these protobufs are specific to the image specification.
Not sure we will have protobufs for the run-time spec but I think it might be helpful to explicit about the namespaces here since the specifications for image and run-time are distinct.
This would also imply that we might want to change the name of the files? Maybe spec_image.proto
- Avinash sridharan
On Dec. 11, 2016, 2:15 p.m., Qian Zhang wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52349/
> -----------------------------------------------------------
>
> (Updated Dec. 11, 2016, 2:15 p.m.)
>
>
> Review request for mesos and Jie Yu.
>
>
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add protobuf messages for OCI image spec.
>
>
> Diffs
> -----
>
> include/mesos/oci/spec.hpp PRE-CREATION
> include/mesos/oci/spec.proto PRE-CREATION
> src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47
>
> Diff: https://reviews.apache.org/r/52349/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Qian Zhang
>
>
Re: Review Request 52349: Add protobuf messages 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/52349/
-----------------------------------------------------------
(Updated Dec. 11, 2016, 10:15 p.m.)
Review request for mesos and Jie Yu.
Changes
-------
Addressed Avinash's commentes.
Bugs: MESOS-6681
https://issues.apache.org/jira/browse/MESOS-6681
Repository: mesos
Description
-------
Add protobuf messages for OCI image spec.
Diffs (updated)
-----
include/mesos/oci/spec.hpp PRE-CREATION
include/mesos/oci/spec.proto PRE-CREATION
src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47
Diff: https://reviews.apache.org/r/52349/diff/
Testing
-------
Thanks,
Qian Zhang