launchpad-reviewers team mailing list archive

[Colin Watson <cjwatson@xxxxxxxxxxxxx>]

Colin Watson has proposed merging lp:~cjwatson/launchpad/kill-process-accepted-dry-run into lp:launchpad.

Commit message:
Remove unused and semi-broken process-accepted.py --dry-run option.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #54773 in Launchpad itself: "Dry run on process-accepted isn't always dry"
  https://bugs.launchpad.net/launchpad/+bug/54773

For more details, see:
https://code.launchpad.net/~cjwatson/launchpad/kill-process-accepted-dry-run/+merge/188113

process-accepted.py --dry-run hasn't been reliably dry for a long time (custom uploads).  As established in bug 54773, they aren't very useful anyway, and it's better not to offer something that claims to be a dry-run and in fact isn't.
-- 
https://code.launchpad.net/~cjwatson/launchpad/kill-process-accepted-dry-run/+merge/188113
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~cjwatson/launchpad/kill-process-accepted-dry-run into lp:launchpad.

=== modified file 'lib/lp/soyuz/scripts/processaccepted.py'
--- lib/lp/soyuz/scripts/processaccepted.py	2012-11-15 01:41:14 +0000
+++ lib/lp/soyuz/scripts/processaccepted.py	2013-09-27 17:34:49 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Helper functions for the process-accepted.py script."""
@@ -267,11 +267,6 @@
     def add_my_options(self):
         """Command line options for this script."""
         self.parser.add_option(
-            "-n", "--dry-run", action="store_true",
-            dest="dryrun", metavar="DRY_RUN", default=False,
-            help="Whether to treat this as a dry-run or not.")
-
-        self.parser.add_option(
             '-D', '--derived', action="store_true", dest="derived",
             default=False, help="Process all Ubuntu-derived distributions.")
 
@@ -283,13 +278,6 @@
             "--copy-archives", action="store_true", dest="copy_archives",
             default=False, help="Run only over COPY archives.")
 
-    def _commit(self):
-        """Commit transaction (unless in dry-run mode)."""
-        if self.options.dryrun:
-            self.logger.debug("Skipping commit: dry-run mode.")
-        else:
-            self.txn.commit()
-
     def findNamedDistro(self, distro_name):
         """Find the `Distribution` called `distro_name`."""
         self.logger.debug("Finding distribution %s.", distro_name)
@@ -358,7 +346,7 @@
     def processForDistro(self, distribution, target_policy):
         """Process all queue items for a distribution.
 
-        Commits between items, except in dry-run mode.
+        Commits between items.
 
         :param distribution: The `Distribution` to process queue items for.
         :param target_policy: The applicable `TargetPolicy`.
@@ -380,7 +368,7 @@
                     # Commit even on error; we may have altered the
                     # on-disk archive, so the partial state must
                     # make it to the DB.
-                    self._commit()
+                    self.txn.commit()
         return processed_queue_ids
 
     def main(self):
@@ -390,9 +378,9 @@
         try:
             for distro in self.findTargetDistros():
                 queue_ids = self.processForDistro(distro, target_policy)
-                self._commit()
+                self.txn.commit()
                 target_policy.postprocessSuccesses(queue_ids)
-                self._commit()
+                self.txn.commit()
 
         finally:
             self.logger.debug("Rolling back any remaining transactions.")

=== modified file 'lib/lp/soyuz/tests/test_processaccepted.py'
--- lib/lp/soyuz/tests/test_processaccepted.py	2013-06-20 05:50:00 +0000
+++ lib/lp/soyuz/tests/test_processaccepted.py	2013-09-27 17:34:49 +0000
@@ -1,4 +1,4 @@
-# Copyright 2010-2012 Canonical Ltd.  This software is licensed under the
+# Copyright 2010-2013 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 """Test process-accepted.py"""
@@ -169,7 +169,7 @@
         script.main()
         self.assertThat(len(uploads), LessThan(synch.commit_count))
 
-    def test_non_dry_run_commits_work(self):
+    def test_commits_work(self):
         upload = self.factory.makeSourcePackageUpload(
             distroseries=self.factory.makeDistroSeries(
                 distribution=self.distro))
@@ -179,15 +179,6 @@
         self.assertEqual(
             upload, IStore(PackageUpload).get(PackageUpload, upload_id))
 
-    def test_dry_run_aborts_work(self):
-        upload = self.factory.makeSourcePackageUpload(
-            distroseries=self.factory.makeDistroSeries(
-                distribution=self.distro))
-        upload_id = upload.id
-        self.getScript(['--dry-run']).main()
-        self.assertEqual(
-            None, IStore(PackageUpload).get(PackageUpload, upload_id))
-
     def test_validateArguments_requires_distro_by_default(self):
         self.assertRaises(
             OptionValueError, ProcessAccepted(test_args=[]).validateArguments)