Re: Review Request 43198: Added common appc spec utilities.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Feb. 11, 2016, 9:16 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43198/
> ---
> 
> (Updated Feb. 11, 2016, 9:16 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4596
> https://issues.apache.org/jira/browse/MESOS-4596
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds common utility functions such as :
>   - validating image information against actual data in the image directory.
>   - getting list of dependencies at depth 1 for an image.
>   - getting image path simple image discovery.
> 
> 
> Diffs
> -
> 
>   include/mesos/appc/spec.hpp 462159daeb5c5a75dd5c2c340d797d3cfd4d7e11 
>   src/appc/spec.cpp fc36a0b5bc36c236cc4f49210d0058b34d0ffc40 
> 
> Diff: https://reviews.apache.org/r/43198/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 43198: Added common appc spec utilities.

[Jojy Varghese]

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

(Updated Feb. 11, 2016, 9:16 p.m.)


Review request for mesos and Jie Yu.


Changes
---

fixed blank line issue.


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


Repository: mesos


Description
---

This change adds common utility functions such as :
  - validating image information against actual data in the image directory.
  - getting list of dependencies at depth 1 for an image.
  - getting image path simple image discovery.


Diffs (updated)
-

  include/mesos/appc/spec.hpp 462159daeb5c5a75dd5c2c340d797d3cfd4d7e11 
  src/appc/spec.cpp fc36a0b5bc36c236cc4f49210d0058b34d0ffc40 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 43198: Added common appc spec utilities.

[Jojy Varghese]

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

(Updated Feb. 11, 2016, 1:16 a.m.)


Review request for mesos and Jie Yu.


Changes
---

review addressed.


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


Repository: mesos


Description
---

This change adds common utility functions such as :
  - validating image information against actual data in the image directory.
  - getting list of dependencies at depth 1 for an image.
  - getting image path simple image discovery.


Diffs (updated)
-

  include/mesos/appc/spec.hpp 462159daeb5c5a75dd5c2c340d797d3cfd4d7e11 
  src/appc/spec.cpp fc36a0b5bc36c236cc4f49210d0058b34d0ffc40 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 43198: Added common appc spec utilities.

[Jie Yu]

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


Fix it, then Ship it!





include/mesos/appc/spec.hpp (line 60)


s/imageDir/imagePath/

to be consistent.



src/appc/spec.cpp (line 87)


can you reorder the functions in cpp file to match the order in the hpp 
file?



src/appc/spec.cpp (line 109)


s/imageDir/imagePath/



src/appc/spec.cpp (line 134)


Please validate manifest as well.



src/appc/spec.cpp (line 141)


Hum, you want to do:
```
validate = validateImageID(...);
if (imageId.isSome()) {
  return Error("Invalid image ID: " + validate->message);
}

return None();
```


- Jie Yu


On Feb. 10, 2016, 3:11 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43198/
> ---
> 
> (Updated Feb. 10, 2016, 3:11 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4596
> https://issues.apache.org/jira/browse/MESOS-4596
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds common utility functions such as :
>   - validating image information against actual data in the image directory.
>   - getting list of dependencies at depth 1 for an image.
>   - getting image path simple image discovery.
> 
> 
> Diffs
> -
> 
>   include/mesos/appc/spec.hpp 462159daeb5c5a75dd5c2c340d797d3cfd4d7e11 
>   src/appc/spec.cpp fc36a0b5bc36c236cc4f49210d0058b34d0ffc40 
> 
> Diff: https://reviews.apache.org/r/43198/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 43198: Added common appc spec utilities.

[Jojy Varghese]

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

(Updated Feb. 10, 2016, 3:11 p.m.)


Review request for mesos and Jie Yu.


Changes
---

addressed review.


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


Repository: mesos


Description
---

This change adds common utility functions such as :
  - validating image information against actual data in the image directory.
  - getting list of dependencies at depth 1 for an image.
  - getting image path simple image discovery.


Diffs (updated)
-

  include/mesos/appc/spec.hpp 462159daeb5c5a75dd5c2c340d797d3cfd4d7e11 
  src/appc/spec.cpp fc36a0b5bc36c236cc4f49210d0058b34d0ffc40 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 43198: Added common appc spec utilities.

[Jie Yu]

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




include/mesos/appc/spec.hpp (line 27)


add a blank line below



include/mesos/appc/spec.hpp (line 46)


Why Future here. I think Try should be sufficient. Let's use os::read 
instead of io::read.

Also, the parameter is a 'Path' while the function right above this 
function is using 'string' as the image path. I think we should be consistent 
here, either using string or Path for all of them.



include/mesos/appc/spec.hpp (lines 69 - 71)


Hum, the semantics of this validation function is complicated. Can we 
instead of a simpler validation function just for imagePath?
```
Option validate(const std::string& imagePath)
{
  // validate layout
  // validate manifest
  // validate image id
}
```

We can pull the name/label matching to the top level (does not belong to 
validate, maybe introduce a 'match' function in appc::Store).



src/appc/spec.cpp (line 128)


Please use os::read instead.



src/appc/spec.cpp (lines 160 - 185)


This label comparision is nasty. I think one of the reason is because we're 
using mesos::Labels in Image::Appc while appc::spec::ImageManifest is using its 
own Label.

Can you first change Image::Appc:
```
message Appc {
  required string name = 1;
  optional string id = 2;
  repeated appc::spec::Label labels = 3;
}
```
(You need to pull Label in ImageManifest to the top level).

Then, add a C++ appc::spec::Labels class to wrap the repeated protobuf 
field (like we did for Resource) in spec.hpp|cpp

Then, you can introduce a == operator for Labels in spec.hpp|cpp.



src/appc/spec.cpp (line 192)


Let's move this function to appc::Store. Simple discovery is already 
removed from the spec, we're just add this as one of our extention to the spec.



src/appc/spec.cpp (line 214)


I got confused, we do we need to call os::uname for this? The target image 
should have nothing to do with the os/arch of the host, right (even default 
value does not make sense to me).


- Jie Yu


On Feb. 9, 2016, 7:40 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43198/
> ---
> 
> (Updated Feb. 9, 2016, 7:40 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4596
> https://issues.apache.org/jira/browse/MESOS-4596
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change adds common utility functions such as :
>   - validating image information against actual data in the image directory.
>   - getting list of dependencies at depth 1 for an image.
>   - getting image path simple image discovery.
> 
> 
> Diffs
> -
> 
>   include/mesos/appc/spec.hpp 462159daeb5c5a75dd5c2c340d797d3cfd4d7e11 
>   src/appc/spec.cpp fc36a0b5bc36c236cc4f49210d0058b34d0ffc40 
> 
> Diff: https://reviews.apache.org/r/43198/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 43198: Added common appc spec utilities.

[Jojy Varghese]

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

(Updated Feb. 9, 2016, 7:40 p.m.)


Review request for mesos and Jie Yu.


Changes
---

removed getDependencies.


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


Repository: mesos


Description
---

This change adds common utility functions such as :
  - validating image information against actual data in the image directory.
  - getting list of dependencies at depth 1 for an image.
  - getting image path simple image discovery.


Diffs (updated)
-

  include/mesos/appc/spec.hpp 462159daeb5c5a75dd5c2c340d797d3cfd4d7e11 
  src/appc/spec.cpp fc36a0b5bc36c236cc4f49210d0058b34d0ffc40 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 43198: Added common appc spec utilities.

[Jojy Varghese]

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

(Updated Feb. 8, 2016, 8:27 p.m.)


Review request for mesos and Jie Yu.


Changes
---

changed getDependencies API.


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


Repository: mesos


Description
---

This change adds common utility functions such as :
  - validating image information against actual data in the image directory.
  - getting list of dependencies at depth 1 for an image.
  - getting image path simple image discovery.


Diffs (updated)
-

  include/mesos/appc/spec.hpp 462159daeb5c5a75dd5c2c340d797d3cfd4d7e11 
  src/appc/spec.cpp fc36a0b5bc36c236cc4f49210d0058b34d0ffc40 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Review Request 43198: Added common appc spec utilities.

[Jojy Varghese]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

This change adds common utility functions such as :
  - validating image information against actual data in the image directory.
  - getting list of dependencies at depth 1 for an image.
  - getting image path simple image discovery.


Diffs
-

  include/mesos/appc/spec.hpp d4fd8cbfd5668da9ce4ac86baaadf42945c5abbd 
  src/appc/spec.cpp 324cdfec3766da4a8e324378a6e413477fa2b5d9 

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


Testing
---

make check.


Thanks,

Jojy Varghese