launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/pabj-remove-feature-flag into lp:launchpad.

Commit message:
Remove soyuz.processacceptedbugsjob.enabled feature flag, now that ProcessAcceptedBugsJob has been deployed successfully.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #745799 in Launchpad itself: "DistroSeries:+queue Timeout accepting packages (bug structural subscriptions)"
  https://bugs.launchpad.net/launchpad/+bug/745799

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/pabj-remove-feature-flag/+merge/126090

The deployment of https://code.launchpad.net/~cjwatson/launchpad/process-accepted-bugs-job/+merge/122420 went well, and the cron script and feature flag are enabled on production now with no issues that I've seen, so we should be able to remove the feature flag and finally close the saga of bug 745799.

(I don't actually intend to land this until after the 12.10 beta-2 freeze has completed and given us some more experimental data, just to make sure, but I might as well parallelise the review process.)
-- 
https://code.launchpad.net/~cjwatson/launchpad/pabj-remove-feature-flag/+merge/126090
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/pabj-remove-feature-flag into lp:launchpad.

=== modified file 'lib/lp/services/features/flags.py'
--- lib/lp/services/features/flags.py	2012-09-19 11:47:37 +0000
+++ lib/lp/services/features/flags.py	2012-09-24 20:43:22 +0000
@@ -264,12 +264,6 @@
      'disabled',
      '',
      'https://dev.launchpad.net/LEP/PrivateProjects'),
-    ('soyuz.processacceptedbugsjob.enabled',
-     'boolean',
-     'If true, use a job to close bugs when accepting uploads from a queue.',
-     'disabled',
-     '',
-     ''),
 
     ])
 

=== modified file 'lib/lp/soyuz/model/processacceptedbugsjob.py'
--- lib/lp/soyuz/model/processacceptedbugsjob.py	2012-09-19 11:47:37 +0000
+++ lib/lp/soyuz/model/processacceptedbugsjob.py	2012-09-24 20:43:22 +0000
@@ -21,7 +21,6 @@
     classProvides,
     implements,
     )
-from zope.security.proxy import removeSecurityProxy
 
 from lp.app.interfaces.launchpad import ILaunchpadCelebrities
 from lp.bugs.interfaces.bug import IBugSet
@@ -55,15 +54,6 @@
         "\n\n---------------\n%s" % (spr.title, spr.changelog_entry))
 
     for bug in bugs:
-        # We need to remove the security proxy here because the bug might be
-        # private and if this code is called via someone using the +queue
-        # page they will get an OOPS.  BE CAREFUL with the unproxied bug
-        # object and look at what you're doing with it that might violate
-        # security.
-        # XXX cjwatson 2012-09-19: This can be removed once the
-        # soyuz.processacceptedbugsjob.enabled feature flag is switched on
-        # everywhere and the code to support disabling it is removed.
-        bug = removeSecurityProxy(bug)
         edited_task = bug.setStatus(
             target=target, status=BugTaskStatus.FIXRELEASED, user=janitor)
         if edited_task is not None:

=== modified file 'lib/lp/soyuz/scripts/processaccepted.py'
--- lib/lp/soyuz/scripts/processaccepted.py	2012-09-19 11:47:37 +0000
+++ lib/lp/soyuz/scripts/processaccepted.py	2012-09-24 20:43:22 +0000
@@ -23,7 +23,6 @@
 from lp.archiveuploader.tagfiles import parse_tagfile_content
 from lp.registry.interfaces.distribution import IDistributionSet
 from lp.registry.interfaces.pocket import PackagePublishingPocket
-from lp.services.features import getFeatureFlag
 from lp.services.scripts.base import (
     LaunchpadCronScript,
     LaunchpadScriptFailure,
@@ -180,10 +179,8 @@
     if not bug_ids_to_close:
         return
 
-    if (getSecurityPolicy() == LaunchpadPermissiveSecurityPolicy or
-        not getFeatureFlag("soyuz.processacceptedbugsjob.enabled")):
-        # We're already running in a script (or the feature flag to allow
-        # use of the job is disabled), so we can just close the bugs
+    if getSecurityPolicy() == LaunchpadPermissiveSecurityPolicy:
+        # We're already running in a script, so we can just close the bugs
         # directly.
         close_bug_ids_for_sourcepackagerelease(
             distroseries, source_release, bug_ids_to_close)

=== modified file 'lib/lp/soyuz/scripts/tests/test_queue.py'
--- lib/lp/soyuz/scripts/tests/test_queue.py	2012-09-20 15:36:33 +0000
+++ lib/lp/soyuz/scripts/tests/test_queue.py	2012-09-24 20:43:22 +0000
@@ -35,7 +35,6 @@
 from lp.registry.interfaces.series import SeriesStatus
 from lp.services.config import config
 from lp.services.database.lpstorm import IStore
-from lp.services.features.testing import FeatureFixture
 from lp.services.librarian.interfaces import ILibraryFileAliasSet
 from lp.services.librarian.model import LibraryFileAlias
 from lp.services.librarian.utils import filechunks
@@ -1080,11 +1079,6 @@
 
     layer = DatabaseFunctionalLayer
 
-    def assertBugChanges(self, series, spr, bug):
-        with celebrity_logged_in("admin"):
-            self.assertEqual(
-                BugTaskStatus.FIXRELEASED, bug.default_bugtask.status)
-
     def test_close_bugs_for_sourcepackagerelease_with_private_bug(self):
         # lp.soyuz.scripts.processaccepted.close_bugs_for_sourcepackagerelease
         # should work with private bugs where the person using the queue
@@ -1106,22 +1100,8 @@
             # But the bug closure should work.
             close_bugs_for_sourcepackagerelease(series, spr, changes)
 
-        # Verify it was closed.
-        self.assertBugChanges(series, spr, bug)
-
-
-class TestQueuePageClosingBugsJob(TestQueuePageClosingBugs):
-    # Repeat TestQueuePageClosingBugs, but with the feature flag set to
-    # cause close_bugs_for_sourcepackagerelease to create a job rather than
-    # closing bugs immediately.
-
-    def setUp(self):
-        super(TestQueuePageClosingBugsJob, self).setUp()
-        self.useFixture(FeatureFixture(
-            {"soyuz.processacceptedbugsjob.enabled": "on"},
-            ))
-
-    def assertBugChanges(self, series, spr, bug):
+        # Rather than closing the bugs immediately, this creates a
+        # ProcessAcceptedBugsJob.
         with celebrity_logged_in("admin"):
             self.assertEqual(BugTaskStatus.NEW, bug.default_bugtask.status)
         job_source = getUtility(IProcessAcceptedBugsJobSource)