Merge lp:~timrchavez/offspring/offspring-sync-milestones-gracefully-handle-buggy-launchpad-api into lp:offspring

[Timothy R. Chavez]

Gracefully skip over milestones that are not created correctly due to a buggy Launchpad API.

[Kevin McDermott (bigkevmcd)]

#1

+ def test_sync_project_milestones(self):
+ """
+ Test milestone syncing does not abort with bad milestone objects.
+ """
+
+ def test_sync_project_milestones_handles_bad_milestone(self):
+ """
+ Test milestone syncing does not abort with bad milestone objects.
+ """

Something's not quite right, they both have the same docstring ;-)

#2

I appreciate that this is a hangover from the previous implementation...but...

+ try:
+ credentials.load(open(config.get("master", "launchpad_oauth")))
+ launchpad = Launchpad(credentials, LPNET_SERVICE_ROOT)
+ except:
+ launchpad = Launchpad.get_token_and_login(
+ 'Offspring Image Build System', LPNET_SERVICE_ROOT, cachedir)
+ launchpad.credentials.save(
+ file(config.get("master", "launchpad_oauth"), "w"))

the bare except should be fixed with something a bit more appropriate...

#3

I'd like to see a test with a successful run of the script...that doesn't have to hit Launchpad, but there should be a test with sync_milestones returning something other than None

review: Needs Fixing

[lp:~timrchavez/offspring/offspring-sync-milestones-gracefully-handle-buggy-launchpad-api]

148. By Timothy R. Chavez on 2012-06-12

Update the test to actually test against fake Launchpad milestones
to exercise more of the code.

[Timothy R. Chavez (timrchavez)]

> #1
>
> + def test_sync_project_milestones(self):
> + """
> + Test milestone syncing does not abort with bad milestone objects.
> + """
> +
> + def test_sync_project_milestones_handles_bad_milestone(self):
> + """
> + Test milestone syncing does not abort with bad milestone objects.
> + """
>
> Something's not quite right, they both have the same docstring ;-)

Oops. Good catch.

>
> #2
>
> I appreciate that this is a hangover from the previous implementation...but...
>
> + try:
> + credentials.load(open(config.get("master", "launchpad_oauth")))
> + launchpad = Launchpad(credentials, LPNET_SERVICE_ROOT)
> + except:
> + launchpad = Launchpad.get_token_and_login(
> + 'Offspring Image Build System', LPNET_SERVICE_ROOT, cachedir)
> + launchpad.credentials.save(
> + file(config.get("master", "launchpad_oauth"), "w"))
>
> the bare except should be fixed with something a bit more appropriate...
>

I agree with you that bare excepts are trouble, but I'm addressing a specific problem with this MP. I've filed a separate bug to address them: https://bugs.launchpad.net/offspring/+bug/1012487

> #3
>
> I'd like to see a test with a successful run of the script...that doesn't have
> to hit Launchpad, but there should be a test with sync_milestones returning
> something other than None

Not sure I follow here. I re-arranged the script at little as possible to write tests that were specifically designed to validate the fix contained in this MP (though one could argue that only the second test is needed to accomplish this). The point is, an invalid milestone (retrieved from Launchpad and simulated in the second test) should not cause the script to abort. By returning 'None', we prove that we did not abort, and the function returned. I think any other tests fall outside the scope of this MP an will require additional changes to the script (if I understand your request correctly).

I pushed a new branch which makes the first test more solid (actually tests a valid milestone scenario).

[Kevin McDermott (bigkevmcd)]

Looks good to me, +1

review: Approve

[lp:~timrchavez/offspring/offspring-sync-milestones-gracefully-handle-buggy-launchpad-api]

149. By Timothy R. Chavez on 2012-06-13

Get rid if superfulous db commit action and extra space at
the bottom of sync_launchpad_project_milestones.py

=== modified file 'lib/offspring/master/scripts/sync_launchpad_project_milestones.py'
--- lib/offspring/master/scripts/sync_launchpad_project_milestones.py	2012-04-19 19:41:36 +0000
+++ lib/offspring/master/scripts/sync_launchpad_project_milestones.py	2012-06-13 14:30:26 +0000
@@ -4,6 +4,7 @@
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 import os
+import sys
 
 from launchpadlib.credentials import Credentials
 from launchpadlib.launchpad import Launchpad
@@ -17,24 +18,7 @@
     Release
 )
 
-def run():
-    # Setup connection to launchpad
-    config = get_configuration()
-    cachedir = os.path.expanduser("~/.launchpadlib/cache/")
-    credentials = Credentials()
-    try:
-        credentials.load(open(config.get("master", "launchpad_oauth")))
-        launchpad = Launchpad(credentials, LPNET_SERVICE_ROOT)
-    except:
-        launchpad = Launchpad.get_token_and_login(
-            'Offspring Image Build System', LPNET_SERVICE_ROOT, cachedir)
-        launchpad.credentials.save(
-            file(config.get("master", "launchpad_oauth"), "w"))
-
-    # Setup connection to database
-    database = create_database(config.get("master", "db"))
-    store = Store(database)
-
+def sync_milestones(launchpad, store):
     for launchpad_project in store.find(LaunchpadProject):
         # Get launchpad instance of the project
         try:
@@ -50,10 +34,19 @@
         all_milestones = p.all_milestones
 
         # Create new and update existing milestones.
+        sys.stdout.write("I: Creating / updating existing milestones...\n")
+        sys.stdout.flush()
         for m in all_milestones:
-            result = store.find(LaunchpadProjectMilestone,
-                LaunchpadProjectMilestone.launchpad_project == launchpad_project,
-                LaunchpadProjectMilestone.name == unicode(m.name))
+            try:
+                result = store.find(LaunchpadProjectMilestone,
+                    LaunchpadProjectMilestone.launchpad_project == launchpad_project,
+                    LaunchpadProjectMilestone.name == unicode(m.name))
+            except AttributeError:
+                sys.stdout.write(
+                    "W: Could not retrieve the milestone from Launchpad "
+                    "at '%s'\n" % m)
+                sys.stdout.flush()
+                continue
             if result.is_empty():
                 milestone = LaunchpadProjectMilestone(
                     m.name, m.code_name, launchpad_project, m.date_targeted, 
@@ -71,10 +64,25 @@
                 store.commit()
 
         # Delete milestones that no longer exist.
-        rescindedMilestones = set(store.find(LaunchpadProjectMilestone,
-                    LaunchpadProjectMilestone.launchpad_project == launchpad_project
-                    ).values(LaunchpadProjectMilestone.name)
-                ).difference(set([m.name for m in all_milestones]))
+        sys.stdout.write("I: Deleting milestones that no longer exist...\n")
+        sys.stdout.flush()
+
+        milestones = store.find(
+            LaunchpadProjectMilestone,
+            LaunchpadProjectMilestone.launchpad_project == launchpad_project
+        )
+
+        try:
+            rescindedMilestones = set(
+                milestones.values(LaunchpadProjectMilestone.name)
+            ).difference(set([m.name for m in all_milestones]))
+        except AttributeError:
+            sys.stdout.write(
+                "W: Could not retrieve the milestone from Launchpad "
+                "at '%s'\n" % m)
+            sys.stdout.flush()
+            continue
+
         for rescindedMilestoneName in rescindedMilestones:
             result = store.find(
                 LaunchpadProjectMilestone,
@@ -87,3 +95,23 @@
                 ).set(milestone_id=None)
             result.remove()
             store.commit()
+
+def run():
+    # Setup connection to launchpad
+    config = get_configuration()
+    cachedir = os.path.expanduser("~/.launchpadlib/cache/")
+    credentials = Credentials()
+    try:
+        credentials.load(open(config.get("master", "launchpad_oauth")))
+        launchpad = Launchpad(credentials, LPNET_SERVICE_ROOT)
+    except:
+        launchpad = Launchpad.get_token_and_login(
+            'Offspring Image Build System', LPNET_SERVICE_ROOT, cachedir)
+        launchpad.credentials.save(
+            file(config.get("master", "launchpad_oauth"), "w"))
+
+    # Setup connection to database
+    database = create_database(config.get("master", "db"))
+    store = Store(database)
+
+    sync_milestones(launchpad, store)

=== modified file 'lib/offspring/master/tests/helpers.py'
--- lib/offspring/master/tests/helpers.py	2012-04-10 10:18:46 +0000
+++ lib/offspring/master/tests/helpers.py	2012-06-13 14:30:26 +0000
@@ -26,6 +26,26 @@
         self.mails.append((sender, rcpt, msg))
 
 
+class FakeLaunchpadMilestone(object):
+    def __init__(self, name):
+        self.name = name
+        self.code_name = name
+        self.date_targeted = None
+        self.is_active = True
+
+    def __str__(self):
+        return self.code_name
+
+
+class FakeLaunchpadProject(object):
+    def __init__(self, title, milestone):
+        self.title = title
+        self.all_milestones = [milestone]
+
+    def __str__(self):
+        return self.title
+
+
 class FakeBuilder(object):
     def __init__(self, current_state, build_result=None):
         self.name = u"test-builder"
@@ -170,7 +190,7 @@
 
         self.connection.execute(
             "CREATE TABLE buildresults ("
-            "id SERIAL NOT NULL, "
+            "id serial NOT NULL PRIMARY KEY, "
             "name VARCHAR(200), "
             "builder_id INTEGER, "
             "project_name VARCHAR(200) NOT NULL, "
@@ -198,6 +218,36 @@
             "id SERIAL NOT NULL PRIMARY KEY, "
             "dailybuildorder_id integer NOT NULL, "
             "project_id VARCHAR(200) NOT NULL)")
+
+        self.connection.execute(
+            "CREATE TABLE launchpad_projects ( "
+            "name varchar(200) NOT NULL PRIMARY KEY, "
+            "title varchar(200)) ")
+
+        self.connection.execute(
+            "CREATE TABLE launchpad_project_milestones ("
+            "id serial NOT NULL PRIMARY KEY, "
+            "name varchar(200) NOT NULL, "
+            "launchpad_project_id varchar(200) REFERENCES "
+            "launchpad_projects (name) DEFERRABLE INITIALLY DEFERRED, "
+            "title varchar(200) NOT NULL, "
+            "date_targeted date, "
+            "active boolean NOT NULL)")
+
+        self.connection.execute(
+            "CREATE TABLE releases ( "
+            "build_id integer NOT NULL PRIMARY KEY REFERENCES buildresults (id) DEFERRABLE INITIALLY DEFERRED, "
+            "name varchar(200) NOT NULL, "
+            "milestone_id integer REFERENCES launchpad_project_milestones (id) DEFERRABLE INITIALLY DEFERRED, "
+            "tag varchar(5), "
+            "creator_id integer NOT NULL REFERENCES auth_user (id) DEFERRABLE INITIALLY DEFERRED, "
+            "created_at timestamp with time zone NOT NULL, "
+            "updated_at timestamp with time zone NOT NULL, "
+            "published_at timestamp with time zone, "
+            "checklist_url varchar(200) NOT NULL, "
+            "notes text NOT NULL, "
+            "status smallint CHECK (status >= 0) NOT NULL)")
+
         self.connection.commit()
 
     def create_db_store(self):
@@ -208,12 +258,13 @@
         self.db_store.close()
 
     def drop_tables(self):
-        for table in ["auth_user", "projects", "lexbuilders",
-                      "project_notification_subscriptions",
+        for table in ["auth_user", "releases", "launchpad_projects", "projects",
+                      "lexbuilders", "project_notification_subscriptions",
                       "buildresults", "buildrequests", "dailybuildorders",
-                      "dailybuildorders_projects"]:
+                      "dailybuildorders_projects",
+                      "launchpad_project_milestones"]:
             try:
-                self.connection.execute("DROP TABLE %s" % table)
+                self.connection.execute("DROP TABLE %s CASCADE" % table)
                 self.connection.commit()
             except:
                 self.connection.rollback()

=== modified file 'lib/offspring/master/tests/test_utils.py'
--- lib/offspring/master/tests/test_utils.py	2011-12-02 09:03:14 +0000
+++ lib/offspring/master/tests/test_utils.py	2012-06-13 14:30:26 +0000
@@ -7,9 +7,69 @@
 from offspring.master.utils import (
     build_active_projects, process_pending_build_orders)
 from offspring.master.models import (
-    BuildRequest, Project, DailyBuildOrder, DailyBuildOrderProject)
-
-from offspring.master.tests.helpers import BaseOffspringMasterTestCase
+    BuildRequest,
+    Project,
+    DailyBuildOrder,
+    DailyBuildOrderProject,
+    LaunchpadProject,
+    LaunchpadProjectMilestone)
+from offspring.master.scripts import sync_launchpad_project_milestones
+
+from offspring.master.tests.helpers import (
+    BaseOffspringMasterTestCase,
+    FakeLaunchpadProject,
+    FakeLaunchpadMilestone
+)
+
+class TestSyncLaunchpadProjectMilestones(BaseOffspringMasterTestCase):
+
+    def test_sync_project_milestones(self):
+        """
+        Test milestone syncing succeeds.
+        """
+        lp_project = LaunchpadProject()
+        lp_project.name = u"test-project"
+        self.db_store.add(lp_project)
+
+        lp_project_milestone = LaunchpadProjectMilestone(
+            u"test-project-1.0", u"test-project-1.0", lp_project)
+        self.db_store.add(lp_project_milestone)
+
+        launchpad = self.mocker.mock()
+        launchpad.projects["test-project"]
+        fake_lp_milestone = FakeLaunchpadMilestone(u"test-project-1.0")
+        self.mocker.result(FakeLaunchpadProject(u"test-project", fake_lp_milestone))
+        self.mocker.replay()
+
+        self.assertIs(
+            sync_launchpad_project_milestones.sync_milestones(
+                launchpad, self.db_store), None)
+
+
+    def test_sync_project_milestones_handles_bad_milestone(self):
+        """
+        Test milestone syncing does not abort with bad milestone objects.
+        """
+        lp_project = LaunchpadProject()
+        lp_project.name = u"test-project"
+        self.db_store.add(lp_project)
+
+        lp_project_milestone = LaunchpadProjectMilestone(
+            u"test-project-1.0", u"test-project-1.0", lp_project)
+        self.db_store.add(lp_project_milestone)
+
+        launchpad = self.mocker.mock()
+        launchpad.projects[u"test-project"]
+        fake_lp_milestone = FakeLaunchpadMilestone(u"test-project-1.0")
+        delattr(fake_lp_milestone, "name")
+        fake_lp_project = FakeLaunchpadProject(
+            u"test-project", fake_lp_milestone)
+        self.mocker.result(fake_lp_project)
+        self.mocker.replay()
+
+        self.assertIs(
+            sync_launchpad_project_milestones.sync_milestones(
+                launchpad, self.db_store), None)
 
 
 class TestBuildActiveProjects(BaseOffspringMasterTestCase):