Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Migrate Launchpad bug importer to Twisted #682

Closed
imported-from-roundup opened this Issue · 0 comments

1 participant

@imported-from-roundup

Comment by pythonian4000:

This issue is tracked in issue260.


Comment by paulproteus:

If we do this, we have to write our own consumer of the Launchpad API, I think.

That's life. Let's do it. Marking as 0.11.11.


Comment by paulproteus:

Here's the progress of work on this so far:

In general, you don't have to call self.process_queries() with the Launchpad
BugImporter class; that should be handled by the framework.

I notice the new Launchpad importer has no subclass of "BugParser". This is a
convention; it seems it's okay to not follow it. The most idiomatic use of
BugParser can be seen in bugimporters/bugzilla.py

I added a test and some sample data and did a little bit of cleanup in the
attached patches. They should apply cleanly on top of the current state of
armooo's gitorious branch; armooo, feel free to push them on top your branch
immediately.


Comment by paulproteus:

Three more notes:

  1. I might be wrong about when a call to self.push_urls_onto_reactor() is needed.

  2. Feel free to make liberal use of the FakeGetPage() object at the top of
    mysite/customs/tests.py. If you use @mock.patch() to patch out the real getPage
    with that one, then that will let you download arbitrary URLs (so long as you
    save a copy of them somewhere in the repo) during a run of a test.

  3. I can totally see how crazy this code is now. Luckily there are some ideas
    for cleaning up our use of Twisted, which would de-insane it.

I don't think this is going to be done before 0.11.11, but we've made a lot of
really good progress on it! Marking as "later," but I'm hoping we can land it
for 0.11.12.


Comment by paulproteus:

A further thought:

The way we're modeling things now, there is one Launchpad BugTracker per project.

This is a little odd for the following case: if someone adds a bug to their
project, but then within Launchpad the same bug ID is reassigned to some other
project, we might handle things wrong.

It's something we should just make sure to cook up a test for, and be
happy-enough with the results at some point.


Comment by paulproteus:

This looks quite good. Just a few nitpicks:

  • You don't need a LaunchpadQueryModel at all since you don't use it

  • The changes to mysite/customs/bugimporters/base.py should be in their own
    commit. (Good changes, though!)

  • This delta:

  •    # The bug data that show up in bug_collection['entries']
    
  • # is equivalent to what we get back if we asked for the
  • # data on that bug explicitly.
  • for bug in bug_collection['entries']:
  • self.bug_urls.append((bug['web_link'], bug))

arguably reproduces logic available in:

  • def handle_task_data_json(self, data, lp_bug):

So why don't you just call the handle_task_data_json() method?

In asheesh-additions.patch there are some changes that, effectively, make URL
models optional. I suggest adding those to your patchset.

Also, it's easier to review if you do:

git format-patch --stdout master > entire-series-in-one-file.patch

Can you address those items? Beyond those things, this looks great, and I'm
thrilled that we can likely land this quite soon.


Comment by armooo:

* It looks like asheesh-additions.patch it making the TrackerQueryModel optional
from the UI, but customs_twist.Command.update_trackers is based on the last
polled date of the TrackerQueryModel. I could directly use TrackerQueryModel and
not LaunchpadQueryModel (if the django ORM is ok with it). Is this your suggestion?

  • I split the mysite/customs/bugimporters/base.py changes in to a new commit.

  • I removed self.bug_urls, but changed it to a call to process_bugs not
    handle_task_data_json. This way the LaunchpadBug creation stays in one place and
    we have the to call determine_if_finished which seems to be required to stop the
    reactor if no new bugs are found. The other process_bugs methods also take the
    same list of (url, data).


Comment by paulproteus:

I just deployed this.

I made some small additions -- primarily, removing the LaunchpadQueryForm. I
agree that the Model is needed, although I determined the Form is not.

Also the LaunchpadTrackerForm now causes a QueryModel to be created. That way,
the customs_twist.Command.update_trackers code will detect that it should
refresh that Launchpad tracker (as you pointed out).

This is totally great. Thanks!.


File at http://roundup-archive.openhatch.org/bugs/file434/0001-Create-a-launchpad-bugimporter.patch by armooo
File at http://roundup-archive.openhatch.org/bugs/file435/0002-Add-Launchpad-form-and-adjust-views-accordingly.patch by armooo
File at http://roundup-archive.openhatch.org/bugs/file436/0003-Added-bitesized-and-doc-tag-processing.patch by armooo
File at http://roundup-archive.openhatch.org/bugs/file437/0004-Removed-the-old-launchpad-bugtracker.patch by armooo
File at http://roundup-archive.openhatch.org/bugs/file432/asheesh-additions.patch by paulproteus
File at http://roundup-archive.openhatch.org/bugs/file438/launchpad_review2.patch by armooo

Status: resolved
Nosy List: armooo, paulproteus, pythonian4000
Priority: feature
Imported from roundup ID: 367 (view archived page)
Last modified: 2011-12-22.22:04:04

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.