Re: Review Request 26363: Make the large-update check in the client update command consider instance parameters.

2014-10-20 Thread David McLaughlin

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

Ship it!


Ship It!

- David McLaughlin


On Oct. 8, 2014, 11:56 p.m., Mark Chu-Carroll wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/26363/
 ---
 
 (Updated Oct. 8, 2014, 11:56 p.m.)
 
 
 Review request for Aurora, David McLaughlin and Zameer Manji.
 
 
 Bugs: aurora-792
 https://issues.apache.org/jira/browse/aurora-792
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Make the large-update check in the client update command consider instance 
 parameters.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/context.py 
 4e94a4e3017f3b2ace38d6d8ce68c3c778c8cd0e 
   src/main/python/apache/aurora/client/cli/jobs.py 
 0277cbed4b7eb927d6e5823b4b41d90b366f81d0 
   src/test/python/apache/aurora/client/cli/BUILD 
 d33e86643a59879c115876c98bd1dc19aa7ae61c 
   src/test/python/apache/aurora/client/cli/test_update.py 
 cff1b6578aec6f5bcc1e610e58b47af233f32b41 
 
 Diff: https://reviews.apache.org/r/26363/diff/
 
 
 Testing
 ---
 
 New unit tests added, all test pass.
 
 
 Thanks,
 
 Mark Chu-Carroll
 




Re: Review Request 26363: Make the large-update check in the client update command consider instance parameters.

2014-10-08 Thread Mark Chu-Carroll


 On Oct. 6, 2014, 12:44 p.m., Joshua Cohen wrote:
  src/test/python/apache/aurora/client/cli/test_update.py, lines 342-353
  https://reviews.apache.org/r/26363/diff/1/?file=714123#file714123line342
 
  This sequence of mocks and writing config to a file is repeated in... 
  many (all?) of these tests. Can you refactor to remove the repetition?

I really think that that should not be done.

For tests, I really like to be able to see, in an instant, exactly what mocks 
are being used by a particular test.  I don't want to have to look somewhere 
else; I don't want to have to mentally combine the stuff that's mocked in a 
common frame and the different stuff that's mocked and/or reconfigured in a 
particular test. The test should be as clear as it can be, and anything from 
the environment that it's mucking with should be done explicitly in the most 
local-possible context.


- Mark


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


On Oct. 6, 2014, 10:57 a.m., Mark Chu-Carroll wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/26363/
 ---
 
 (Updated Oct. 6, 2014, 10:57 a.m.)
 
 
 Review request for Aurora, David McLaughlin and Zameer Manji.
 
 
 Bugs: aurora-792
 https://issues.apache.org/jira/browse/aurora-792
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Make the large-update check in the client update command consider instance 
 parameters.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/context.py 
 301531fcb443297facb78d87a18073c8b7fd4064 
   src/main/python/apache/aurora/client/cli/jobs.py 
 103fb53cb5101d3e025d8712e3887bccdfbb6aeb 
   src/test/python/apache/aurora/client/cli/BUILD 
 8ce6bd5b7faa1579372fb6935180ea982af64af8 
   src/test/python/apache/aurora/client/cli/test_update.py 
 85b1db19d89967a741bfba7964eeb368426f0b61 
 
 Diff: https://reviews.apache.org/r/26363/diff/
 
 
 Testing
 ---
 
 New unit tests added, all test pass.
 
 
 Thanks,
 
 Mark Chu-Carroll
 




Re: Review Request 26363: Make the large-update check in the client update command consider instance parameters.

2014-10-08 Thread Mark Chu-Carroll

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

(Updated Oct. 8, 2014, 1:40 p.m.)


Review request for Aurora, David McLaughlin and Zameer Manji.


Changes
---

Remove debug prints.


Bugs: aurora-792
https://issues.apache.org/jira/browse/aurora-792


Repository: aurora


Description
---

Make the large-update check in the client update command consider instance 
parameters.


Diffs (updated)
-

  src/main/python/apache/aurora/client/cli/context.py 
301531fcb443297facb78d87a18073c8b7fd4064 
  src/main/python/apache/aurora/client/cli/jobs.py 
103fb53cb5101d3e025d8712e3887bccdfbb6aeb 
  src/test/python/apache/aurora/client/cli/BUILD 
8ce6bd5b7faa1579372fb6935180ea982af64af8 
  src/test/python/apache/aurora/client/cli/test_update.py 
85b1db19d89967a741bfba7964eeb368426f0b61 

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


Testing
---

New unit tests added, all test pass.


Thanks,

Mark Chu-Carroll



Re: Review Request 26363: Make the large-update check in the client update command consider instance parameters.

2014-10-08 Thread Joshua Cohen


 On Oct. 6, 2014, 4:44 p.m., Joshua Cohen wrote:
  src/test/python/apache/aurora/client/cli/test_update.py, lines 342-353
  https://reviews.apache.org/r/26363/diff/1/?file=714123#file714123line342
 
  This sequence of mocks and writing config to a file is repeated in... 
  many (all?) of these tests. Can you refactor to remove the repetition?
 
 Mark Chu-Carroll wrote:
 I really think that that should not be done.
 
 For tests, I really like to be able to see, in an instant, exactly what 
 mocks are being used by a particular test.  I don't want to have to look 
 somewhere else; I don't want to have to mentally combine the stuff that's 
 mocked in a common frame and the different stuff that's mocked and/or 
 reconfigured in a particular test. The test should be as clear as it can be, 
 and anything from the environment that it's mucking with should be done 
 explicitly in the most local-possible context.

Not a ship blocker for me, just my perspective, but these mocks are identical 
in several calls, if many client test cases need to mock the exact same set of 
things in the exact same way, it seems worthwhile to simplify that process.


- Joshua


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


On Oct. 8, 2014, 5:40 p.m., Mark Chu-Carroll wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/26363/
 ---
 
 (Updated Oct. 8, 2014, 5:40 p.m.)
 
 
 Review request for Aurora, David McLaughlin and Zameer Manji.
 
 
 Bugs: aurora-792
 https://issues.apache.org/jira/browse/aurora-792
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Make the large-update check in the client update command consider instance 
 parameters.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/context.py 
 301531fcb443297facb78d87a18073c8b7fd4064 
   src/main/python/apache/aurora/client/cli/jobs.py 
 103fb53cb5101d3e025d8712e3887bccdfbb6aeb 
   src/test/python/apache/aurora/client/cli/BUILD 
 8ce6bd5b7faa1579372fb6935180ea982af64af8 
   src/test/python/apache/aurora/client/cli/test_update.py 
 85b1db19d89967a741bfba7964eeb368426f0b61 
 
 Diff: https://reviews.apache.org/r/26363/diff/
 
 
 Testing
 ---
 
 New unit tests added, all test pass.
 
 
 Thanks,
 
 Mark Chu-Carroll
 




Re: Review Request 26363: Make the large-update check in the client update command consider instance parameters.

2014-10-08 Thread Mark Chu-Carroll

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

(Updated Oct. 8, 2014, 7:56 p.m.)


Review request for Aurora, David McLaughlin and Zameer Manji.


Changes
---

rebase


Bugs: aurora-792
https://issues.apache.org/jira/browse/aurora-792


Repository: aurora


Description
---

Make the large-update check in the client update command consider instance 
parameters.


Diffs (updated)
-

  src/main/python/apache/aurora/client/cli/context.py 
4e94a4e3017f3b2ace38d6d8ce68c3c778c8cd0e 
  src/main/python/apache/aurora/client/cli/jobs.py 
0277cbed4b7eb927d6e5823b4b41d90b366f81d0 
  src/test/python/apache/aurora/client/cli/BUILD 
d33e86643a59879c115876c98bd1dc19aa7ae61c 
  src/test/python/apache/aurora/client/cli/test_update.py 
cff1b6578aec6f5bcc1e610e58b47af233f32b41 

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


Testing
---

New unit tests added, all test pass.


Thanks,

Mark Chu-Carroll



Re: Review Request 26363: Make the large-update check in the client update command consider instance parameters.

2014-10-06 Thread Joshua Cohen

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



src/main/python/apache/aurora/client/cli/jobs.py
https://reviews.apache.org/r/26363/#comment95894

Are these debug statements? Should they be removed or switched to log.debug?



src/test/python/apache/aurora/client/cli/test_update.py
https://reviews.apache.org/r/26363/#comment95895

This sequence of mocks and writing config to a file is repeated in... many 
(all?) of these tests. Can you refactor to remove the repetition?


- Joshua Cohen


On Oct. 6, 2014, 2:57 p.m., Mark Chu-Carroll wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/26363/
 ---
 
 (Updated Oct. 6, 2014, 2:57 p.m.)
 
 
 Review request for Aurora, David McLaughlin and Zameer Manji.
 
 
 Bugs: aurora-792
 https://issues.apache.org/jira/browse/aurora-792
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Make the large-update check in the client update command consider instance 
 parameters.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/context.py 
 301531fcb443297facb78d87a18073c8b7fd4064 
   src/main/python/apache/aurora/client/cli/jobs.py 
 103fb53cb5101d3e025d8712e3887bccdfbb6aeb 
   src/test/python/apache/aurora/client/cli/BUILD 
 8ce6bd5b7faa1579372fb6935180ea982af64af8 
   src/test/python/apache/aurora/client/cli/test_update.py 
 85b1db19d89967a741bfba7964eeb368426f0b61 
 
 Diff: https://reviews.apache.org/r/26363/diff/
 
 
 Testing
 ---
 
 New unit tests added, all test pass.
 
 
 Thanks,
 
 Mark Chu-Carroll
 




Re: Review Request 26363: Make the large-update check in the client update command consider instance parameters.

2014-10-06 Thread Zameer Manji

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

Ship it!


Ship It!

- Zameer Manji


On Oct. 6, 2014, 7:57 a.m., Mark Chu-Carroll wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/26363/
 ---
 
 (Updated Oct. 6, 2014, 7:57 a.m.)
 
 
 Review request for Aurora, David McLaughlin and Zameer Manji.
 
 
 Bugs: aurora-792
 https://issues.apache.org/jira/browse/aurora-792
 
 
 Repository: aurora
 
 
 Description
 ---
 
 Make the large-update check in the client update command consider instance 
 parameters.
 
 
 Diffs
 -
 
   src/main/python/apache/aurora/client/cli/context.py 
 301531fcb443297facb78d87a18073c8b7fd4064 
   src/main/python/apache/aurora/client/cli/jobs.py 
 103fb53cb5101d3e025d8712e3887bccdfbb6aeb 
   src/test/python/apache/aurora/client/cli/BUILD 
 8ce6bd5b7faa1579372fb6935180ea982af64af8 
   src/test/python/apache/aurora/client/cli/test_update.py 
 85b1db19d89967a741bfba7964eeb368426f0b61 
 
 Diff: https://reviews.apache.org/r/26363/diff/
 
 
 Testing
 ---
 
 New unit tests added, all test pass.
 
 
 Thanks,
 
 Mark Chu-Carroll