charmworld

Merge lp://staging/~sinzui/charmworld/tools-use-charm-model into lp://staging/~juju-jitsu/charmworld/trunk

  1. tools-use-charm-model
  2. Merge into trunk
Proposed by Curtis Hovey
Status: Merged
Approved by: Curtis Hovey
Approved revision: 278
Merged at revision: 269
Proposed branch: lp://staging/~sinzui/charmworld/tools-use-charm-model
Merge into: lp://staging/~juju-jitsu/charmworld/trunk
Diff against target: 412 lines (+163/-68)
9 files modified
charmworld/models.py (+8/-0)
charmworld/templates/charm-proof-errors.pt (+1/-1)
charmworld/templates/recent.pt (+3/-3)
charmworld/templates/store-missing.pt (+5/-5)
charmworld/tests/test_models.py (+7/-0)
charmworld/views/feeds.py (+31/-44)
charmworld/views/tests/test_feeds.py (+86/-2)
charmworld/views/tests/test_tools.py (+17/-2)
charmworld/views/tools.py (+5/-11)
To merge this branch: bzr merge lp://staging/~sinzui/charmworld/tools-use-charm-model
Reviewer Review Type Date Requested Status
Curtis Hovey (community) code Approve
Richard Harding (community) Approve
Review via email: mp+169813@code.staging.launchpad.net

Commit message

Allow users to view the listing of charms with store errors.

Description of the change

The KeyError: 'short_url' error seen on m.jc.com is ultimately caused
by inconsistencies in charm data. The definition of required and optional
charm attributes has changed over time, but charms were not updated to
meet the current definition. There is no guarantee that a defective
charm has the required properties.

RULES

    This branch. pre-implementation: abentley
    * Update the remaining views to use the Charm model.
    * This branch fixes the last occurrences of the KeyError: 'short_url'
      bug.

    Next Branch:
    * Remove the shims that cast charm dicts to Charm objects. All
      view call sites should be working with the object now.

QA

    The tools modules
    * Visit http://charmworld:2464/tools/store-missing
    * Verify the page loads...fixing the The KeyError: 'short_url' problem.
      The page lists the charms that have store errors like:
      ~benji/charms/precise/juju-gui/trunk
    * Visit http://staging.jujucharms.com/tools/proof-errors
    * Verify the links to the charmworld charms work.

    The feeds module
    * Visit http://staging.jujucharms.com/recently-changed
    * Verify links to the charmworld charms and the branch revisions work.
    * Visit http://staging.jujucharms.com/feed/charm-changes
    * Verify the atom feed loads and the first entry tags are for the
      same charms from the previous page.
    * Visit http://staging.jujucharms.com/charms/precise/apache2/changes
    * Verify the atom loads.
    * Visit http://staging.jujucharms.com/~abentley/precise/charmworld/changes
    * Verify the atom loads.

IMPLEMENTATION

      * The updates to the templates are mechanical.
      * I updated the Charm object to support __eq__ to simplify testing.
      * The feed module was a nuance. I first added tests for the current
        behaviour. I next refactored out the duplicate implementations
        in the module. I finally updated the module to use the Charm object.
        * I also removed a comment about promulgated branches that was no
          longer true.
      * I updated the tools module to us the Charm object. I choose to
        cast the dict as a Charm during the phase where the db results
        were cast as a list.

To post a comment you must log in.
Revision history for this message
Richard Harding (rharding) wrote :

Thanks for the update. Just a couple of questions but r=me

#63 Should we add something in models to map the db find to? It can then do the Charm(data) and just return a list of objects. Mini-model hydrate helper.

#286 Can you add a comment on the response.ubody.replace bit there? I'm assuming that the manual method call didn't render out the right ns due to headers or something?

review: Approve
Revision history for this message
Curtis Hovey (sinzui) wrote :

Thank you for the the review.

#63 might be the right thing to do. My next clean up branch is dedicated to removing cruft I left behind. There are several method like bzr_url that could be simple formatting properties, like the age formatter.

I will add the comment, sorry for not making it clear.

Revision history for this message
Charmworld Lander (charmworld-lander) wrote :

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

Revision history for this message
Curtis Hovey (sinzui) wrote :

I approve my revision for Rick.

review: Approve (code)
Revision history for this message
Abel Deuring (adeuring) wrote :

> + def __eq__(self, other):
> + if (isinstance(other, self.__class__)
> + and self.store_url != ''
> + and self.store_url == other.store_url):
> + # The store thinks these two charms are the same.
> + return True
> + return False

This bothered me for some time: Promulgated charms use the short variant
"cs:series/charm_name" as the store URL. At least while ingest is
running, we may have for a short time two different charms with
identical store URLs:

Let cs:~owner-b/series/charm-name be the promulgated charm. Then another
charm cs:~owner-a/series/charm-name is promulgated instead. The ingest
script sorts the charms alphabetically for processing, so the new
promulgated charm is processed first, and while the former promulgated
charm is not yet processed, we have two different charms with the same
store URL in the Mongo DB.

I think it would be better to use charm.branch_spec in __eq__()

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches