You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Mark Chu-Carroll <mc...@twopensource.com> on 2014/10/06 16:57:15 UTC
Review Request 26363: Make the large-update check in the client update
command consider instance parameters.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/26363/
-----------------------------------------------------------
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.
Posted by Joshua Cohen <jc...@twopensource.com>.
> 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.
Posted by Mark Chu-Carroll <mc...@twopensource.com>.
> 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.
Posted by Joshua Cohen <jc...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
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.
Posted by Mark Chu-Carroll <mc...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by Mark Chu-Carroll <mc...@twopensource.com>.
-----------------------------------------------------------
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.
Posted by Zameer Manji <zm...@twopensource.com>.
-----------------------------------------------------------
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
>
>