Re: Review Request 34136: Add ContainerImage protobuf.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review93481
---



include/mesos/mesos.proto (line 1213)


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.

[Timothy Chen]

> On July 14, 2015, 9:03 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1211-1213
> > 
> >
> > 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.

[Jiang Yan Xu]

> On July 14, 2015, 2:03 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1211-1213
> > 
> >
> > 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.

[Timothy Chen]

> On July 14, 2015, 9:03 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1211-1213
> > 
> >
> > 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.

[Timothy Chen]

> On July 14, 2015, 9:03 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1211-1213
> > 
> >
> > 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.

[Jiang Yan Xu]

> On June 26, 2015, 2:57 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1212-1214
> > 
> >
> > 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.

[Jiang Yan Xu]

---
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)


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.

[Ian Downes]

---
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.

[Ian Downes]

> On June 26, 2015, 2:57 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1212-1214
> > 
> >
> > 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.

[Ian Downes]

> On June 25, 2015, 3:32 p.m., Paul Brett wrote:
> > include/mesos/mesos.proto, line 1221
> > 
> >
> > 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
> > 
> >
> > 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.

[Ian Downes]

> On July 1, 2015, 4:54 p.m., Lily Chen wrote:
> > include/mesos/mesos.proto, line 1209
> > 
> >
> > 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.

[Timothy Chen]

> On June 25, 2015, 10:32 p.m., Paul Brett wrote:
> > include/mesos/mesos.proto, line 1221
> > 
> >
> > 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.

[Vinod Kone]

> On June 25, 2015, 10:32 p.m., Paul Brett wrote:
> > include/mesos/mesos.proto, line 1221
> > 
> >
> > 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
> > 
> >
> > 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.

[Vinod Kone]

---
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)


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.

[Vinod Kone]

> On June 26, 2015, 9:57 p.m., Jiang Yan Xu wrote:
> > include/mesos/mesos.proto, lines 1212-1214
> > 
> >
> > 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.

[Lily Chen]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review90166
---



include/mesos/mesos.proto (line 1209)


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.

[Jiang Yan Xu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review89577
---



include/mesos/mesos.proto (lines 1212 - 1214)


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.

[Paul Brett]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review89438
---



include/mesos/mesos.proto (line 1196)


I assume that much of this is prescribed by the AppC specification - can we 
reference it?



include/mesos/mesos.proto (line 1202)


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)


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.

[Timothy Chen]

---
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
> 
>

Re: Review Request 34136: Add ContainerImage protobuf.

[Ian Downes]

---
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.

[Timothy Chen]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/#review83823
---



include/mesos/mesos.proto


BenH left a similar comment earlier last time when we looked at the new 
changes, and basically I think instead of calling it just ContainerImage we 
should make sure it's Appc specific, in case there are other formats we want to 
support in the future and now we have to deprecate and reintroduce new ones 
like we did the in past.


- Timothy Chen


On May 13, 2015, 12:47 a.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34136/
> ---
> 
> (Updated May 13, 2015, 12: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 db4fc8c001dd68bc3b9ca83650170c4f26db18c7 
> 
> Diff: https://reviews.apache.org/r/34136/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Review Request 34136: Add ContainerImage protobuf.

[Ian Downes]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/34136/
---

Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.


Repository: mesos


Description
---

Add ContainerImage protobuf.


Diffs
-

  include/mesos/mesos.proto db4fc8c001dd68bc3b9ca83650170c4f26db18c7 

Diff: https://reviews.apache.org/r/34136/diff/


Testing
---


Thanks,

Ian Downes