Re: Review Request 49208: Added tests to check if appc spec is properly parsed.

2016-07-13 Thread Jie Yu 

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


Ship it!




I committed this patch and helped address some style issues while committing. 
Please take a look at the final patch and see what needs to be taken care of 
next time. Thanks!

- Jie Yu


On July 12, 2016, 4:14 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49208/
> ---
> 
> (Updated July 12, 2016, 4:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4778
> https://issues.apache.org/jira/browse/MESOS-4778
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests to check if appc spec is properly parsed.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 599ebbef6d164fb2a530b55427ddabb5cd607634 
>   src/tests/containerizer/appc_spec_tests.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 061f80c62319817b22a5c1880a4858fdafbfb72a 
> 
> Diff: https://reviews.apache.org/r/49208/diff/
> 
> 
> Testing
> ---
> 
> Make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49208: Added tests to check if appc spec is properly parsed.

2016-07-13 Thread Gilbert Song 


> On July 13, 2016, 11 a.m., Gilbert Song wrote:
> > src/tests/containerizer/appc_spec_tests.cpp, lines 227-228
> > 
> >
> > ```
> >   EXPECT_EQ(
> >   imageManifest->annotations(0).value(),
> >   "acbuild set-name \"example.com/hello\"");```

I meant:

EXPECT_EQ(
imageManifest->annotations(0).value(),
"acbuild set-name \"example.com/hello\"");


- Gilbert


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


On July 12, 2016, 9:14 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49208/
> ---
> 
> (Updated July 12, 2016, 9:14 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4778
> https://issues.apache.org/jira/browse/MESOS-4778
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests to check if appc spec is properly parsed.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 599ebbef6d164fb2a530b55427ddabb5cd607634 
>   src/tests/containerizer/appc_spec_tests.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 061f80c62319817b22a5c1880a4858fdafbfb72a 
> 
> Diff: https://reviews.apache.org/r/49208/diff/
> 
> 
> Testing
> ---
> 
> Make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49208: Added tests to check if appc spec is properly parsed.

2016-07-13 Thread Gilbert Song 

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


Fix it, then Ship it!





src/tests/containerizer/appc_spec_tests.cpp (lines 38 - 75)


Diito.



src/tests/containerizer/appc_spec_tests.cpp (line 83)


ASSERT_TRUE



src/tests/containerizer/appc_spec_tests.cpp (line 84)


ASSERT_TRUE



src/tests/containerizer/appc_spec_tests.cpp (lines 101 - 105)


Indentation issue. Move 2 speces to the left.



src/tests/containerizer/appc_spec_tests.cpp (lines 118 - 123)


ditto.



src/tests/containerizer/appc_spec_tests.cpp (lines 145 - 211)


Ditto.



src/tests/containerizer/appc_spec_tests.cpp (line 219)


ASSERT_TRUE



src/tests/containerizer/appc_spec_tests.cpp (line 220)


EXPECT_FALSE



src/tests/containerizer/appc_spec_tests.cpp (lines 227 - 228)


```
  EXPECT_EQ(
  imageManifest->annotations(0).value(),
  "acbuild set-name \"example.com/hello\"");```



src/tests/containerizer/provisioner_appc_tests.cpp (line 76)


Kill this line


- Gilbert Song


On July 12, 2016, 9:14 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49208/
> ---
> 
> (Updated July 12, 2016, 9:14 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4778
> https://issues.apache.org/jira/browse/MESOS-4778
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests to check if appc spec is properly parsed.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 599ebbef6d164fb2a530b55427ddabb5cd607634 
>   src/tests/containerizer/appc_spec_tests.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 061f80c62319817b22a5c1880a4858fdafbfb72a 
> 
> Diff: https://reviews.apache.org/r/49208/diff/
> 
> 
> Testing
> ---
> 
> Make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49208: Added tests to check if appc spec is properly parsed.

2016-07-13 Thread Guangya Liu 

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




src/tests/containerizer/appc_spec_tests.cpp (lines 77 - 80)


Please update here as this:

```javascript
ASSERT_SOME(json);

Try imageManifest = spec::parse(stringify(json.get()));
ASSERT_SOME(imageManifest);
```



src/tests/containerizer/appc_spec_tests.cpp (lines 107 - 108)


new line here



src/tests/containerizer/appc_spec_tests.cpp (lines 125 - 129)


Please update as this:

```javascript
ASSERT_SOME(json);

Try manifest = spec::parse(stringify(json.get()));
ASSERT_SOME(manifest);

ASSERT_SOME(os::write(path::join(image, "manifest"), 
stringify(json.get(;
```



src/tests/containerizer/appc_spec_tests.cpp (lines 213 - 216)


Please update as following:

```javascript
ASSERT_SOME(json);

Try imageManifest = spec::parse(stringify(json.get()));
ASSERT_SOME(imageManifest);
```



src/tests/containerizer/appc_spec_tests.cpp (line 228)


keep align with `imageManifest->annotations(0).value(),`

```javascript
 EXPECT_EQ(imageManifest->annotations(0).value(),
   "acbuild set-name \"example.com/hello\"");
```



src/tests/containerizer/provisioner_appc_tests.cpp (line 76)


kill this line


- Guangya Liu


On 七月 12, 2016, 4:14 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49208/
> ---
> 
> (Updated 七月 12, 2016, 4:14 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4778
> https://issues.apache.org/jira/browse/MESOS-4778
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests to check if appc spec is properly parsed.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 599ebbef6d164fb2a530b55427ddabb5cd607634 
>   src/tests/containerizer/appc_spec_tests.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 061f80c62319817b22a5c1880a4858fdafbfb72a 
> 
> Diff: https://reviews.apache.org/r/49208/diff/
> 
> 
> Testing
> ---
> 
> Make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49208: Added tests to check if appc spec is properly parsed.

2016-07-12 Thread Srinivas Brahmaroutu 

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

(Updated July 12, 2016, 4:14 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added tests to check if appc spec is properly parsed.


Diffs (updated)
-

  src/Makefile.am 599ebbef6d164fb2a530b55427ddabb5cd607634 
  src/tests/containerizer/appc_spec_tests.cpp PRE-CREATION 
  src/tests/containerizer/provisioner_appc_tests.cpp 
061f80c62319817b22a5c1880a4858fdafbfb72a 

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


Testing
---

Make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49208: Added tests to check if appc spec is properly parsed.

2016-07-11 Thread Srinivas Brahmaroutu 

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

(Updated July 12, 2016, 4:38 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added tests to check if appc spec is properly parsed.


Diffs (updated)
-

  src/Makefile.am 599ebbef6d164fb2a530b55427ddabb5cd607634 
  src/tests/containerizer/appc_spec_tests.cpp PRE-CREATION 
  src/tests/containerizer/provisioner_appc_tests.cpp 
061f80c62319817b22a5c1880a4858fdafbfb72a 

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


Testing
---

Make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49208: Added tests to check if appc spec is properly parsed.

2016-07-06 Thread Guangya Liu 


> On July 5, 2016, 4:47 p.m., Gilbert Song wrote:
> > src/tests/containerizer/appc_spec_tests.cpp, line 80
> > 
> >
> > We should firstly check `has_workingDirectory()` right? Otherwise, it 
> > may segfault if not existed.

+1 to check `has_workingDirectory()` here but as we already defined the 
manifest above and it does include a `workingDirectory` there, so will not 
cause issue here.


- Guangya


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


On June 30, 2016, 6 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49208/
> ---
> 
> (Updated June 30, 2016, 6 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4778
> https://issues.apache.org/jira/browse/MESOS-4778
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests to check if appc spec is properly parsed.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 86c39fdf379ada470c9b1f86be263ef71dc47c41 
>   src/tests/containerizer/appc_spec_tests.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 061f80c62319817b22a5c1880a4858fdafbfb72a 
> 
> Diff: https://reviews.apache.org/r/49208/diff/
> 
> 
> Testing
> ---
> 
> Make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49208: Added tests to check if appc spec is properly parsed.

2016-07-05 Thread Gilbert Song 

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



Thanks, Srini! This patch looks good to me. Please address my comments. I will 
give a `ship it` once you update.


src/tests/containerizer/appc_spec_tests.cpp (line 32)


one more newline here.



src/tests/containerizer/appc_spec_tests.cpp (lines 36 - 73)


Not yours, it is a tech debt.

Could you please use `R"~(...)~"` express the json string? Please follow 
`DockerSpecTest::ParseV2ImageManifest` for example. Thanks.



src/tests/containerizer/appc_spec_tests.cpp (line 76)


We should use `ASSERT_SOME()` here.



src/tests/containerizer/appc_spec_tests.cpp (line 80)


We should firstly check `has_workingDirectory()` right? Otherwise, it may 
segfault if not existed.



src/tests/containerizer/appc_spec_tests.cpp (line 103)


Add one more newline.



src/tests/containerizer/provisioner_appc_tests.cpp (line 109)


Add one more newline.


- Gilbert Song


On June 29, 2016, 11 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49208/
> ---
> 
> (Updated June 29, 2016, 11 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4778
> https://issues.apache.org/jira/browse/MESOS-4778
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests to check if appc spec is properly parsed.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 86c39fdf379ada470c9b1f86be263ef71dc47c41 
>   src/tests/containerizer/appc_spec_tests.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 061f80c62319817b22a5c1880a4858fdafbfb72a 
> 
> Diff: https://reviews.apache.org/r/49208/diff/
> 
> 
> Testing
> ---
> 
> Make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49208: Added tests to check if appc spec is properly parsed.

2016-06-30 Thread Srinivas Brahmaroutu 

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

(Updated June 30, 2016, 6 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added tests to check if appc spec is properly parsed.


Diffs (updated)
-

  src/Makefile.am 86c39fdf379ada470c9b1f86be263ef71dc47c41 
  src/tests/containerizer/appc_spec_tests.cpp PRE-CREATION 
  src/tests/containerizer/provisioner_appc_tests.cpp 
061f80c62319817b22a5c1880a4858fdafbfb72a 

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


Testing
---

Make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49208: Added tests to check if appc spec is properly parsed.

2016-06-29 Thread Srinivas Brahmaroutu 

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

(Updated June 30, 2016, 5:38 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added tests to check if appc spec is properly parsed.


Diffs (updated)
-

  src/Makefile.am 86c39fdf379ada470c9b1f86be263ef71dc47c41 
  src/tests/containerizer/appc_spec_tests.cpp PRE-CREATION 
  src/tests/containerizer/provisioner_appc_tests.cpp 
061f80c62319817b22a5c1880a4858fdafbfb72a 

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


Testing
---

Make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49208: Added tests to check if appc spec is properly parsed.

2016-06-28 Thread Srinivas Brahmaroutu 

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

(Updated June 28, 2016, 10:13 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added tests to check if appc spec is properly parsed.


Diffs (updated)
-

  src/Makefile.am 86c39fdf379ada470c9b1f86be263ef71dc47c41 
  src/tests/containerizer/appc_spec_tests.cpp PRE-CREATION 
  src/tests/containerizer/provisioner_appc_tests.cpp 
061f80c62319817b22a5c1880a4858fdafbfb72a 

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


Testing
---

Make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 49208: Added tests to check if appc spec is properly parsed.

2016-06-28 Thread Guangya Liu 

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




src/Makefile.am (lines 2097 - 2098)


adjust the order here



src/tests/containerizer/appc_spec_tests.cpp (line 32)


It's already in its own test file ;-)



src/tests/containerizer/appc_spec_tests.cpp (line 81)


period to the end



src/tests/containerizer/appc_spec_tests.cpp (lines 84 - 85)


Also validate the `size` as `1`?



src/tests/containerizer/appc_spec_tests.cpp (line 90)


period to the end


- Guangya Liu


On 六月 28, 2016, 5:05 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49208/
> ---
> 
> (Updated 六月 28, 2016, 5:05 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4778
> https://issues.apache.org/jira/browse/MESOS-4778
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests to check if appc spec is properly parsed.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 23cac95a805e46d216e9479fea09f2c1619c45a5 
>   src/tests/containerizer/appc_spec_tests.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 061f80c62319817b22a5c1880a4858fdafbfb72a 
> 
> Diff: https://reviews.apache.org/r/49208/diff/
> 
> 
> Testing
> ---
> 
> Make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49208: Added tests to check if appc spec is properly parsed.

2016-06-27 Thread Srinivas Brahmaroutu 

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

(Updated June 28, 2016, 5:05 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Summary (updated)
-

Added tests to check if appc spec is properly parsed.


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


Repository: mesos


Description (updated)
---

Added tests to check if appc spec is properly parsed.


Diffs (updated)
-

  src/Makefile.am 23cac95a805e46d216e9479fea09f2c1619c45a5 
  src/tests/containerizer/appc_spec_tests.cpp PRE-CREATION 
  src/tests/containerizer/provisioner_appc_tests.cpp 
061f80c62319817b22a5c1880a4858fdafbfb72a 

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


Testing
---

Make check


Thanks,

Srinivas Brahmaroutu