You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Ian Downes <ia...@gmail.com> on 2015/06/22 18:42:55 UTC
Re: Review Request 34136: Add ContainerImage protobuf.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/
-----------------------------------------------------------
(Updated June 22, 2015, 9:42 a.m.)
Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
Changes
-------
Changed the hierarchy so the ContainerInfo::Image is a union of different types, initially just Appc.
Repository: mesos
Description
-------
Add ContainerImage protobuf.
Diffs (updated)
-----
include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
Diff: https://reviews.apache.org/r/34136/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On June 26, 2015, 2:57 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1212-1214
> > <https://reviews.apache.org/r/34136/diff/2/?file=989752#file989752line1212>
> >
> > Is it the intention that Image type is **defined** outside MesosInfo because DockerInfo can later reference it?
> >
> > Otherwise it feels more natual to define Image within MesosInfo.
>
> Vinod Kone wrote:
> +1 to put it inside MesosInfo.
>
> Ian Downes wrote:
> I wanted it to be outside MesosInfo so it could be used by other container types, i.e., I don't see it as specific to MesosInfo, e.g., we could also support an AppcInfo which contained an Image::APPC.
OK SGTM.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review89577
-----------------------------------------------------------
On July 11, 2015, 9:47 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> -----------------------------------------------------------
>
> (Updated July 11, 2015, 9:47 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ContainerImage protobuf.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6
>
> Diff: https://reviews.apache.org/r/34136/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 26, 2015, 9:57 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1212-1214
> > <https://reviews.apache.org/r/34136/diff/2/?file=989752#file989752line1212>
> >
> > Is it the intention that Image type is **defined** outside MesosInfo because DockerInfo can later reference it?
> >
> > Otherwise it feels more natual to define Image within MesosInfo.
+1 to put it inside MesosInfo.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review89577
-----------------------------------------------------------
On June 22, 2015, 4:42 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 4:42 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ContainerImage protobuf.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
>
> Diff: https://reviews.apache.org/r/34136/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Ian Downes <ia...@gmail.com>.
> On June 26, 2015, 2:57 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1212-1214
> > <https://reviews.apache.org/r/34136/diff/2/?file=989752#file989752line1212>
> >
> > Is it the intention that Image type is **defined** outside MesosInfo because DockerInfo can later reference it?
> >
> > Otherwise it feels more natual to define Image within MesosInfo.
>
> Vinod Kone wrote:
> +1 to put it inside MesosInfo.
I wanted it to be outside MesosInfo so it could be used by other container types, i.e., I don't see it as specific to MesosInfo, e.g., we could also support an AppcInfo which contained an Image::APPC.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review89577
-----------------------------------------------------------
On June 22, 2015, 9:42 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 9:42 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ContainerImage protobuf.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
>
> Diff: https://reviews.apache.org/r/34136/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review89577
-----------------------------------------------------------
include/mesos/mesos.proto (lines 1212 - 1214)
<https://reviews.apache.org/r/34136/#comment142204>
Is it the intention that Image type is **defined** outside MesosInfo because DockerInfo can later reference it?
Otherwise it feels more natual to define Image within MesosInfo.
- Jiang Yan Xu
On June 22, 2015, 9:42 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 9:42 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ContainerImage protobuf.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
>
> Diff: https://reviews.apache.org/r/34136/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Ian Downes <ia...@gmail.com>.
> On June 25, 2015, 3:32 p.m., Paul Brett wrote:
> > include/mesos/mesos.proto, line 1221
> > <https://reviews.apache.org/r/34136/diff/2/?file=989752#file989752line1221>
> >
> > Mesos info is optional, but if present can optionally contain an image? So what does a mesos present with a MesosInfo message with no image mean?
>
> Vinod Kone wrote:
> yea. why optional?
>
> Timothy Chen wrote:
> I think image can be optional which bypasses the image provisioning. MesosInfo potentially will contain other information like what DockerInfo does that is MesosContainerizer specific, so I think this API makes sense to me.
Yep, Tim's comments reflect what I'm thinking.
> On June 25, 2015, 3:32 p.m., Paul Brett wrote:
> > include/mesos/mesos.proto, line 1202
> > <https://reviews.apache.org/r/34136/diff/2/?file=989752#file989752line1202>
> >
> > Not really an id, more of a signature for the image. Would we be better making this optional string sha512hash to allow for the possibility that a different signature might be used in the future?
>
> Vinod Kone wrote:
> i'm assuming this is because appc calls it an image id.
Correct, this reflects naming in the appc spec.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review89438
-----------------------------------------------------------
On June 22, 2015, 9:42 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 9:42 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ContainerImage protobuf.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
>
> Diff: https://reviews.apache.org/r/34136/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Timothy Chen <tn...@apache.org>.
> On June 25, 2015, 10:32 p.m., Paul Brett wrote:
> > include/mesos/mesos.proto, line 1221
> > <https://reviews.apache.org/r/34136/diff/2/?file=989752#file989752line1221>
> >
> > Mesos info is optional, but if present can optionally contain an image? So what does a mesos present with a MesosInfo message with no image mean?
>
> Vinod Kone wrote:
> yea. why optional?
I think image can be optional which bypasses the image provisioning. MesosInfo potentially will contain other information like what DockerInfo does that is MesosContainerizer specific, so I think this API makes sense to me.
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review89438
-----------------------------------------------------------
On June 22, 2015, 4:42 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 4:42 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ContainerImage protobuf.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
>
> Diff: https://reviews.apache.org/r/34136/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Vinod Kone <vi...@gmail.com>.
> On June 25, 2015, 10:32 p.m., Paul Brett wrote:
> > include/mesos/mesos.proto, line 1221
> > <https://reviews.apache.org/r/34136/diff/2/?file=989752#file989752line1221>
> >
> > Mesos info is optional, but if present can optionally contain an image? So what does a mesos present with a MesosInfo message with no image mean?
yea. why optional?
> On June 25, 2015, 10:32 p.m., Paul Brett wrote:
> > include/mesos/mesos.proto, line 1202
> > <https://reviews.apache.org/r/34136/diff/2/?file=989752#file989752line1202>
> >
> > Not really an id, more of a signature for the image. Would we be better making this optional string sha512hash to allow for the possibility that a different signature might be used in the future?
i'm assuming this is because appc calls it an image id.
- Vinod
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review89438
-----------------------------------------------------------
On June 22, 2015, 4:42 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 4:42 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ContainerImage protobuf.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
>
> Diff: https://reviews.apache.org/r/34136/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Paul Brett <pa...@twopensource.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review89438
-----------------------------------------------------------
include/mesos/mesos.proto (line 1196)
<https://reviews.apache.org/r/34136/#comment142070>
I assume that much of this is prescribed by the AppC specification - can we reference it?
include/mesos/mesos.proto (line 1202)
<https://reviews.apache.org/r/34136/#comment142067>
Not really an id, more of a signature for the image. Would we be better making this optional string sha512hash to allow for the possibility that a different signature might be used in the future?
include/mesos/mesos.proto (line 1221)
<https://reviews.apache.org/r/34136/#comment142073>
Mesos info is optional, but if present can optionally contain an image? So what does a mesos present with a MesosInfo message with no image mean?
- Paul Brett
On June 22, 2015, 4:42 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 4:42 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ContainerImage protobuf.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
>
> Diff: https://reviews.apache.org/r/34136/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review93481
-----------------------------------------------------------
include/mesos/mesos.proto (line 1213)
<https://reviews.apache.org/r/34136/#comment147861>
Hmm... I don't think this should be required. It's too inflexible and tasks likely will use name and labels.
- Jiang Yan Xu
On July 11, 2015, 9:47 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> -----------------------------------------------------------
>
> (Updated July 11, 2015, 9:47 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ContainerImage protobuf.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6
>
> Diff: https://reviews.apache.org/r/34136/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Timothy Chen <tn...@apache.org>.
> On July 14, 2015, 9:03 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1211-1213
> > <https://reviews.apache.org/r/34136/diff/3/?file=1009139#file1009139line1211>
> >
> > So I found the use of the field `id` inconsistent in the code.
> >
> > Sometimes `id` has the `sha512-` prefix and sometimes not.
> >
> > I think we should consistently refer to `id` using the definition in the [spec](https://github.com/appc/spec/blob/806b17c86ba5e5d595fca3f7ed339c8a22fb46c3/spec/aci.md#image-id), i.e., with the prefix.
> >
> > The fact that the ID is computed by the image creator using sha512 and that the provisioner validates it using sha512 is merely an implementation detail that is not a conern of higher level abstractions / APIs.
> >
> > So here I think in the comments we should not call it "Image hash" but rather refer to the spec for its full definition. We can of course call out the fact that it should have the "sha512-" perfix.
> >
> > What do you think?
>
> Timothy Chen wrote:
> Hi there,
> I'm going to commit this for Ian and just saw your comment.
> How about I reword the comment here to "// The ID of the Image. Please refer to the Appc spec for its definition."?
Actually I mis-read what you meant, how about:
// The ID of the Image.
// An image ID is canonically represented as a string prefixed by
// the algorithm used and the hash output (e.g. sha512-a83...).
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review91666
-----------------------------------------------------------
On July 12, 2015, 4:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ContainerImage protobuf.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6
>
> Diff: https://reviews.apache.org/r/34136/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Timothy Chen <tn...@apache.org>.
> On July 14, 2015, 9:03 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1211-1213
> > <https://reviews.apache.org/r/34136/diff/3/?file=1009139#file1009139line1211>
> >
> > So I found the use of the field `id` inconsistent in the code.
> >
> > Sometimes `id` has the `sha512-` prefix and sometimes not.
> >
> > I think we should consistently refer to `id` using the definition in the [spec](https://github.com/appc/spec/blob/806b17c86ba5e5d595fca3f7ed339c8a22fb46c3/spec/aci.md#image-id), i.e., with the prefix.
> >
> > The fact that the ID is computed by the image creator using sha512 and that the provisioner validates it using sha512 is merely an implementation detail that is not a conern of higher level abstractions / APIs.
> >
> > So here I think in the comments we should not call it "Image hash" but rather refer to the spec for its full definition. We can of course call out the fact that it should have the "sha512-" perfix.
> >
> > What do you think?
Hi there,
I'm going to commit this for Ian and just saw your comment.
How about I reword the comment here to "// The ID of the Image. Please refer to the Appc spec for its definition."?
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review91666
-----------------------------------------------------------
On July 12, 2015, 4:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ContainerImage protobuf.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6
>
> Diff: https://reviews.apache.org/r/34136/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Jiang Yan Xu <ya...@jxu.me>.
> On July 14, 2015, 2:03 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1211-1213
> > <https://reviews.apache.org/r/34136/diff/3/?file=1009139#file1009139line1211>
> >
> > So I found the use of the field `id` inconsistent in the code.
> >
> > Sometimes `id` has the `sha512-` prefix and sometimes not.
> >
> > I think we should consistently refer to `id` using the definition in the [spec](https://github.com/appc/spec/blob/806b17c86ba5e5d595fca3f7ed339c8a22fb46c3/spec/aci.md#image-id), i.e., with the prefix.
> >
> > The fact that the ID is computed by the image creator using sha512 and that the provisioner validates it using sha512 is merely an implementation detail that is not a conern of higher level abstractions / APIs.
> >
> > So here I think in the comments we should not call it "Image hash" but rather refer to the spec for its full definition. We can of course call out the fact that it should have the "sha512-" perfix.
> >
> > What do you think?
>
> Timothy Chen wrote:
> Hi there,
> I'm going to commit this for Ian and just saw your comment.
> How about I reword the comment here to "// The ID of the Image. Please refer to the Appc spec for its definition."?
>
> Timothy Chen wrote:
> Actually I mis-read what you meant, how about:
>
> // The ID of the Image.
> // An image ID is canonically represented as a string prefixed by
> // the algorithm used and the hash output (e.g. sha512-a83...).
Or we could just copy the definition here verbatim. :)
https://github.com/appc/spec/blob/master/spec/types.md#image-id-type
An image ID is a string of the format "hash-value", where "hash" is the hash algorithm used and "value" is the hex encoded string of the digest. Currently the only permitted hash algorithm is sha512.
Thanks, please commit it!
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review91666
-----------------------------------------------------------
On July 11, 2015, 9:47 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> -----------------------------------------------------------
>
> (Updated July 11, 2015, 9:47 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ContainerImage protobuf.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6
>
> Diff: https://reviews.apache.org/r/34136/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Timothy Chen <tn...@apache.org>.
> On July 14, 2015, 9:03 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1211-1213
> > <https://reviews.apache.org/r/34136/diff/3/?file=1009139#file1009139line1211>
> >
> > So I found the use of the field `id` inconsistent in the code.
> >
> > Sometimes `id` has the `sha512-` prefix and sometimes not.
> >
> > I think we should consistently refer to `id` using the definition in the [spec](https://github.com/appc/spec/blob/806b17c86ba5e5d595fca3f7ed339c8a22fb46c3/spec/aci.md#image-id), i.e., with the prefix.
> >
> > The fact that the ID is computed by the image creator using sha512 and that the provisioner validates it using sha512 is merely an implementation detail that is not a conern of higher level abstractions / APIs.
> >
> > So here I think in the comments we should not call it "Image hash" but rather refer to the spec for its full definition. We can of course call out the fact that it should have the "sha512-" perfix.
> >
> > What do you think?
>
> Timothy Chen wrote:
> Hi there,
> I'm going to commit this for Ian and just saw your comment.
> How about I reword the comment here to "// The ID of the Image. Please refer to the Appc spec for its definition."?
>
> Timothy Chen wrote:
> Actually I mis-read what you meant, how about:
>
> // The ID of the Image.
> // An image ID is canonically represented as a string prefixed by
> // the algorithm used and the hash output (e.g. sha512-a83...).
>
> Jiang Yan Xu wrote:
> Or we could just copy the definition here verbatim. :)
>
> https://github.com/appc/spec/blob/master/spec/types.md#image-id-type
>
> An image ID is a string of the format "hash-value", where "hash" is the hash algorithm used and "value" is the hex encoded string of the digest. Currently the only permitted hash algorithm is sha512.
>
> Thanks, please commit it!
Thanks!
- Timothy
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review91666
-----------------------------------------------------------
On July 12, 2015, 4:47 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> -----------------------------------------------------------
>
> (Updated July 12, 2015, 4:47 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ContainerImage protobuf.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6
>
> Diff: https://reviews.apache.org/r/34136/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review91666
-----------------------------------------------------------
Sorry I didn't notice it in my original review but I traced the issue back from future reviews.
include/mesos/mesos.proto (lines 1211 - 1213)
<https://reviews.apache.org/r/34136/#comment145213>
So I found the use of the field `id` inconsistent in the code.
Sometimes `id` has the `sha512-` prefix and sometimes not.
I think we should consistently refer to `id` using the definition in the [spec](https://github.com/appc/spec/blob/806b17c86ba5e5d595fca3f7ed339c8a22fb46c3/spec/aci.md#image-id), i.e., with the prefix.
The fact that the ID is computed by the image creator using sha512 and that the provisioner validates it using sha512 is merely an implementation detail that is not a conern of higher level abstractions / APIs.
So here I think in the comments we should not call it "Image hash" but rather refer to the spec for its full definition. We can of course call out the fact that it should have the "sha512-" perfix.
What do you think?
- Jiang Yan Xu
On July 11, 2015, 9:47 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> -----------------------------------------------------------
>
> (Updated July 11, 2015, 9:47 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ContainerImage protobuf.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6
>
> Diff: https://reviews.apache.org/r/34136/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Ian Downes <ia...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/
-----------------------------------------------------------
(Updated July 11, 2015, 9:47 p.m.)
Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
Changes
-------
Addressed comments.
Repository: mesos
Description
-------
Add ContainerImage protobuf.
Diffs (updated)
-----
include/mesos/mesos.proto 1763129da535561503e89cbd8c4a371f8553d8d6
Diff: https://reviews.apache.org/r/34136/diff/
Testing
-------
Thanks,
Ian Downes
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review91002
-----------------------------------------------------------
Ship it!
LGTM.
include/mesos/mesos.proto (line 1220)
<https://reviews.apache.org/r/34136/#comment144200>
Add a comment that only one of these should be set.
- Vinod Kone
On June 22, 2015, 4:42 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 4:42 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ContainerImage protobuf.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
>
> Diff: https://reviews.apache.org/r/34136/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Ian Downes <ia...@gmail.com>.
> On July 1, 2015, 4:54 p.m., Lily Chen wrote:
> > include/mesos/mesos.proto, line 1209
> > <https://reviews.apache.org/r/34136/diff/2/?file=989752#file989752line1209>
> >
> > Same issue as when MesosInfo is not present. What happens when there is no Appc message present in Image?
This is how we represent a union: a required type and then 1 or more optional messages for those types. Code is responsible for verifiying that the optional message is present to match the type.
- Ian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review90166
-----------------------------------------------------------
On June 22, 2015, 9:42 a.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 9:42 a.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ContainerImage protobuf.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
>
> Diff: https://reviews.apache.org/r/34136/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Lily Chen <li...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review90166
-----------------------------------------------------------
include/mesos/mesos.proto (line 1209)
<https://reviews.apache.org/r/34136/#comment143158>
Same issue as when MesosInfo is not present. What happens when there is no Appc message present in Image?
- Lily Chen
On June 22, 2015, 4:42 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 4:42 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ContainerImage protobuf.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
>
> Diff: https://reviews.apache.org/r/34136/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>
Re: Review Request 34136: Add ContainerImage protobuf.
Posted by Timothy Chen <tn...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review89226
-----------------------------------------------------------
Ship it!
Ship It!
- Timothy Chen
On June 22, 2015, 4:42 p.m., Ian Downes wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> -----------------------------------------------------------
>
> (Updated June 22, 2015, 4:42 p.m.)
>
>
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Add ContainerImage protobuf.
>
>
> Diffs
> -----
>
> include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70
>
> Diff: https://reviews.apache.org/r/34136/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Ian Downes
>
>