Re: Review Request 42773: Added utility function to compute SHA512 digest.

2016-01-28 Thread Jojy Varghese 

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

(Updated Jan. 29, 2016, 2:03 a.m.)


Review request for mesos.


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


Repository: mesos


Description
---

Added utility function to compute SHA512 digest.


Diffs
-

  src/common/command_utils.hpp bfab34571dd37980d9f99585e506eb015b499e28 
  src/common/command_utils.cpp e9d782127f948e165f55d03a2b1bf47946cc7f4e 
  src/tests/common/command_utils_tests.cpp 
938f3995aacf74bac28ae11040de292aa328f1e5 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 42773: Added utility function to compute SHA512 digest.

2016-01-28 Thread Jie Yu 

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


Fix it, then Ship it!




I'll fix the remaining issue for you.


src/common/command_utils.cpp (lines 178 - 179)


I would put these two in one line.



src/common/command_utils.cpp (lines 191 - 197)


I would remove this check for now (will keep the TODO here).


- Jie Yu


On Jan. 29, 2016, 12:54 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42773/
> ---
> 
> (Updated Jan. 29, 2016, 12:54 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added utility function to compute SHA512 digest.
> 
> 
> Diffs
> -
> 
>   src/common/command_utils.hpp bfab34571dd37980d9f99585e506eb015b499e28 
>   src/common/command_utils.cpp e9d782127f948e165f55d03a2b1bf47946cc7f4e 
>   src/tests/common/command_utils_tests.cpp 
> 938f3995aacf74bac28ae11040de292aa328f1e5 
> 
> Diff: https://reviews.apache.org/r/42773/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 42773: Added utility function to compute SHA512 digest.

2016-01-28 Thread Jojy Varghese 

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

(Updated Jan. 29, 2016, 12:54 a.m.)


Review request for mesos.


Changes
---

review addressed.


Repository: mesos


Description
---

Added utility function to compute SHA512 digest.


Diffs (updated)
-

  src/common/command_utils.hpp bfab34571dd37980d9f99585e506eb015b499e28 
  src/common/command_utils.cpp e9d782127f948e165f55d03a2b1bf47946cc7f4e 
  src/tests/common/command_utils_tests.cpp 
938f3995aacf74bac28ae11040de292aa328f1e5 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 42773: Added utility function to compute SHA512 digest.

2016-01-28 Thread Jie Yu 

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




src/common/command_utils.hpp (line 68)


s/alg/algorithm/

(e.g., 512)



src/common/command_utils.hpp (line 71)


s/alg/algorithm/



src/common/command_utils.cpp (line 169)


hum, can you move the body of this method to shasum and call shasum in this 
function?

```
Future sha512(const Path& input)
{
  return shasum("512", input);
}
```


- Jie Yu


On Jan. 28, 2016, 6:44 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42773/
> ---
> 
> (Updated Jan. 28, 2016, 6:44 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added utility function to compute SHA512 digest.
> 
> 
> Diffs
> -
> 
>   src/common/command_utils.hpp bfab34571dd37980d9f99585e506eb015b499e28 
>   src/common/command_utils.cpp e9d782127f948e165f55d03a2b1bf47946cc7f4e 
>   src/tests/common/command_utils_tests.cpp 
> 938f3995aacf74bac28ae11040de292aa328f1e5 
> 
> Diff: https://reviews.apache.org/r/42773/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 42773: Added utility function to compute SHA512 digest.

2016-01-28 Thread Jojy Varghese 

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

(Updated Jan. 28, 2016, 6:44 p.m.)


Review request for mesos.


Changes
---

addressed review comments


Repository: mesos


Description
---

Added utility function to compute SHA512 digest.


Diffs (updated)
-

  src/common/command_utils.hpp bfab34571dd37980d9f99585e506eb015b499e28 
  src/common/command_utils.cpp e9d782127f948e165f55d03a2b1bf47946cc7f4e 
  src/tests/common/command_utils_tests.cpp 
938f3995aacf74bac28ae11040de292aa328f1e5 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 42773: Added utility function to compute SHA512 digest.

2016-01-28 Thread Jie Yu 

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


Fix it, then Ship it!





src/common/command_utils.hpp (line 65)


can you s/sha512/shasum/ and add an algorithm parameter (type string).

we can probably create sha512 method still, but will invoke the underlying 
shasum method.



src/common/command_utils.hpp (lines 65 - 66)


s/filePath/file/

I mentioned this in a couple of reviews. Please don't forget next time:)



src/common/command_utils.cpp (line 20)


Why do you need this?



src/common/command_utils.cpp (lines 30 - 31)


Is it used?



src/common/command_utils.cpp (line 173)


please make sure the parameter name is consistent with that in the header!



src/common/command_utils.cpp (line 177)


First letter capitalized



src/common/command_utils.cpp (line 178)


First letter capitalized.



src/common/command_utils.cpp (line 187)


s/output//

from shasum command



src/common/command_utils.cpp (lines 193 - 194)


Ditto.



src/tests/common/command_utils_tests.cpp (line 226)


s/DigestTest/ShasumTest/



src/tests/common/command_utils_tests.cpp (line 231)


do you need this temp var?



src/tests/common/command_utils_tests.cpp (line 237)


s/sha512Future/sha512/



src/tests/common/command_utils_tests.cpp (lines 242 - 243)


Please don't split this string. You can use
```
// NOLINT(whitespace/line_length)
```

in the end to silence style checker.


- Jie Yu


On Jan. 27, 2016, 10:29 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42773/
> ---
> 
> (Updated Jan. 27, 2016, 10:29 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added utility function to compute SHA512 digest.
> 
> 
> Diffs
> -
> 
>   src/common/command_utils.hpp PRE-CREATION 
>   src/common/command_utils.cpp PRE-CREATION 
>   src/tests/common/command_utils_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/42773/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 42773: Added utility function to compute SHA512 digest.

2016-01-27 Thread Jojy Varghese 

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

(Updated Jan. 27, 2016, 10:29 p.m.)


Review request for mesos.


Changes
---

simplified the method.


Repository: mesos


Description
---

Added utility function to compute SHA512 digest.


Diffs (updated)
-

  src/common/command_utils.hpp PRE-CREATION 
  src/common/command_utils.cpp PRE-CREATION 
  src/tests/common/command_utils_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese