Re: Review Request 55139: Implemented parse methods for OCI image spec.

[Qian Zhang]

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

(Updated Feb. 7, 2017, 4:41 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description
---

Implemented parse methods for OCI image spec.


Diffs (updated)
-

  include/mesos/oci/spec.hpp PRE-CREATION 
  src/CMakeLists.txt c09bcde3c14a73ab0b296c400f79f6a775da2470 
  src/Makefile.am 78d55d86b1e1238beda3163d3240c85609ea982d 
  src/oci/spec.cpp PRE-CREATION 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 55139: Implemented parse methods for OCI image spec.

[Qian Zhang]

> On Feb. 5, 2017, 12:02 p.m., Jie Yu wrote:
> > src/oci/spec.cpp, lines 163-166
> > 
> >
> > what if `digest.isNone()`?
> 
> Qian Zhang wrote:
> `digest` is a required field, so it can not be `None()` here.
> 
> Jie Yu wrote:
> I'd prefer to be a bit more defensive here, and self contained. Either 
> add a CHECK here, or doing explicit check of `isNone()`. I prefer the latter.

OK, let me do an explicit check of `isNone()`.


- Qian


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


On Feb. 6, 2017, 10:43 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55139/
> ---
> 
> (Updated Feb. 6, 2017, 10:43 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented parse methods for OCI image spec.
> 
> 
> Diffs
> -
> 
>   include/mesos/oci/spec.hpp PRE-CREATION 
>   src/CMakeLists.txt c09bcde3c14a73ab0b296c400f79f6a775da2470 
>   src/Makefile.am 78d55d86b1e1238beda3163d3240c85609ea982d 
>   src/oci/spec.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55139/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 55139: Implemented parse methods for OCI image spec.

[Jie Yu]

> On Feb. 5, 2017, 4:02 a.m., Jie Yu wrote:
> > src/oci/spec.cpp, lines 163-166
> > 
> >
> > what if `digest.isNone()`?
> 
> Qian Zhang wrote:
> `digest` is a required field, so it can not be `None()` here.

I'd prefer to be a bit more defensive here, and self contained. Either add a 
CHECK here, or doing explicit check of `isNone()`. I prefer the latter.


- Jie


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


On Feb. 6, 2017, 2:43 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55139/
> ---
> 
> (Updated Feb. 6, 2017, 2:43 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented parse methods for OCI image spec.
> 
> 
> Diffs
> -
> 
>   include/mesos/oci/spec.hpp PRE-CREATION 
>   src/CMakeLists.txt c09bcde3c14a73ab0b296c400f79f6a775da2470 
>   src/Makefile.am 78d55d86b1e1238beda3163d3240c85609ea982d 
>   src/oci/spec.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55139/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 55139: Implemented parse methods for OCI image spec.

[Qian Zhang]

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

(Updated Feb. 6, 2017, 10:43 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description
---

Implemented parse methods for OCI image spec.


Diffs (updated)
-

  include/mesos/oci/spec.hpp PRE-CREATION 
  src/CMakeLists.txt c09bcde3c14a73ab0b296c400f79f6a775da2470 
  src/Makefile.am 78d55d86b1e1238beda3163d3240c85609ea982d 
  src/oci/spec.cpp PRE-CREATION 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 55139: Implemented parse methods for OCI image spec.

[Qian Zhang]

> On Feb. 5, 2017, 12:02 p.m., Jie Yu wrote:
> > src/oci/spec.cpp, lines 163-166
> > 
> >
> > what if `digest.isNone()`?

`digest` is a required field, so it can not be `None()` here.


> On Feb. 5, 2017, 12:02 p.m., Jie Yu wrote:
> > src/oci/spec.cpp, lines 182-185
> > 
> >
> > What if `platform.isNone()`?

Ditto.


- Qian


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


On Feb. 1, 2017, 9:13 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55139/
> ---
> 
> (Updated Feb. 1, 2017, 9:13 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented parse methods for OCI image spec.
> 
> 
> Diffs
> -
> 
>   include/mesos/oci/spec.hpp PRE-CREATION 
>   src/CMakeLists.txt 09ef1aee680c6b91476092dbf61d4ca3a8d256bb 
>   src/Makefile.am 6c9be54fe0ce3faaa26e2f090773d1b77bb6e430 
>   src/oci/spec.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55139/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 55139: Implemented parse methods for OCI image spec.

[Jie Yu]

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


Fix it, then Ship it!





src/oci/spec.cpp (line 33)


`const string&`



src/oci/spec.cpp (line 76)


insert extra blank line above.



src/oci/spec.cpp (line 156)


Any reason you need this temp var?



src/oci/spec.cpp (lines 163 - 166)


what if `digest.isNone()`?



src/oci/spec.cpp (lines 182 - 185)


What if `platform.isNone()`?



src/oci/spec.cpp (line 226)


replace .get() with `->`



src/oci/spec.cpp (line 271)


You can use `!annotations->is()`



src/oci/spec.cpp (line 319)


kill this line



src/oci/spec.cpp (line 325)


no need for this temp var?



src/oci/spec.cpp (line 338)


Ditto. No need for this temp var.



src/oci/spec.cpp (line 351)


No need for this temp var?


- Jie Yu


On Feb. 1, 2017, 1:13 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55139/
> ---
> 
> (Updated Feb. 1, 2017, 1:13 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented parse methods for OCI image spec.
> 
> 
> Diffs
> -
> 
>   include/mesos/oci/spec.hpp PRE-CREATION 
>   src/CMakeLists.txt 09ef1aee680c6b91476092dbf61d4ca3a8d256bb 
>   src/Makefile.am 6c9be54fe0ce3faaa26e2f090773d1b77bb6e430 
>   src/oci/spec.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55139/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 55139: Implemented parse methods for OCI image spec.

[Qian Zhang]

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

(Updated Feb. 1, 2017, 9:13 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Added internal validate method.


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


Repository: mesos


Description
---

Implemented parse methods for OCI image spec.


Diffs (updated)
-

  include/mesos/oci/spec.hpp PRE-CREATION 
  src/CMakeLists.txt 09ef1aee680c6b91476092dbf61d4ca3a8d256bb 
  src/Makefile.am 6c9be54fe0ce3faaa26e2f090773d1b77bb6e430 
  src/oci/spec.cpp PRE-CREATION 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 55139: Implemented parse methods for OCI image spec.

[Qian Zhang]

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

(Updated Jan. 9, 2017, 3:25 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description
---

Implemented parse methods for OCI image spec.


Diffs (updated)
-

  include/mesos/oci/spec.hpp PRE-CREATION 
  src/CMakeLists.txt c8d4260c03d8cdee1951a50d293e9fdabcd2cf84 
  src/Makefile.am 6d0f77be37af9bc4e22199418796d6d0c5b6c462 
  src/oci/spec.cpp PRE-CREATION 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 55139: Implemented parse methods for OCI image spec.

[Jie Yu]

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




include/mesos/oci/spec.hpp (lines 27 - 48)


I would use template and template specification here:
```
template 
Try parse(const string& s);

template <>
Try parse(const string& s);

...
```

At callsite, you can do:
```
Try descriptor = parse(s);
```


- Jie Yu


On Jan. 3, 2017, 2:11 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55139/
> ---
> 
> (Updated Jan. 3, 2017, 2:11 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6681
> https://issues.apache.org/jira/browse/MESOS-6681
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented parse methods for OCI image spec.
> 
> 
> Diffs
> -
> 
>   include/mesos/oci/spec.hpp PRE-CREATION 
>   src/CMakeLists.txt c8d4260c03d8cdee1951a50d293e9fdabcd2cf84 
>   src/Makefile.am 6d0f77be37af9bc4e22199418796d6d0c5b6c462 
>   src/oci/spec.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/55139/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 55139: Implemented parse methods for OCI image spec.

[Qian Zhang]

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

Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Implemented parse methods for OCI image spec.


Diffs
-

  include/mesos/oci/spec.hpp PRE-CREATION 
  src/CMakeLists.txt c8d4260c03d8cdee1951a50d293e9fdabcd2cf84 
  src/Makefile.am 6d0f77be37af9bc4e22199418796d6d0c5b6c462 
  src/oci/spec.cpp PRE-CREATION 

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


Testing
---


Thanks,

Qian Zhang