Re: Review Request 47198: Added labels to docker v1 spec config.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On May 24, 2016, 5:18 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47198/
> ---
> 
> (Updated May 24, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5272
> https://issues.apache.org/jira/browse/MESOS-5272
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added labels to docker v1 spec config.
> 
> 
> Diffs
> -
> 
>   include/mesos/docker/v1.proto ff18f8cadfce3315467f194d50d3469fa839f686 
> 
> Diff: https://reviews.apache.org/r/47198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 47198: Added labels to docker v1 spec config.

[Gilbert Song]

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

(Updated May 24, 2016, 10:18 a.m.)


Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, and Kevin 
Klues.


Bugs: MESOS-5272
https://issues.apache.org/jira/browse/MESOS-5272


Repository: mesos


Description
---

Added labels to docker v1 spec config.


Diffs (updated)
-

  include/mesos/docker/v1.proto ff18f8cadfce3315467f194d50d3469fa839f686 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 47198: Added labels to docker v1 spec config.

[Jie Yu]

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




include/mesos/docker/v1.proto (lines 53 - 56)


Mind pulling this to the top level?
```
spec::v1::Label
```



include/mesos/docker/v1.proto (lines 58 - 64)


I would mention that we cannot use `Labels` here because otherwise, 
json->protobuf parsing will fail. `labels` is manually set during parsing.


- Jie Yu


On May 13, 2016, 8:48 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47198/
> ---
> 
> (Updated May 13, 2016, 8:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5272
> https://issues.apache.org/jira/browse/MESOS-5272
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added labels to docker v1 spec config.
> 
> 
> Diffs
> -
> 
>   include/mesos/docker/v1.proto ff18f8cadfce3315467f194d50d3469fa839f686 
> 
> Diff: https://reviews.apache.org/r/47198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 47198: Added labels to docker v1 spec config.

[Guangya Liu]

> On 五月 12, 2016, 12:25 p.m., Guangya Liu wrote:
> > include/mesos/docker/v1.proto, lines 56-57
> > 
> >
> > Why both `optional`? I found that other `Label` definition are defining 
> > `key` as required but `value` as `optional` or `required`. I think that for 
> > your case, both `key` and `value` should be required?
> 
> Gilbert Song wrote:
> The docker label should be independent from the label in our first class 
> proto. 
> 
> Even though from docker documentation `Keys should start and end with an 
> alpha numeric character`. But I can still generate the followings by some 
> experiments using docker build:
> ```
> "Labels": {
> "": "something",
> "com.example.release-date": "2015-02-12",
> "com.example.version": "0.0.1-beta",
> "com.example.version.is-beta": "",
> "com.example.version.is-production": "",
> "vendor": "ACME Incorporated"
> }```
> 
> So that is the reason we want both optional. Actually `optional` should 
> be more safe even if you dont have the issue above.

I think that at least we need to have the `key` as required, otherwise, the 
label as `"": "something",` may have no meaning, how does a label without `key` 
works?


- Guangya


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


On 五月 13, 2016, 8:48 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47198/
> ---
> 
> (Updated 五月 13, 2016, 8:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5272
> https://issues.apache.org/jira/browse/MESOS-5272
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added labels to docker v1 spec config.
> 
> 
> Diffs
> -
> 
>   include/mesos/docker/v1.proto ff18f8cadfce3315467f194d50d3469fa839f686 
> 
> Diff: https://reviews.apache.org/r/47198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 47198: Added labels to docker v1 spec config.

[Gilbert Song]

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

(Updated May 13, 2016, 1:48 p.m.)


Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, and Kevin 
Klues.


Bugs: MESOS-5272
https://issues.apache.org/jira/browse/MESOS-5272


Repository: mesos


Description
---

Added labels to docker v1 spec config.


Diffs (updated)
-

  include/mesos/docker/v1.proto ff18f8cadfce3315467f194d50d3469fa839f686 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 47198: Added labels to docker v1 spec config.

[Gilbert Song]

> On May 12, 2016, 5:25 a.m., Guangya Liu wrote:
> > include/mesos/docker/v1.proto, lines 56-57
> > 
> >
> > Why both `optional`? I found that other `Label` definition are defining 
> > `key` as required but `value` as `optional` or `required`. I think that for 
> > your case, both `key` and `value` should be required?

The docker label should be independent from the label in our first class proto. 

Even though from docker documentation `Keys should start and end with an alpha 
numeric character`. But I can still generate the followings by some experiments 
using docker build:
```
"Labels": {
"": "something",
"com.example.release-date": "2015-02-12",
"com.example.version": "0.0.1-beta",
"com.example.version.is-beta": "",
"com.example.version.is-production": "",
"vendor": "ACME Incorporated"
}```

So that is the reason we want both optional. Actually `optional` should be more 
safe even if you dont have the issue above.


- Gilbert


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


On May 11, 2016, 8:56 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47198/
> ---
> 
> (Updated May 11, 2016, 8:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5272
> https://issues.apache.org/jira/browse/MESOS-5272
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added labels to docker v1 spec config.
> 
> 
> Diffs
> -
> 
>   include/mesos/docker/v1.proto ff18f8cadfce3315467f194d50d3469fa839f686 
> 
> Diff: https://reviews.apache.org/r/47198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 47198: Added labels to docker v1 spec config.

[Guangya Liu]

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




include/mesos/docker/v1.proto (lines 56 - 57)


Why both `optional`? I found that other `Label` definition are defining 
`key` as required but `value` as `optional` or `required`. I think that for 
your case, both `key` and `value` should be required?


- Guangya Liu


On 五月 12, 2016, 3:56 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47198/
> ---
> 
> (Updated 五月 12, 2016, 3:56 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, and Kevin 
> Klues.
> 
> 
> Bugs: MESOS-5272
> https://issues.apache.org/jira/browse/MESOS-5272
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added labels to docker v1 spec config.
> 
> 
> Diffs
> -
> 
>   include/mesos/docker/v1.proto ff18f8cadfce3315467f194d50d3469fa839f686 
> 
> Diff: https://reviews.apache.org/r/47198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 47198: Added labels to docker v1 spec config.

[Kevin Klues]

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




include/mesos/docker/v1.proto (lines 53 - 54)


I don't think you need this comment. To me `mesos` labels are completely 
independent from `docker` labels (even though they happen to have similar 
signatures).



include/mesos/docker/v1.proto (lines 60 - 63)


I still don't understand this comment.

Are you saying that all of the other fields in this protobuf can be parsed 
automatically, but this field is special and cannot?

If so, what makes it special, and why can't it be parsed automatically like 
the others?  That's what would be more interesting to me in the comment.


- Kevin Klues


On May 12, 2016, 3:56 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47198/
> ---
> 
> (Updated May 12, 2016, 3:56 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, and Kevin 
> Klues.
> 
> 
> Bugs: MESOS-5272
> https://issues.apache.org/jira/browse/MESOS-5272
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added labels to docker v1 spec config.
> 
> 
> Diffs
> -
> 
>   include/mesos/docker/v1.proto ff18f8cadfce3315467f194d50d3469fa839f686 
> 
> Diff: https://reviews.apache.org/r/47198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 47198: Added labels to docker v1 spec config.

[Gilbert Song]

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

(Updated May 11, 2016, 8:56 p.m.)


Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, and Kevin 
Klues.


Bugs: MESOS-5272
https://issues.apache.org/jira/browse/MESOS-5272


Repository: mesos


Description
---

Added labels to docker v1 spec config.


Diffs (updated)
-

  include/mesos/docker/v1.proto ff18f8cadfce3315467f194d50d3469fa839f686 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 47198: Added labels to docker v1 spec config.

[Kevin Klues]

> On May 11, 2016, 12:41 a.m., Kevin Klues wrote:
> > include/mesos/docker/v1.proto, lines 55-60
> > 
> >
> > I don't quite understand this comment.  WHy isn't this just a repeated 
> > optional string?
> 
> Gilbert Song wrote:
> WHy isn't this just a repeated optional string?
> I thought about it and did plan to use repeated `Label`, but in our first 
> class mesos.proto, all labels are used as an optional Labels since a repeated 
> Label is included. And the reason it is not a string is that any metadata 
> label in docker image should be a key-value pair (that is identical to how we 
> defind Label in mesos.proto.
> 
> About the commnet:
> Since we use protobuf::parse() to automatically parse all JSON to 
> protobuf message, the parse function relies on the field name and they are 
> case sensitive. It means we cannot use `Labels` (which is identical to docker 
> manifest), otherwise protobuf::parse would return an error. You can see all 
> other field names are exactly identical to those names in docker image 
> manifest execpt this one (`labels`).

It just feels weird to me to have this file depend on mesos.proto. If we don't 
want pure strings, I would expect to duplicate something similar to the 
`Labels` message in mesos.proto, but not pull it in directly. Curious what 
others think.


- Kevin


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


On May 10, 2016, 11:21 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47198/
> ---
> 
> (Updated May 10, 2016, 11:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, and Kevin 
> Klues.
> 
> 
> Bugs: MESOS-5272
> https://issues.apache.org/jira/browse/MESOS-5272
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added labels to docker v1 spec config.
> 
> 
> Diffs
> -
> 
>   include/mesos/docker/v1.proto ff18f8cadfce3315467f194d50d3469fa839f686 
> 
> Diff: https://reviews.apache.org/r/47198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 47198: Added labels to docker v1 spec config.

[Gilbert Song]

> On May 10, 2016, 5:41 p.m., Kevin Klues wrote:
> > include/mesos/docker/v1.proto, lines 55-60
> > 
> >
> > I don't quite understand this comment.  WHy isn't this just a repeated 
> > optional string?

WHy isn't this just a repeated optional string?
I thought about it and did plan to use repeated `Label`, but in our first class 
mesos.proto, all labels are used as an optional Labels since a repeated Label 
is included. And the reason it is not a string is that any metadata label in 
docker image should be a key-value pair (that is identical to how we defind 
Label in mesos.proto.

About the commnet:
Since we use protobuf::parse() to automatically parse all JSON to protobuf 
message, the parse function relies on the field name and they are case 
sensitive. It means we cannot use `Labels` (which is identical to docker 
manifest), otherwise protobuf::parse would return an error. You can see all 
other field names are exactly identical to those names in docker image manifest 
execpt this one (`labels`).


- Gilbert


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


On May 10, 2016, 4:21 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47198/
> ---
> 
> (Updated May 10, 2016, 4:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, and Kevin 
> Klues.
> 
> 
> Bugs: MESOS-5272
> https://issues.apache.org/jira/browse/MESOS-5272
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added labels to docker v1 spec config.
> 
> 
> Diffs
> -
> 
>   include/mesos/docker/v1.proto ff18f8cadfce3315467f194d50d3469fa839f686 
> 
> Diff: https://reviews.apache.org/r/47198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 47198: Added labels to docker v1 spec config.

[Gilbert Song]

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

Review request for mesos, Ben Mahler, Artem Harutyunyan, Jie Yu, and Kevin 
Klues.


Bugs: MESOS-5272
https://issues.apache.org/jira/browse/MESOS-5272


Repository: mesos


Description
---

Added labels to docker v1 spec config.


Diffs
-

  include/mesos/docker/v1.proto ff18f8cadfce3315467f194d50d3469fa839f686 

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


Testing
---

make check


Thanks,

Gilbert Song