You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@aurora.apache.org by Maxim Khutornenko <ma...@apache.org> on 2014/11/22 00:14:10 UTC
Review Request 28350: Add cron replace command.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28350/
-----------------------------------------------------------
Review request for Aurora, David McLaughlin and Bill Farner.
Bugs: AURORA-921
https://issues.apache.org/jira/browse/AURORA-921
Repository: aurora
Description
-------
Implementing "aurora cron replace" command to close the functionality gap that will be created with client updater removal.
Diffs
-----
docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7
docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72
src/main/python/apache/aurora/client/api/__init__.py d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59
src/main/python/apache/aurora/client/cli/cron.py cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2
src/main/python/apache/aurora/client/cli/update.py 12774af8bcd1c953fdbc799b0a142c27407d69f5
src/test/python/apache/aurora/client/api/test_api.py 1f4e9fe9111ac88726d7c45b699b3b91438448b6
src/test/python/apache/aurora/client/cli/test_cron.py c748212febf5867f5f7cc54e34bf91a8890d9999
src/test/python/apache/aurora/client/cli/test_supdate.py 7637352feea6b07408256158814c05bc17ec14f3
Diff: https://reviews.apache.org/r/28350/diff/
Testing
-------
./pants src/test/python:all
tested in vagrant as well
Thanks,
Maxim Khutornenko
Re: Review Request 28350: Add cron replace command.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Nov. 21, 2014, 11:34 p.m., David McLaughlin wrote:
> > src/test/python/apache/aurora/client/cli/test_cron.py, lines 199-215
> > <https://reviews.apache.org/r/28350/diff/1/?file=772866#file772866line199>
> >
> > This is an integration test. Can we replace this with a more fine-grained unit test that only tests against the Replace verb you added? There is an example of this for beta-update start in your review. You'll be able to remove the use of mock.patch and the creation of a temporary file if you do.
I am a bit concerned about this spread of different approaches in command testing but in this particular case it clearly made it simpler. Done.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28350/#review62679
-----------------------------------------------------------
On Nov. 21, 2014, 11:14 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28350/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2014, 11:14 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-921
> https://issues.apache.org/jira/browse/AURORA-921
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing "aurora cron replace" command to close the functionality gap that will be created with client updater removal.
>
>
> Diffs
> -----
>
> docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7
> docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72
> src/main/python/apache/aurora/client/api/__init__.py d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59
> src/main/python/apache/aurora/client/cli/cron.py cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2
> src/main/python/apache/aurora/client/cli/update.py 12774af8bcd1c953fdbc799b0a142c27407d69f5
> src/test/python/apache/aurora/client/api/test_api.py 1f4e9fe9111ac88726d7c45b699b3b91438448b6
> src/test/python/apache/aurora/client/cli/test_cron.py c748212febf5867f5f7cc54e34bf91a8890d9999
> src/test/python/apache/aurora/client/cli/test_supdate.py 7637352feea6b07408256158814c05bc17ec14f3
>
> Diff: https://reviews.apache.org/r/28350/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
> tested in vagrant as well
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 28350: Add cron replace command.
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28350/#review62679
-----------------------------------------------------------
src/test/python/apache/aurora/client/cli/test_cron.py
<https://reviews.apache.org/r/28350/#comment104751>
This is an integration test. Can we replace this with a more fine-grained unit test that only tests against the Replace verb you added? There is an example of this for beta-update start in your review. You'll be able to remove the use of mock.patch and the creation of a temporary file if you do.
- David McLaughlin
On Nov. 21, 2014, 11:14 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28350/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2014, 11:14 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-921
> https://issues.apache.org/jira/browse/AURORA-921
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing "aurora cron replace" command to close the functionality gap that will be created with client updater removal.
>
>
> Diffs
> -----
>
> docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7
> docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72
> src/main/python/apache/aurora/client/api/__init__.py d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59
> src/main/python/apache/aurora/client/cli/cron.py cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2
> src/main/python/apache/aurora/client/cli/update.py 12774af8bcd1c953fdbc799b0a142c27407d69f5
> src/test/python/apache/aurora/client/api/test_api.py 1f4e9fe9111ac88726d7c45b699b3b91438448b6
> src/test/python/apache/aurora/client/cli/test_cron.py c748212febf5867f5f7cc54e34bf91a8890d9999
> src/test/python/apache/aurora/client/cli/test_supdate.py 7637352feea6b07408256158814c05bc17ec14f3
>
> Diff: https://reviews.apache.org/r/28350/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
> tested in vagrant as well
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 28350: Add cron replace command.
Posted by Kevin Sweeney <ke...@apache.org>.
> On Nov. 21, 2014, 3:26 p.m., Kevin Sweeney wrote:
> > docs/cron-jobs.md, line 89
> > <https://reviews.apache.org/r/28350/diff/1/?file=772861#file772861line89>
> >
> > Hold on a sec - why do we have this and cron schedule (see above)?
>
> Maxim Khutornenko wrote:
> The "cron replace" is an atomic convenience command for "cron deschedule" + "cron schedule". It finalizes the cron/service split:
> job create <> cron schedule
> job killall <> cron deschedule (with caveat that active tasks are not killed)
> job update <> cron replace
cron schedule does both "job create" and "job update"
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28350/#review62676
-----------------------------------------------------------
On Nov. 21, 2014, 4:54 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28350/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2014, 4:54 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-921
> https://issues.apache.org/jira/browse/AURORA-921
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing "aurora cron replace" command to close the functionality gap that will be created with client updater removal.
>
>
> Diffs
> -----
>
> docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7
> docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72
> src/main/python/apache/aurora/client/api/__init__.py d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59
> src/main/python/apache/aurora/client/cli/cron.py cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2
> src/main/python/apache/aurora/client/cli/update.py 12774af8bcd1c953fdbc799b0a142c27407d69f5
> src/test/python/apache/aurora/client/api/test_api.py 1f4e9fe9111ac88726d7c45b699b3b91438448b6
> src/test/python/apache/aurora/client/cli/test_cron.py c748212febf5867f5f7cc54e34bf91a8890d9999
> src/test/python/apache/aurora/client/cli/test_supdate.py 7637352feea6b07408256158814c05bc17ec14f3
> src/test/python/apache/aurora/client/cli/util.py 0ec74e675aaabc7ac0cb28e02f5b8534570b7a49
>
> Diff: https://reviews.apache.org/r/28350/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
> tested in vagrant as well
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 28350: Add cron replace command.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Nov. 21, 2014, 11:26 p.m., Kevin Sweeney wrote:
> > docs/cron-jobs.md, line 89
> > <https://reviews.apache.org/r/28350/diff/1/?file=772861#file772861line89>
> >
> > Hold on a sec - why do we have this and cron schedule (see above)?
>
> Maxim Khutornenko wrote:
> The "cron replace" is an atomic convenience command for "cron deschedule" + "cron schedule". It finalizes the cron/service split:
> job create <> cron schedule
> job killall <> cron deschedule (with caveat that active tasks are not killed)
> job update <> cron replace
>
> Kevin Sweeney wrote:
> cron schedule does both "job create" and "job update"
What..? I completely forgot that scheduleCronJob actually acts as "upsert". Should we move the "update" functionality from it in favor of replaceCronTemplate and have our client `cron` commands consistent with `job` commands though? I can argue both ways. Any thoughts?
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28350/#review62676
-----------------------------------------------------------
On Nov. 22, 2014, 12:54 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28350/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2014, 12:54 a.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-921
> https://issues.apache.org/jira/browse/AURORA-921
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing "aurora cron replace" command to close the functionality gap that will be created with client updater removal.
>
>
> Diffs
> -----
>
> docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7
> docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72
> src/main/python/apache/aurora/client/api/__init__.py d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59
> src/main/python/apache/aurora/client/cli/cron.py cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2
> src/main/python/apache/aurora/client/cli/update.py 12774af8bcd1c953fdbc799b0a142c27407d69f5
> src/test/python/apache/aurora/client/api/test_api.py 1f4e9fe9111ac88726d7c45b699b3b91438448b6
> src/test/python/apache/aurora/client/cli/test_cron.py c748212febf5867f5f7cc54e34bf91a8890d9999
> src/test/python/apache/aurora/client/cli/test_supdate.py 7637352feea6b07408256158814c05bc17ec14f3
> src/test/python/apache/aurora/client/cli/util.py 0ec74e675aaabc7ac0cb28e02f5b8534570b7a49
>
> Diff: https://reviews.apache.org/r/28350/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
> tested in vagrant as well
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 28350: Add cron replace command.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Nov. 21, 2014, 11:26 p.m., Kevin Sweeney wrote:
> > docs/cron-jobs.md, line 89
> > <https://reviews.apache.org/r/28350/diff/1/?file=772861#file772861line89>
> >
> > Hold on a sec - why do we have this and cron schedule (see above)?
>
> Maxim Khutornenko wrote:
> The "cron replace" is an atomic convenience command for "cron deschedule" + "cron schedule". It finalizes the cron/service split:
> job create <> cron schedule
> job killall <> cron deschedule (with caveat that active tasks are not killed)
> job update <> cron replace
>
> Kevin Sweeney wrote:
> cron schedule does both "job create" and "job update"
>
> Maxim Khutornenko wrote:
> What..? I completely forgot that scheduleCronJob actually acts as "upsert". Should we move the "update" functionality from it in favor of replaceCronTemplate and have our client `cron` commands consistent with `job` commands though? I can argue both ways. Any thoughts?
>
> Kevin Sweeney wrote:
> I was completely surprised to learn that when I was writing this document as well.
>
> I'm in favor of leaving the existing behavior as-is and removing any unused endpoints.
There are no unused endpoints yet. The replaceCronTemplate is still used by the client updater.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28350/#review62676
-----------------------------------------------------------
On Nov. 22, 2014, 12:54 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28350/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2014, 12:54 a.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-921
> https://issues.apache.org/jira/browse/AURORA-921
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing "aurora cron replace" command to close the functionality gap that will be created with client updater removal.
>
>
> Diffs
> -----
>
> docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7
> docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72
> src/main/python/apache/aurora/client/api/__init__.py d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59
> src/main/python/apache/aurora/client/cli/cron.py cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2
> src/main/python/apache/aurora/client/cli/update.py 12774af8bcd1c953fdbc799b0a142c27407d69f5
> src/test/python/apache/aurora/client/api/test_api.py 1f4e9fe9111ac88726d7c45b699b3b91438448b6
> src/test/python/apache/aurora/client/cli/test_cron.py c748212febf5867f5f7cc54e34bf91a8890d9999
> src/test/python/apache/aurora/client/cli/test_supdate.py 7637352feea6b07408256158814c05bc17ec14f3
> src/test/python/apache/aurora/client/cli/util.py 0ec74e675aaabc7ac0cb28e02f5b8534570b7a49
>
> Diff: https://reviews.apache.org/r/28350/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
> tested in vagrant as well
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 28350: Add cron replace command.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Nov. 21, 2014, 11:26 p.m., Kevin Sweeney wrote:
> > docs/cron-jobs.md, line 89
> > <https://reviews.apache.org/r/28350/diff/1/?file=772861#file772861line89>
> >
> > Hold on a sec - why do we have this and cron schedule (see above)?
The "cron replace" is an atomic convenience command for "cron deschedule" + "cron schedule". It finalizes the cron/service split:
job create <> cron schedule
job killall <> cron deschedule (with caveat that active tasks are not killed)
job update <> cron replace
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28350/#review62676
-----------------------------------------------------------
On Nov. 21, 2014, 11:14 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28350/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2014, 11:14 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-921
> https://issues.apache.org/jira/browse/AURORA-921
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing "aurora cron replace" command to close the functionality gap that will be created with client updater removal.
>
>
> Diffs
> -----
>
> docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7
> docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72
> src/main/python/apache/aurora/client/api/__init__.py d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59
> src/main/python/apache/aurora/client/cli/cron.py cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2
> src/main/python/apache/aurora/client/cli/update.py 12774af8bcd1c953fdbc799b0a142c27407d69f5
> src/test/python/apache/aurora/client/api/test_api.py 1f4e9fe9111ac88726d7c45b699b3b91438448b6
> src/test/python/apache/aurora/client/cli/test_cron.py c748212febf5867f5f7cc54e34bf91a8890d9999
> src/test/python/apache/aurora/client/cli/test_supdate.py 7637352feea6b07408256158814c05bc17ec14f3
>
> Diff: https://reviews.apache.org/r/28350/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
> tested in vagrant as well
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 28350: Add cron replace command.
Posted by Kevin Sweeney <ke...@apache.org>.
> On Nov. 21, 2014, 3:26 p.m., Kevin Sweeney wrote:
> > docs/cron-jobs.md, line 89
> > <https://reviews.apache.org/r/28350/diff/1/?file=772861#file772861line89>
> >
> > Hold on a sec - why do we have this and cron schedule (see above)?
>
> Maxim Khutornenko wrote:
> The "cron replace" is an atomic convenience command for "cron deschedule" + "cron schedule". It finalizes the cron/service split:
> job create <> cron schedule
> job killall <> cron deschedule (with caveat that active tasks are not killed)
> job update <> cron replace
>
> Kevin Sweeney wrote:
> cron schedule does both "job create" and "job update"
>
> Maxim Khutornenko wrote:
> What..? I completely forgot that scheduleCronJob actually acts as "upsert". Should we move the "update" functionality from it in favor of replaceCronTemplate and have our client `cron` commands consistent with `job` commands though? I can argue both ways. Any thoughts?
I was completely surprised to learn that when I was writing this document as well.
I'm in favor of leaving the existing behavior as-is and removing any unused endpoints.
- Kevin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28350/#review62676
-----------------------------------------------------------
On Nov. 21, 2014, 4:54 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28350/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2014, 4:54 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-921
> https://issues.apache.org/jira/browse/AURORA-921
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing "aurora cron replace" command to close the functionality gap that will be created with client updater removal.
>
>
> Diffs
> -----
>
> docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7
> docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72
> src/main/python/apache/aurora/client/api/__init__.py d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59
> src/main/python/apache/aurora/client/cli/cron.py cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2
> src/main/python/apache/aurora/client/cli/update.py 12774af8bcd1c953fdbc799b0a142c27407d69f5
> src/test/python/apache/aurora/client/api/test_api.py 1f4e9fe9111ac88726d7c45b699b3b91438448b6
> src/test/python/apache/aurora/client/cli/test_cron.py c748212febf5867f5f7cc54e34bf91a8890d9999
> src/test/python/apache/aurora/client/cli/test_supdate.py 7637352feea6b07408256158814c05bc17ec14f3
> src/test/python/apache/aurora/client/cli/util.py 0ec74e675aaabc7ac0cb28e02f5b8534570b7a49
>
> Diff: https://reviews.apache.org/r/28350/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
> tested in vagrant as well
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 28350: Add cron replace command.
Posted by Kevin Sweeney <ke...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28350/#review62676
-----------------------------------------------------------
docs/cron-jobs.md
<https://reviews.apache.org/r/28350/#comment104746>
Hold on a sec - why do we have this and cron schedule (see above)?
- Kevin Sweeney
On Nov. 21, 2014, 3:14 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28350/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2014, 3:14 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-921
> https://issues.apache.org/jira/browse/AURORA-921
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing "aurora cron replace" command to close the functionality gap that will be created with client updater removal.
>
>
> Diffs
> -----
>
> docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7
> docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72
> src/main/python/apache/aurora/client/api/__init__.py d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59
> src/main/python/apache/aurora/client/cli/cron.py cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2
> src/main/python/apache/aurora/client/cli/update.py 12774af8bcd1c953fdbc799b0a142c27407d69f5
> src/test/python/apache/aurora/client/api/test_api.py 1f4e9fe9111ac88726d7c45b699b3b91438448b6
> src/test/python/apache/aurora/client/cli/test_cron.py c748212febf5867f5f7cc54e34bf91a8890d9999
> src/test/python/apache/aurora/client/cli/test_supdate.py 7637352feea6b07408256158814c05bc17ec14f3
>
> Diff: https://reviews.apache.org/r/28350/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
> tested in vagrant as well
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 28350: Add cron replace command.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Nov. 21, 2014, 11:32 p.m., Bill Farner wrote:
> > src/test/python/apache/aurora/client/cli/test_cron.py, lines 218-227
> > <https://reviews.apache.org/r/28350/diff/1/?file=772866#file772866line218>
> >
> > This block is repeated ~verbatim 4x in this file. Cleaning up the underlying code to improve testing is probably too high of a bar, but you could at least extract a helper function to minimize the spread.
Obviated by refactoring.
> On Nov. 21, 2014, 11:32 p.m., Bill Farner wrote:
> > docs/cron-jobs.md, line 90
> > <https://reviews.apache.org/r/28350/diff/1/?file=772861#file772861line90>
> >
> > This makes it seem like only the _execution schedule_ is changed by this command, not the template. Suggest rewording to mkae it more obvious that this will update subsequent cron executions.
Good point. Reworded.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28350/#review62675
-----------------------------------------------------------
On Nov. 21, 2014, 11:14 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28350/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2014, 11:14 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-921
> https://issues.apache.org/jira/browse/AURORA-921
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing "aurora cron replace" command to close the functionality gap that will be created with client updater removal.
>
>
> Diffs
> -----
>
> docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7
> docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72
> src/main/python/apache/aurora/client/api/__init__.py d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59
> src/main/python/apache/aurora/client/cli/cron.py cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2
> src/main/python/apache/aurora/client/cli/update.py 12774af8bcd1c953fdbc799b0a142c27407d69f5
> src/test/python/apache/aurora/client/api/test_api.py 1f4e9fe9111ac88726d7c45b699b3b91438448b6
> src/test/python/apache/aurora/client/cli/test_cron.py c748212febf5867f5f7cc54e34bf91a8890d9999
> src/test/python/apache/aurora/client/cli/test_supdate.py 7637352feea6b07408256158814c05bc17ec14f3
>
> Diff: https://reviews.apache.org/r/28350/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
> tested in vagrant as well
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 28350: Add cron replace command.
Posted by Maxim Khutornenko <ma...@apache.org>.
> On Nov. 21, 2014, 11:32 p.m., Bill Farner wrote:
> > src/main/python/apache/aurora/client/api/__init__.py, line 67
> > <https://reviews.apache.org/r/28350/diff/1/?file=772862#file772862line67>
> >
> > We seem to be firmly divided on the quoting convention. Seems like some more formal agreement is needed to keep this kind of change from being undone by the next passer-by:
> > ```
> > $ grep -ro '"' src/{main,test}/python | wc -l
> > 13055
> > $ grep -ro "'" src/{main,test}/python | wc -l
> > 16243
> > ```
All I am doing here is bringing everything in this file to the common denominator.
> On Nov. 21, 2014, 11:32 p.m., Bill Farner wrote:
> > src/test/python/apache/aurora/client/cli/test_cron.py, line 211
> > <https://reviews.apache.org/r/28350/diff/1/?file=772866#file772866line211>
> >
> > Newly-established convention is to assert on `mock_calls` for better coverage. Here and elsewhere.
The mock_calls is useful only if you can assert its content, which is not really the case here. Perhaps it's the reason alone to move off of file-based tests.
- Maxim
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28350/#review62675
-----------------------------------------------------------
On Nov. 21, 2014, 11:14 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28350/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2014, 11:14 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-921
> https://issues.apache.org/jira/browse/AURORA-921
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing "aurora cron replace" command to close the functionality gap that will be created with client updater removal.
>
>
> Diffs
> -----
>
> docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7
> docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72
> src/main/python/apache/aurora/client/api/__init__.py d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59
> src/main/python/apache/aurora/client/cli/cron.py cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2
> src/main/python/apache/aurora/client/cli/update.py 12774af8bcd1c953fdbc799b0a142c27407d69f5
> src/test/python/apache/aurora/client/api/test_api.py 1f4e9fe9111ac88726d7c45b699b3b91438448b6
> src/test/python/apache/aurora/client/cli/test_cron.py c748212febf5867f5f7cc54e34bf91a8890d9999
> src/test/python/apache/aurora/client/cli/test_supdate.py 7637352feea6b07408256158814c05bc17ec14f3
>
> Diff: https://reviews.apache.org/r/28350/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
> tested in vagrant as well
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 28350: Add cron replace command.
Posted by Bill Farner <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28350/#review62675
-----------------------------------------------------------
docs/cron-jobs.md
<https://reviews.apache.org/r/28350/#comment104745>
This makes it seem like only the _execution schedule_ is changed by this command, not the template. Suggest rewording to mkae it more obvious that this will update subsequent cron executions.
src/main/python/apache/aurora/client/api/__init__.py
<https://reviews.apache.org/r/28350/#comment104747>
We seem to be firmly divided on the quoting convention. Seems like some more formal agreement is needed to keep this kind of change from being undone by the next passer-by:
```
$ grep -ro '"' src/{main,test}/python | wc -l
13055
$ grep -ro "'" src/{main,test}/python | wc -l
16243
```
src/test/python/apache/aurora/client/cli/test_cron.py
<https://reviews.apache.org/r/28350/#comment104748>
Newly-established convention is to assert on `mock_calls` for better coverage. Here and elsewhere.
src/test/python/apache/aurora/client/cli/test_cron.py
<https://reviews.apache.org/r/28350/#comment104750>
This block is repeated ~verbatim 4x in this file. Cleaning up the underlying code to improve testing is probably too high of a bar, but you could at least extract a helper function to minimize the spread.
- Bill Farner
On Nov. 21, 2014, 11:14 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28350/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2014, 11:14 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-921
> https://issues.apache.org/jira/browse/AURORA-921
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing "aurora cron replace" command to close the functionality gap that will be created with client updater removal.
>
>
> Diffs
> -----
>
> docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7
> docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72
> src/main/python/apache/aurora/client/api/__init__.py d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59
> src/main/python/apache/aurora/client/cli/cron.py cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2
> src/main/python/apache/aurora/client/cli/update.py 12774af8bcd1c953fdbc799b0a142c27407d69f5
> src/test/python/apache/aurora/client/api/test_api.py 1f4e9fe9111ac88726d7c45b699b3b91438448b6
> src/test/python/apache/aurora/client/cli/test_cron.py c748212febf5867f5f7cc54e34bf91a8890d9999
> src/test/python/apache/aurora/client/cli/test_supdate.py 7637352feea6b07408256158814c05bc17ec14f3
>
> Diff: https://reviews.apache.org/r/28350/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
> tested in vagrant as well
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 28350: Add cron replace command.
Posted by David McLaughlin <da...@dmclaughlin.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28350/#review62693
-----------------------------------------------------------
Ship it!
Ship It!
- David McLaughlin
On Nov. 22, 2014, 12:54 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28350/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2014, 12:54 a.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-921
> https://issues.apache.org/jira/browse/AURORA-921
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing "aurora cron replace" command to close the functionality gap that will be created with client updater removal.
>
>
> Diffs
> -----
>
> docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7
> docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72
> src/main/python/apache/aurora/client/api/__init__.py d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59
> src/main/python/apache/aurora/client/cli/cron.py cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2
> src/main/python/apache/aurora/client/cli/update.py 12774af8bcd1c953fdbc799b0a142c27407d69f5
> src/test/python/apache/aurora/client/api/test_api.py 1f4e9fe9111ac88726d7c45b699b3b91438448b6
> src/test/python/apache/aurora/client/cli/test_cron.py c748212febf5867f5f7cc54e34bf91a8890d9999
> src/test/python/apache/aurora/client/cli/test_supdate.py 7637352feea6b07408256158814c05bc17ec14f3
> src/test/python/apache/aurora/client/cli/util.py 0ec74e675aaabc7ac0cb28e02f5b8534570b7a49
>
> Diff: https://reviews.apache.org/r/28350/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
> tested in vagrant as well
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 28350: Add cron replace command.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28350/#review62817
-----------------------------------------------------------
Discarding this in favor of an "upserting" cron schedule command. Will send a new diff to fix help/messaging.
- Maxim Khutornenko
On Nov. 22, 2014, 12:54 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28350/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2014, 12:54 a.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-921
> https://issues.apache.org/jira/browse/AURORA-921
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing "aurora cron replace" command to close the functionality gap that will be created with client updater removal.
>
>
> Diffs
> -----
>
> docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7
> docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72
> src/main/python/apache/aurora/client/api/__init__.py d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59
> src/main/python/apache/aurora/client/cli/cron.py cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2
> src/main/python/apache/aurora/client/cli/update.py 12774af8bcd1c953fdbc799b0a142c27407d69f5
> src/test/python/apache/aurora/client/api/test_api.py 1f4e9fe9111ac88726d7c45b699b3b91438448b6
> src/test/python/apache/aurora/client/cli/test_cron.py c748212febf5867f5f7cc54e34bf91a8890d9999
> src/test/python/apache/aurora/client/cli/test_supdate.py 7637352feea6b07408256158814c05bc17ec14f3
> src/test/python/apache/aurora/client/cli/util.py 0ec74e675aaabc7ac0cb28e02f5b8534570b7a49
>
> Diff: https://reviews.apache.org/r/28350/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
> tested in vagrant as well
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 28350: Add cron replace command.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28350/#review62700
-----------------------------------------------------------
Ship it!
Master (a431b1d) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Nov. 22, 2014, 12:54 a.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28350/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2014, 12:54 a.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-921
> https://issues.apache.org/jira/browse/AURORA-921
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing "aurora cron replace" command to close the functionality gap that will be created with client updater removal.
>
>
> Diffs
> -----
>
> docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7
> docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72
> src/main/python/apache/aurora/client/api/__init__.py d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59
> src/main/python/apache/aurora/client/cli/cron.py cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2
> src/main/python/apache/aurora/client/cli/update.py 12774af8bcd1c953fdbc799b0a142c27407d69f5
> src/test/python/apache/aurora/client/api/test_api.py 1f4e9fe9111ac88726d7c45b699b3b91438448b6
> src/test/python/apache/aurora/client/cli/test_cron.py c748212febf5867f5f7cc54e34bf91a8890d9999
> src/test/python/apache/aurora/client/cli/test_supdate.py 7637352feea6b07408256158814c05bc17ec14f3
> src/test/python/apache/aurora/client/cli/util.py 0ec74e675aaabc7ac0cb28e02f5b8534570b7a49
>
> Diff: https://reviews.apache.org/r/28350/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
> tested in vagrant as well
>
>
> Thanks,
>
> Maxim Khutornenko
>
>
Re: Review Request 28350: Add cron replace command.
Posted by Maxim Khutornenko <ma...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28350/
-----------------------------------------------------------
(Updated Nov. 22, 2014, 12:54 a.m.)
Review request for Aurora, David McLaughlin and Bill Farner.
Changes
-------
CR comments.
Bugs: AURORA-921
https://issues.apache.org/jira/browse/AURORA-921
Repository: aurora
Description
-------
Implementing "aurora cron replace" command to close the functionality gap that will be created with client updater removal.
Diffs (updated)
-----
docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7
docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72
src/main/python/apache/aurora/client/api/__init__.py d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59
src/main/python/apache/aurora/client/cli/cron.py cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2
src/main/python/apache/aurora/client/cli/update.py 12774af8bcd1c953fdbc799b0a142c27407d69f5
src/test/python/apache/aurora/client/api/test_api.py 1f4e9fe9111ac88726d7c45b699b3b91438448b6
src/test/python/apache/aurora/client/cli/test_cron.py c748212febf5867f5f7cc54e34bf91a8890d9999
src/test/python/apache/aurora/client/cli/test_supdate.py 7637352feea6b07408256158814c05bc17ec14f3
src/test/python/apache/aurora/client/cli/util.py 0ec74e675aaabc7ac0cb28e02f5b8534570b7a49
Diff: https://reviews.apache.org/r/28350/diff/
Testing
-------
./pants src/test/python:all
tested in vagrant as well
Thanks,
Maxim Khutornenko
Re: Review Request 28350: Add cron replace command.
Posted by Aurora ReviewBot <wf...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/28350/#review62678
-----------------------------------------------------------
Ship it!
Master (ecc3fbc) is green with this patch.
./build-support/jenkins/build.sh
I will refresh this build result if you post a review containing "@ReviewBot retry"
- Aurora ReviewBot
On Nov. 21, 2014, 11:14 p.m., Maxim Khutornenko wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/28350/
> -----------------------------------------------------------
>
> (Updated Nov. 21, 2014, 11:14 p.m.)
>
>
> Review request for Aurora, David McLaughlin and Bill Farner.
>
>
> Bugs: AURORA-921
> https://issues.apache.org/jira/browse/AURORA-921
>
>
> Repository: aurora
>
>
> Description
> -------
>
> Implementing "aurora cron replace" command to close the functionality gap that will be created with client updater removal.
>
>
> Diffs
> -----
>
> docs/clientv2.md 6e69af3837f63321e273f1df7b0f5a1c07505cc7
> docs/cron-jobs.md 7d04bcd3776bfd85f14e4398c4bc57dfbbdbcc72
> src/main/python/apache/aurora/client/api/__init__.py d12132f8ee64a390c9b5b4c9ab26b5e4b2b1bb59
> src/main/python/apache/aurora/client/cli/cron.py cfd957cb99fb1c0bff04f0c91969a66faaf3b6f2
> src/main/python/apache/aurora/client/cli/update.py 12774af8bcd1c953fdbc799b0a142c27407d69f5
> src/test/python/apache/aurora/client/api/test_api.py 1f4e9fe9111ac88726d7c45b699b3b91438448b6
> src/test/python/apache/aurora/client/cli/test_cron.py c748212febf5867f5f7cc54e34bf91a8890d9999
> src/test/python/apache/aurora/client/cli/test_supdate.py 7637352feea6b07408256158814c05bc17ec14f3
>
> Diff: https://reviews.apache.org/r/28350/diff/
>
>
> Testing
> -------
>
> ./pants src/test/python:all
>
> tested in vagrant as well
>
>
> Thanks,
>
> Maxim Khutornenko
>
>