Merge lp:~mabac/svammel/bug-749561-set-series-and-milestone into lp:svammel

[Mattias Backman]

Hi,

This branch adds the option to set milestones and series on newly created bugs.

--milestone and --target-series options are added.

Milestone and series are looked up in Ubuntu and on any error setting them, the bug is closed. I have limited permissions for nominating series, so everything is not thoroughly tested.

Thanks,

Mattias

[Mattias Backman (mabac)]

Reminder to self:

147 + importance=args.importance, milestone=args.milestone,

should be milestone_name=

[Mattias Backman (mabac)]

Try something like this

    $ python file-failures.py --archive test-rebuild-20110114-arm -vv --bugtag testseries --bugtag svammel --logfile logstatus.log --serviceroot qastaging --target-series natty --milestone ubuntu-11.04

There is a namespace clash on the command line with --series and --target-series which probably increases confusion.

If setting either milestone or series fails the bug will be closed and the script bails. Unless --ignore-errors is passed.

[Matthias Klose (doko)]

looks fine.

+ # createBug creates only one bugtask, but addNomination
+ # may add another, so we use the last one.
+ set_bugtask_milestone(bug.bug_tasks[0:2][-1], milestone)

just wondering how robust this is ...

[Mattias Backman (mabac)]

> + # createBug creates only one bugtask, but addNomination
> + # may add another, so we use the last one.
> + set_bugtask_milestone(bug.bug_tasks[0:2][-1], milestone)
>
> just wondering how robust this is ...

Can't say that writing that was my proudest moment... I hope that we
can clarify how addNomination works before this feature is done. I
have however not been able to fully test this since I don't seem to
have proper permissions anywhere in the system.

Thanks,

Mattias

[James Westby (james-w)]

Hi,

Sorry for the delay in reviewing this.

8 +class AttributeError(Exception):

Could we avoid re-defining a standard exception, that's only going to lead to confusion.

If the standard exception doesn't do what you want please change the name of
this one.

59 + set_bugtask_milestone(bug.bug_tasks[0:2][-1], milestone)

Like Mattias I am uneasy about this, it looks very fragile. Is there
some rule you can verbalise about which task is needed at this point?
If so then we can try and code that up, rather than relying on ordering
of the tasks here.

Everything else looks good.

Thanks,

James

review: Needs Fixing

[lp:~mabac/svammel/bug-749561-set-series-and-milestone]

97. By Mattias Backman on 2011-04-20

Remove exception which collided with standard Exception and fix tests.

98. By Mattias Backman on 2011-04-20

Find correct bugtask for milestone instead of assuming that it is the last one.

[Mattias Backman (mabac)]

> Sorry for the delay in reviewing this.

No problem. I had left it in status "work in progress" until recently too.
>
> 8       +class AttributeError(Exception):
>
> Could we avoid re-defining a standard exception, that's only going to lead to confusion.

That's my mistake, didn't check for a standard exception with that
name. Removed.

> 59      +                set_bugtask_milestone(bug.bug_tasks[0:2][-1], milestone)
>
> Like Mattias I am uneasy about this, it looks very fragile. Is there
> some rule you can verbalise about which task is needed at this point?
> If so then we can try and code that up, rather than relying on ordering
> of the tasks here.

That rule should be to use the only bug_task if no series has been
nominated, and to use the bug_task that actually is targeted to the
specified series otherwise. Trivial now that I think about it, thanks.
:) I've changed this as well.

Thanks,

Mattias

[lp:~mabac/svammel/bug-749561-set-series-and-milestone]

99. By Mattias Backman on 2011-04-20

Automatically approve series nomination, to be able to proceed with setting milestone on bug_task.

100. By Mattias Backman on 2011-04-20

Only auto approve if user is allowed to do so.

[Mattias Backman (mabac)]

Another piece of the puzzle is that the user might be able to add a nomination but is not allowed to approve it. The new bugtask is only created when the nomination is approved.

Now the script will add a nomination and approve it immediately if the user has the permission to do so. If the nomination is added but cannot be approved, setting a milestone will fail, and I think that has to be since the user tried to do something she hasn't proper permissions for.

Just adding nominations in one pass would work, and setting milestones without nominating a series first is also ok. Only nominating a series and setting a milestone will require special permissions, I think.

[James Westby (james-w)]

53 + set_bugtask_milestone(ms_bugtask, milestone)

I would probably add a check here whether the bugtask was found, asking them to
report a bug if not, otherwise they will get a confusing error.

This looks much better now, thanks.

> Another piece of the puzzle is that the user might be able to add a nomination but is not allowed to
> approve it. The new bugtask is only created when the nomination is approved.

I think an error if they request an impossible situation is fine. Wouldn't the --continue-on-error
support deal with that anyway if they wanted?

As it stands though I don't think it will continue if the person can't do it, as the code
for finding the task now assumes that a series task will be created if --series is specified.

Perhaps it should not assume that and then decide what to do based on --continue-on-error?

Thanks,

James

review: Approve

[lp:~mabac/svammel/bug-749561-set-series-and-milestone]

101. By Mattias Backman on 2011-04-20

Move getting bug_task for milestone to immediately after nomination.

102. By Mattias Backman on 2011-04-20

Continue after failed addNomination if --ignore-errors passed.

[Mattias Backman (mabac)]

> 53 + set_bugtask_milestone(ms_bugtask, milestone)
>
> I would probably add a check here whether the bugtask was found, asking them
> to
> report a bug if not, otherwise they will get a confusing error.

Good idea, I have added that.

> > Another piece of the puzzle is that the user might be able to add a
> nomination but is not allowed to
> > approve it. The new bugtask is only created when the nomination is approved.
>
> I think an error if they request an impossible situation is fine. Wouldn't the
> --continue-on-error
> support deal with that anyway if they wanted?

Yes it would. I changed it a bit so now series nomination can fail in a couple of ways and still try to set milestone if --ignore-errors is passed.

>
> As it stands though I don't think it will continue if the person can't do it,
> as the code
> for finding the task now assumes that a series task will be created if
> --series is specified.
>
> Perhaps it should not assume that and then decide what to do based on
> --continue-on-error?

Now it doesn't assume that and will set milestone even if the user has no permissions for targeting series.

I'll merge this in the morning if no-one comments on the last revisions.

Thanks,

Mattias

[James Westby (james-w)]

This looks good to me, so merge whenever you like :-)

=== modified file 'bug_reporting.py'
--- bug_reporting.py	2011-04-15 06:59:55 +0000
+++ bug_reporting.py	2011-04-20 14:50:23 +0000
@@ -36,34 +36,114 @@
     return project.self_link
 
 
-def file_bug_by_url(target_url, description, title, tags, lp, importance=None):
+def file_bug_by_url(target_url, description, title, tags, lp, importance=None,
+                    milestone_name=None, series_name=None):
     """ File a bug on the bug target specified by target_url.
 
     Returns the newly created bug or None if no bug was filed.
     """
+    logger.debug("lp.bugs.createBug(description=%s..., title=%s, target=%s, " \
+                     "tags=%s)" % (description[:20], title, target_url, tags))
+
     bug = lp.bugs.createBug(description=description,
                             title=title,
                             target=target_url,
                             tags=tags)
     if bug is not None:
         logger.info('Filed bug %i: %s' % (bug.id, bug.web_link))
-        if importance is not None:
-            set_bugtask_importance(bug, importance)
+        try:
+            target = lp.distributions['ubuntu']
+            logger.debug("Series and milestone target is %s." % target)
+            ms_bugtask = None
+            if series_name is not None:
+                series = target.getSeries(name_or_version=series_name)
+                logger.debug("Adding nomination for series %s." % series)
+                try:
+                    nomination = bug.addNomination(target=series)
+                except:
+                    if continue_on_error():
+                        logger.warning("Cannot add nomination for series %s.",
+                                       series)
+                        nomination = None
+                    else:
+                        raise
+
+                if nomination is not None and nomination.canApprove():
+                    logger.debug("Approving nomination %s." % nomination)
+                    nomination.approve()
+                    for candidate_bugtask in bug.bug_tasks:
+                        if candidate_bugtask.target == series:
+                            ms_bugtask = candidate_bugtask
+                    if ms_bugtask is None:
+                        raise Exception(
+                            "A bug task to set milestone was not found, " \
+                            "please report this bug on the svammel project.")
+
+                elif continue_on_error():
+                    logger.warning("Cannot approve nomination %s.", nomination)
+                else:
+                    raise AttributeError(
+                        "Unable to approve nomination %s." % nomination)
+
+            if milestone_name is not None:
+                milestone = target.getMilestone(name=milestone_name)
+                logger.debug("Target milestone is %s." % milestone)
+                if milestone is None:
+                    raise AttributeError(
+                        "Milestone '%s' not valid for target '%s'." % \
+                            (milestone_name, target.name))
+
+                if ms_bugtask is None:
+                    # If series nomination is done the bug_task
+                    # has already been found.
+                    ms_bugtask = bug.bug_tasks[0]
+                set_bugtask_milestone(ms_bugtask, milestone)
+
+            if importance is not None:
+                set_bug_importance(bug, importance)
+        except Exception, e:
+            if continue_on_error():
+                logger.error("Error '%s' but will proceed.", e)
+            else:
+                logger.critical(
+                    "Closing bug before terminating, due to '%s'.", e)
+                for bug_task in bug.bug_tasks:
+                    close_bug(bug_task, BUG_INVALID_STATUS)
+                raise
     else:
         logger.error("Unable to file bug '%s'." % title)
 
     return bug
 
 
-def set_bugtask_importance(bug, importance):
+def set_bug_importance(bug, importance):
+    """Sets importance of all bug_tasks belonging to a bug."""
     try:
         for bug_task in bug.bug_tasks:
+            logger.debug("Setting importance %s for %s" % \
+                             (importance, bug_task.title))
             bug_task.importance = importance
             bug_task.lp_save()
     except:
         if continue_on_error():
             logger.error("Unable to set bug importance to '%s' but " \
-                "will proceed." % importance)
+                             "will proceed." % importance)
+        else:
+            raise
+
+
+def set_bugtask_milestone(bug_task, milestone):
+    """Sets the milestone for a bugtask."""
+    try:
+        logger.debug("Setting milestone %s for %s" % \
+                         (milestone.name, bug_task.title))
+
+        bug_task.milestone = milestone
+        bug_task.lp_save()
+    except:
+        if continue_on_error():
+            logger.error("Unable to set milestone '%s' but will " \
+                "proceed." % (milestone.name))
         else:
             raise
 
@@ -80,10 +160,16 @@
     return len(dupe_bugs[:1]) > 0
 
 
-def close_bug(url, close_status, lp):
+def close_bug_by_url(url, close_status, lp):
+    """ Close the bugtask by setting the status."""
+
+    bug_task = lp.load(url)
+    close_bug(bug_task, close_status)
+
+
+def close_bug(bug_task, close_status):
     """ Close the bugtask by setting status to Invalid."""
 
-    bug_task = lp.load(url)
     logger.info("Changing status for %s [%s ==> %s]" % \
                     (bug_task.title, bug_task.status, close_status))
     bug_task.status = close_status

=== modified file 'config.py'
--- config.py	2011-04-15 06:47:57 +0000
+++ config.py	2011-04-20 14:50:23 +0000
@@ -168,5 +168,11 @@
         '--ignore-errors', action='store_true', dest='ignoreerrors',
         help='Ignore errors that do not prevent proper completion, such as ' \
             'failure to set bug attributes.')
+    parser.add_argument(
+        '--milestone',
+        help='If specified, will be set on all created bugs.')
+    parser.add_argument(
+        '--target-series', dest='targetseries',
+        help='If specified, will be nominated for all created bugs.')
 
     return parser

=== modified file 'file-failures.py'
--- file-failures.py	2011-04-15 07:40:10 +0000
+++ file-failures.py	2011-04-20 14:50:23 +0000
@@ -31,7 +31,7 @@
 from bug_reporting import (
     file_bug_by_url,
     bug_already_filed_by_url,
-    close_bug,
+    close_bug_by_url,
     BUG_INVALID_STATUS,
     BUG_FIXED_STATUS,
     )
@@ -139,7 +139,10 @@
                   (spph.package_name, spph.version, bug_tags))
     if not args.dryrun:
         bug = file_bug_by_url(spph.url, bug_description, bug_title,
-                              bug_tags, get_launchpad(), args.importance)
+                              bug_tags, get_launchpad(),
+                              importance=args.importance,
+                              milestone_name=args.milestone,
+                              series_name=args.targetseries)
         if bug is not None and args.logfile is not None:
             bug_log.write_entry(
                 LogEntry(spph.package_name, spph.version, fail_platform,
@@ -161,7 +164,8 @@
                 new_status = BUG_INVALID_STATUS
             else:
                 new_status = BUG_FIXED_STATUS
-            close_bug(log_entry.bugtask_url, new_status, get_launchpad())
+            close_bug_by_url(log_entry.bugtask_url, new_status,
+                             get_launchpad())
         else:
             bug = get_launchpad().load(log_entry.bugtask_url)
             for bug_task in bug.bug_tasks:

=== modified file 'tests/test_fail_filer.py'
--- tests/test_fail_filer.py	2011-04-15 10:16:29 +0000
+++ tests/test_fail_filer.py	2011-04-20 14:50:23 +0000
@@ -37,9 +37,14 @@
     filter_spph,
     get_build_list,
     )
+import bug_reporting
 from bug_reporting import (
     bug_already_filed_by_url,
-    set_bugtask_importance,
+    set_bug_importance,
+    set_bugtask_milestone,
+    close_bug,
+    file_bug_by_url,
+    BUG_INVALID_STATUS,
     )
 from config import (
     get_service_root,
@@ -117,7 +122,6 @@
                 Launchpad, 'login_with', classmethod(mock_func)))
         init('staging', 'dummyapp', 'dummycache')
 
-
     def test_filter_only_checks_other_archives(self):
         self.init_dummy_config()
         archive_with_failure = self.MockArchive('failarchive')
@@ -129,7 +133,8 @@
                     ]
         spph = SPPH('packagetotest', '1')
         failrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
-                                     FAILED_BUILD_STATE, archive = archive_with_failure)
+                                     FAILED_BUILD_STATE,
+                                     archive=archive_with_failure)
         spph.add_build_record(failrecord)
         spph_list = dict()
         spph_list[spph.package_name] = spph
@@ -139,7 +144,6 @@
         self.assertTrue(other_archive_2.has_been_called)
         self.assertFalse(archive_with_failure.has_been_called)
 
-
     def test_filter_fail_record_not_filtered(self):
         self.init_dummy_config()
         spph = SPPH('packagefail', '1')
@@ -189,10 +193,12 @@
         packagename = 'packagesubstring'
         archive = self.MockArchive('archivename')
         exactrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
-                                      SUCCESS_BUILD_STATE, packagename, archive)
+                                      SUCCESS_BUILD_STATE, packagename,
+                                      archive)
         nonexactrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
                                          SUCCESS_BUILD_STATE,
-                                         'pre' + packagename + 'post', archive)
+                                         'pre' + packagename + 'post',
+                                         archive)
         archive.setRecordsToReturn([exactrecord, nonexactrecord])
         build_records = get_build_list(archive, SUCCESS_BUILD_STATE,
                                        packagename)
@@ -248,7 +254,8 @@
         fail_spph = SPPH(packagename, version)
         archive = self.MockArchive('archivename')
         failrecord = self.MockRecord(TEST_FAIL_ARCH, datetime.now(),
-                                     FAILED_BUILD_STATE, packagename, version, archive)
+                                     FAILED_BUILD_STATE, packagename, version,
+                                     archive)
         successrecord = self.MockRecord(TEST_OK_ARCHS[0], datetime.now(),
                                         SUCCESS_BUILD_STATE, packagename,
                                         version, archive)
@@ -526,19 +533,85 @@
         self.assertTrue(get_highest_version([None, None]) is None)
 
 
-class TestFileBug(TestCase):
+class TestFileBug(TestCaseWithFixtures):
+
+    class TestFailError(Exception):
+        def __init__(self, value):
+            self.value = value
+
+        def __str__(self):
+            return repr(self.value)
+
+    class MockLaunchpad(object):
+        class MockProject(object):
+            def searchTasks(self, tags, tags_combinator):
+                return self.bug_tasks
+
+            def __init__(self, bug_tasks):
+                self.bug_tasks = bug_tasks
+
+        class Bugs(object):
+            def createBug(self, description, title, target, tags):
+                bug = TestFileBug.MockBug()
+                bug_task1 = TestFileBug.MockBugTask(False)
+                bug_task1.status = 'New'
+                bug.add_bug_task(bug_task1)
+                bug_task2 = TestFileBug.MockBugTask(False)
+                bug_task2.status = 'New'
+                bug.add_bug_task(bug_task2)
+                return bug
+
+        def __init__(self, bug_tasks):
+            self.bugs = self.Bugs()
+            self.project = self.MockProject(bug_tasks)
+            self.distributions = dict()
+            self.distributions['ubuntu'] = TestFileBug.MockTarget()
+
+        def load(self, url):
+            return self.project
+
+    class MockMilestone:
+        def __init__(self, name):
+            self.name = name
 
     class MockBug:
         def __init__(self):
             self.bug_tasks = []
+            self.id = 1
+            self.web_link = 'http://dummy'
+            self.title = 'Test bug'
+
         def add_bug_task(self, bug_task):
             self.bug_tasks.append(bug_task)
 
+    class MockTarget:
+        class MockSeries:
+            def __init__(self):
+                self.name = 'Series'
+
+            def getMilestone(self, name):
+                return TestFileBug.MockMilestone(name)
+
+        def __init__(self):
+            self.name = 'Target'
+            self.current_series = self.MockSeries()
+
+        def getMilestone(self, name):
+            return TestFileBug.MockMilestone(name)
+
+        def getSeries(self, name):
+            return self.MockSeries()
+
     class MockBugTask:
+
         def __init__(self, will_fail_on_save):
             self.will_fail_on_save = will_fail_on_save
             self.importance = 'Undecided'
             self.has_saved = False
+            self.target = TestFileBug.MockTarget()
+            self.title = ''
+            self.status = ''
+
         def lp_save(self):
             if self.will_fail_on_save:
                 raise Exception()
@@ -549,47 +622,99 @@
         super(TestFileBug, self).setUp()
 
     def test_duplicate_exists(self):
-        class MockLaunchpad(object):
-
-            def load(obj, url):
-                class MockProject(object):
-                    def searchTasks(obj, tags, tags_combinator):
-                        return ['something here']
-                return MockProject()
-
+        lp = self.MockLaunchpad(['something here'])
         self.assertTrue(bug_already_filed_by_url('https://dummy', [],
-                                            MockLaunchpad()))
+                                                 lp))
 
     def test_duplicate_does_not_exist(self):
-        class MockLaunchpad(object):
-
-            def load(obj, url):
-                class MockProject(object):
-                    def searchTasks(obj, tags, tags_combinator):
-                        return []
-                return MockProject()
-
+        lp = self.MockLaunchpad([])
         self.assertFalse(bug_already_filed_by_url('https://dummy', [],
-                                            MockLaunchpad()))
+                                                  lp))
+
+    def test_close_bug_if_setting_importance_fails(self):
+
+        def raise_func(bug, importance):
+            self.saved_bug = bug
+            raise self.TestFailError('Setting importance failed.')
+
+        self.useFixture(MockSomethingFixture(
+                bug_reporting, 'set_bug_importance', raise_func))
+        try:
+            self.saved_bug = None
+            bug = file_bug_by_url('http://dummy', 'description', 'title', [],
+                                  self.MockLaunchpad([]), importance='Low')
+            self.fail('Error in test case, file_bug_by_url should have '\
+                          'raised an error.')
+        except self.TestFailError:
+            for bug_task in self.saved_bug.bug_tasks:
+                self.assertEquals(BUG_INVALID_STATUS, bug_task.status)
+
+    def test_close_bug_if_setting_milestone_fails(self):
+
+        def raise_func(bug_task, milestone):
+            self.saved_bug_task = bug_task
+            raise self.TestFailError('Setting milestone failed.')
+
+        self.useFixture(MockSomethingFixture(
+                bug_reporting, 'set_bugtask_milestone', raise_func))
+        try:
+            self.saved_bug_task = None
+            bug = file_bug_by_url('http://dummy', 'description', 'title', [],
+                                  self.MockLaunchpad([]),
+                                  milestone_name='1.0.0')
+            self.fail('Error in test case, file_bug_by_url should have '  \
+                          'raised an error.')
+        except self.TestFailError:
+            self.assertEquals(BUG_INVALID_STATUS, self.saved_bug_task.status)
+
+    def test_close_bug_saves(self):
+        bug_task = self.MockBugTask(False)
+        close_bug(bug_task, 'status')
+        self.assertEquals(True, bug_task.has_saved)
 
     def test_set_importance_saves(self):
         bug = self.MockBug()
         bug_task = self.MockBugTask(False)
         bug.add_bug_task(bug_task)
-        set_bugtask_importance(bug, 'High')
+        set_bug_importance(bug, 'High')
         self.assertEquals(True, bug_task.has_saved)
 
     def test_set_importance_can_ignore_errors(self):
         set_ignore_errors(True)
         bug = self.MockBug()
         bug.add_bug_task(self.MockBugTask(True))
-        set_bugtask_importance(bug, 'High')
+        set_bug_importance(bug, 'High')
 
     def test_set_importance_raises(self):
         set_ignore_errors(False)
         bug = self.MockBug()
         bug.add_bug_task(self.MockBugTask(True))
-        self.assertRaises(Exception, set_bugtask_importance, bug, 'High')
+        self.assertRaises(Exception, set_bug_importance, bug, 'High')
+
+    def test_set_milestone_saves(self):
+        bug_task = self.MockBugTask(False)
+        milestone = self.MockMilestone('0.5.0')
+        set_bugtask_milestone(bug_task, milestone)
+        self.assertEquals(milestone, bug_task.milestone)
+        self.assertTrue(bug_task.has_saved)
+
+    def test_set_milestone_can_ignore_errors(self):
+        set_ignore_errors(True)
+        bug_task = self.MockBugTask(True)
+        milestone = self.MockMilestone('0.5.0')
+        set_bugtask_milestone(bug_task, milestone)
+
+    def test_set_milestone_save_fail_raises(self):
+        set_ignore_errors(False)
+        bug_task = self.MockBugTask(True)
+        milestone = self.MockMilestone('0.5.0')
+        self.assertRaises(Exception, set_bugtask_milestone, bug_task,
+                          milestone)
+
+    def test_set_milestone_no_milestone_raises(self):
+        set_ignore_errors(False)
+        bug_task = self.MockBugTask(True)
+        self.assertRaises(Exception, set_bugtask_milestone, bug_task, None)
 
 
 class TestConfigInit(TestCaseWithFixtures):