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

Issue 14811043: Make queue and ingest handle prior charm revs.

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 6 months ago by bac
Modified:
10 years, 6 months ago
Reviewers:
rharding, mp+191731
Visibility:
Public.

Description

Make queue and ingest handle prior charm revs. In order to support multiple charm revisions, the queueing and ingest processes must support older revisions of charms. Up to 10 old revisions of a charm are queued up for ingesting. A charm that is not the latest revision is not updated in the database if it already exists. A current revision of the charm is updated even if it exists. This updating may not be required as a charm cannot change without updating the revision. This branch is a continuation of Benji's work found at: lp:~benji/charmworld/backfilling-first-cut Notes on the adoption of the branch are at: https://docs.google.com/a/canonical.com/document/d/1hGRugwNgwRLjV_uP7W8UYx1ejfKrU1RWw7NoNBPQOzQ TODO: * Make the number of revision configurable through charmworld.ini. * Remove the charm reversal hack now that only the latest is updated. QA: % scripts/reset-db % HOME=_run INI=charmworld.ini bin/enqueue --prefix=~charmers/charms/precise % HOME=_run INI=charmworld.ini bin/ingest-queued Redirect stderr from the above to a file 'stderr.txt'. grep -c Queueing stderr.txt grep -c Saving stderr.txt should be equal to the mongo charm count: % mongo juju > db.charms.count() Next, without resetting run % HOME=_run INI=charmworld.ini bin/enqueue --prefix=~charmers/charms/precise % HOME=_run INI=charmworld.ini bin/ingest-queued grep -c Queueing stderr.txt grep -c Saving stderr.txt grep -c Skipping stderr.txt should show some charms being skipped. Note there is no support in the Charmworld UI for showing older charm revisions yet. https://code.launchpad.net/~bac/charmworld/backfilling/+merge/191731 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 12
Unified diffs Side-by-side diffs Delta from patch set Stats (+233 lines, -97 lines) Patch
M .bzrignore View 1 chunk +1 line, -0 lines 0 comments Download
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M charmworld/charmstore.py View 2 chunks +23 lines, -4 lines 2 comments Download
M charmworld/jobs/ingest.py View 10 chunks +53 lines, -29 lines 6 comments Download
M charmworld/jobs/lp.py View 4 chunks +26 lines, -12 lines 0 comments Download
M charmworld/jobs/tests/test_bzr.py View 5 chunks +5 lines, -7 lines 0 comments Download
M charmworld/jobs/tests/test_ingest.py View 7 chunks +33 lines, -14 lines 2 comments Download
M charmworld/jobs/tests/test_lp.py View 4 chunks +12 lines, -5 lines 0 comments Download
M charmworld/jobs/tests/test_scan.py View 7 chunks +7 lines, -7 lines 0 comments Download
M charmworld/tests/test_charmstore.py View 1 chunk +70 lines, -18 lines 2 comments Download
M charmworld/views/charms.py View 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 6
bac
Please take a look.
10 years, 6 months ago (2013-10-17 22:00:59 UTC) #1
rharding
Code looks good. Just some a question and a typo. The rest are more thoughts ...
10 years, 6 months ago (2013-10-18 12:30:01 UTC) #2
rharding
QA was no good. I didn't get an skipping. I tried to QA by using ...
10 years, 6 months ago (2013-10-18 12:57:49 UTC) #3
rharding
On 2013/10/18 12:57:49, rharding wrote: > QA was no good. I didn't get an skipping. ...
10 years, 6 months ago (2013-10-18 13:06:46 UTC) #4
rharding
LGTM qa-ok
10 years, 6 months ago (2013-10-18 14:26:02 UTC) #5
bac
10 years, 6 months ago (2013-10-18 15:43:48 UTC) #6
Thanks for the review and landing.  As we discussed on IRC the typo you
mentioned did not get fixed.  I think I'll just do a quick trivial branch to fix
it.

https://codereview.appspot.com/14811043/diff/1/charmworld/jobs/ingest.py
File charmworld/jobs/ingest.py (left):

https://codereview.appspot.com/14811043/diff/1/charmworld/jobs/ingest.py#oldc...
charmworld/jobs/ingest.py:102: def do_bzr_update(charm_data, db, fs, log,
root_dir=None, revisionId=None):
I'm surprised the linter doesn't complain about unused arguments.

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

https://codereview.appspot.com/14811043/diff/1/charmworld/jobs/ingest.py#newc...
charmworld/jobs/ingest.py:248: return this_revision == newest_revision or not
charm_in_db
Yes, that is the plan.  I don't think it is strictly necessary as a charm cannot
be updated without the charmstore bumping its version but I left this in to be
safe since there was some mention in the code about clearing out error state in
tip of a charm.  Upon more investigation this condition may be able to be
tightened.

https://codereview.appspot.com/14811043/diff/1/charmworld/jobs/ingest.py#newc...
charmworld/jobs/ingest.py:294: update_charm(charm_data, self.db, CharmStore())
Yeah, old versions won't get statistics updated.  That may be a big deal.

I don't understand your question about the API.

https://codereview.appspot.com/14811043/diff/1/charmworld/jobs/tests/test_ing...
File charmworld/jobs/tests/test_ingest.py (left):

https://codereview.appspot.com/14811043/diff/1/charmworld/jobs/tests/test_ing...
charmworld/jobs/tests/test_ingest.py:194: def test_setting_charm_id(self):
It tests the setting of the _ids in the payload as a side-effect.  We now make a
clean copy and don't modify the passed payload so the test is not needed.

https://codereview.appspot.com/14811043/diff/1/charmworld/tests/test_charmsto...
File charmworld/tests/test_charmstore.py (right):

https://codereview.appspot.com/14811043/diff/1/charmworld/tests/test_charmsto...
charmworld/tests/test_charmstore.py:241: # With five revisions.
C-n-p.  Darn.
Sign in to reply to this message.

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