Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(108)

Issue 45440043: Run charm/bundle queue and ingest from one script

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years, 3 months ago by bac
Modified:
6 years, 3 months ago
Reviewers:
mp+200027, benji-york
Visibility:
Public.

Description

Run charm/bundle queue and ingest from one script Originally, queueing was run from a cronjob every fifteen minutes. Stuff was added without regard to it being queued up already. Ingest was run continuously but slept fifteen minutes when it completed before looking at the queue again. This change makes the supervisord-run worker do the queue job, then ingest charms and bundles. At the end, it sleeps for fifteen minutes minus the amount spent doing the work. The charmworld charm needs to be fixed to not create the cronjob for queueing. It hasn't been done yet but the bin/queued script that is called by the cronjob needs to be disabled before this change lands or the queueing will be run by two different approaches. https://code.launchpad.net/~bac/charmworld/make-queue-a-job/+merge/200027 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+147 lines, -43 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M charmworld/jobs/lp.py View 4 chunks +14 lines, -6 lines 1 comment Download
M charmworld/jobs/tests/test_lp.py View 4 chunks +30 lines, -0 lines 3 comments Download
M charmworld/jobs/tests/test_worker.py View 8 chunks +56 lines, -26 lines 1 comment Download
M charmworld/jobs/worker.py View 5 chunks +26 lines, -11 lines 0 comments Download
M charmworld/testing/__init__.py View 2 chunks +19 lines, -0 lines 0 comments Download

Messages

Total messages: 3
bac
Please take a look.
6 years, 3 months ago (2013-12-24 20:49:57 UTC) #1
bac
Ignore my comment about the cronjob. It seems bin/enqueue is handy to keep around so ...
6 years, 3 months ago (2013-12-24 21:02:47 UTC) #2
benji-york
6 years, 3 months ago (2014-01-02 16:30:36 UTC) #3
LGTM -- there is one critical bug that should be easy to fix (and probably
deserves a test so something like that doesn't happen again).

https://codereview.appspot.com/45440043/diff/1/charmworld/jobs/lp.py
File charmworld/jobs/lp.py (right):

https://codereview.appspot.com/45440043/diff/1/charmworld/jobs/lp.py#newcode206
charmworld/jobs/lp.py:206: def main(arglist):
The bin/enqueue script is an entry point wrapper around this function and
doesn't pass any arguments, so if you run bin/enqueue you'll get

    TypeError: main() takes exactly 1 argument (0 given)

I would use a default argument of None and add this to the body of main:

if arglist is None:
    arglist = sys.argv[1:]

https://codereview.appspot.com/45440043/diff/1/charmworld/jobs/tests/test_lp.py
File charmworld/jobs/tests/test_lp.py (right):

https://codereview.appspot.com/45440043/diff/1/charmworld/jobs/tests/test_lp....
charmworld/jobs/tests/test_lp.py:374: namespace = Namespace()
It took a couple of readings for me to understand this test.  Perhaps naming
"namespace" "expected" instead would have helped.

https://codereview.appspot.com/45440043/diff/1/charmworld/jobs/tests/test_lp....
charmworld/jobs/tests/test_lp.py:375: setattr(namespace, 'prefix', '~sinzui')
I'm curious as to why you used setattr instead of "namespace.prefix =
'~sinzui'".

https://codereview.appspot.com/45440043/diff/1/charmworld/jobs/tests/test_lp....
charmworld/jobs/tests/test_lp.py:380: 
This looks like an errant newline.  Or maybe a space reserved for a forgotten
comment.

https://codereview.appspot.com/45440043/diff/1/charmworld/jobs/tests/test_wor...
File charmworld/jobs/tests/test_worker.py (right):

https://codereview.appspot.com/45440043/diff/1/charmworld/jobs/tests/test_wor...
charmworld/jobs/tests/test_worker.py:231: def test_lp_run_limit(self):
I'm a big fan of the
first-line-of-a-test-is-a-comment-describing-what-the-test-is-about pattern. 
Hint, hint. ;)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b