Merge lp:~bjornt/landscape-client/remove-smart-methods into lp:~landscape/landscape-client/trunk

[Björn Tillenius]

Remove methods that were smart-specific. I removed AptFacade.deinit() as
well as all methods dealing with package locks, including the ones in
the package store. Basically only removals, although I did resurrect an
assert in test_resynchronize, which required some changes in that test
to make it work (it was using a not-installed package for the one being
locked, which isn't possible with apt)

[Mike Milner (milner)]

Looks good, +1

review: Approve

[Alberto Donato (ack)]

Great, +1!

#1:
+ for table in ["locked"]:

no need for the loop anymore

review: Approve

[lp:~bjornt/landscape-client/remove-smart-methods]

572. By Björn Tillenius on 2012-05-16

Remove for-loop.

=== modified file 'landscape/package/changer.py'
--- landscape/package/changer.py	2012-05-08 01:09:17 +0000
+++ landscape/package/changer.py	2012-05-16 08:19:20 +0000
@@ -18,7 +18,7 @@
 from landscape.package.taskhandler import (
     PackageTaskHandler, PackageTaskHandlerConfiguration, PackageTaskError,
     run_task_handler)
-from landscape.manager.manager import FAILED, SUCCEEDED
+from landscape.manager.manager import FAILED
 
 
 class UnknownPackageData(Exception):
@@ -80,10 +80,6 @@
             # Nothing was done
             return
 
-        # In order to let the reporter run smart-update cleanly,
-        # we have to deinitialize Smart, so that the write lock
-        # gets released
-        self._facade.deinit()
         if os.getuid() == 0:
             os.setgid(grp.getgrnam("landscape").gr_gid)
             os.setuid(pwd.getpwnam("landscape").pw_uid)
@@ -294,32 +290,15 @@
     def handle_change_package_locks(self, message):
         """Handle a C{change-package-locks} message.
 
-        Create and delete package locks as requested by the given C{message}.
+        Package locks aren't supported anymore.
         """
 
-        if not self._facade.supports_package_locks:
-            response = {
-                "type": "operation-result",
-                "operation-id": message.get("operation-id"),
-                "status": FAILED,
-                "result-text": "This client doesn't support package locks.",
-                "result-code": 1}
-            return self._broker.send_message(response, True)
-
-        for lock in message.get("create", ()):
-            self._facade.set_package_lock(*lock)
-        for lock in message.get("delete", ()):
-            self._facade.remove_package_lock(*lock)
-        self._facade.save_config()
-
-        response = {"type": "operation-result",
-                    "operation-id": message.get("operation-id"),
-                    "status": SUCCEEDED,
-                    "result-text": "Package locks successfully changed.",
-                    "result-code": 0}
-
-        logging.info("Queuing message with change package locks results to "
-                     "exchange urgently.")
+        response = {
+            "type": "operation-result",
+            "operation-id": message.get("operation-id"),
+            "status": FAILED,
+            "result-text": "This client doesn't support package locks.",
+            "result-code": 1}
         return self._broker.send_message(response, True)
 
     @staticmethod

=== modified file 'landscape/package/facade.py'
--- landscape/package/facade.py	2012-05-08 23:21:07 +0000
+++ landscape/package/facade.py	2012-05-16 08:19:20 +0000
@@ -89,8 +89,6 @@
         database.
     """
 
-    supports_package_holds = True
-    supports_package_locks = False
     _dpkg_status = "/var/lib/dpkg/status"
 
     def __init__(self, root=None):
@@ -133,9 +131,6 @@
             os.makedirs(full_path)
         return full_path
 
-    def deinit(self):
-        """This method exists solely to be compatible with C{SmartFacade}."""
-
     def get_packages(self):
         """Get all the packages available in the channels."""
         return self._hash2pkg.itervalues()
@@ -150,17 +145,6 @@
             if (self.is_package_installed(version)
                 and self._is_package_held(version.package))]
 
-    def get_package_locks(self):
-        """Return all set package locks.
-
-        @return: A C{list} of ternary tuples, contaning the name, relation
-            and version details for each lock currently set on the system.
-
-        XXX: This method isn't implemented yet. It's here to make the
-        transition to Apt in the package reporter easier.
-        """
-        return []
-
     def get_package_holds(self):
         """Return the name of all the packages that are on hold."""
         return [version.package.name for version in self.get_locked_packages()]

=== modified file 'landscape/package/reporter.py'
--- landscape/package/reporter.py	2012-05-08 01:24:21 +0000
+++ landscape/package/reporter.py	2012-05-16 08:19:20 +0000
@@ -278,7 +278,6 @@
         self._store.clear_available_upgrades()
         self._store.clear_installed()
         self._store.clear_locked()
-        self._store.clear_package_locks()
 
         # Don't clear the hash_id_requests table because the messages
         # associated with the existing requests might still have to be
@@ -421,16 +420,13 @@
         reactor.
         """
 
-        def changes_detected(results):
-            # Release all smart locks, in case the changer runs after us.
-            self._facade.deinit()
-            if True in results:
+        def changes_detected(result):
+            if result:
                 # Something has changed, notify the broker.
                 return self._broker.fire_event("package-data-changed")
 
-        result = gather_results([self.detect_packages_changes(),
-                                 self.detect_package_locks_changes()])
-        return result.addCallback(changes_detected)
+        deferred = self.detect_packages_changes()
+        return deferred.addCallback(changes_detected)
 
     def detect_packages_changes(self):
         """Detect changes in the universe of known packages.
@@ -564,53 +560,6 @@
 
         return result
 
-    def detect_package_locks_changes(self):
-        """Detect changes in known package locks.
-
-        This method will verify if there are package locks that:
-
-        - are now set, and were not;
-        - were previously set but are not anymore;
-
-        In all cases, the server is notified of the new situation
-        with a "packages" message.
-
-        @return: A deferred resulting in C{True} if package lock changes were
-            detected with respect to the previous run, or C{False} otherwise.
-        """
-        old_package_locks = set(self._store.get_package_locks())
-        current_package_locks = set(self._facade.get_package_locks())
-
-        set_package_locks = current_package_locks - old_package_locks
-        unset_package_locks = old_package_locks - current_package_locks
-
-        message = {}
-        if set_package_locks:
-            message["created"] = sorted(set_package_locks)
-        if unset_package_locks:
-            message["deleted"] = sorted(unset_package_locks)
-
-        if not message:
-            return succeed(False)
-
-        message["type"] = "package-locks"
-        result = self.send_message(message)
-
-        logging.info("Queuing message with changes in known package locks:"
-                     " %d created, %d deleted." %
-                     (len(set_package_locks), len(unset_package_locks)))
-
-        def update_currently_known(result):
-            if set_package_locks:
-                self._store.add_package_locks(set_package_locks)
-            if unset_package_locks:
-                self._store.remove_package_locks(unset_package_locks)
-            return True
-
-        result.addCallback(update_currently_known)
-
-        return result
-
 
 class FakeGlobalReporter(PackageReporter):
     """

=== modified file 'landscape/package/store.py'
--- landscape/package/store.py	2011-08-19 14:07:05 +0000
+++ landscape/package/store.py	2012-05-16 08:19:20 +0000
@@ -249,40 +249,6 @@
         cursor.execute("DELETE FROM locked")
 
     @with_cursor
-    def get_package_locks(self, cursor):
-        """Get all package locks."""
-        cursor.execute("SELECT name, relation, version FROM package_locks")
-        return [(row[0], row[1], row[2]) for row in cursor.fetchall()]
-
-    @with_cursor
-    def add_package_locks(self, cursor, locks):
-        """Add a list of package locks to the store.
-
-        @param locks: A C{list} of ternary tuples each one contains the
-            name, the relation and the version of the package lock to be added.
-        """
-        for name, relation, version in locks:
-            cursor.execute("REPLACE INTO package_locks VALUES (?, ?, ?)",
-                           (name, relation or "", version or "",))
-
-    @with_cursor
-    def remove_package_locks(self, cursor, locks):
-        """Remove a list of package locks from the store.
-
-        @param locks: A C{list} of ternary tuples each one contains the name,
-            the relation and the version of the package lock to be removed.
-        """
-        for name, relation, version in locks:
-            cursor.execute("DELETE FROM package_locks WHERE name=? AND "
-                           "relation=? AND version=?",
-                           (name, relation or "", version or ""))
-
-    @with_cursor
-    def clear_package_locks(self, cursor):
-        """Remove all package locks."""
-        cursor.execute("DELETE FROM package_locks")
-
-    @with_cursor
     def add_hash_id_request(self, cursor, hashes):
         hashes = list(hashes)
         cursor.execute("INSERT INTO hash_id_request (hashes, timestamp)"
@@ -458,9 +424,6 @@
     #       try block.
     cursor = db.cursor()
     try:
-        cursor.execute("CREATE TABLE package_locks"
-                       " (name TEXT NOT NULL, relation TEXT, version TEXT,"
-                       " UNIQUE(name, relation, version))")
         cursor.execute("CREATE TABLE locked"
                        " (id INTEGER PRIMARY KEY)")
         cursor.execute("CREATE TABLE available"

=== modified file 'landscape/package/tests/test_changer.py'
--- landscape/package/tests/test_changer.py	2012-05-08 23:21:07 +0000
+++ landscape/package/tests/test_changer.py	2012-05-16 08:19:20 +0000
@@ -632,8 +632,7 @@
         """
         After the package changer has run, we want the package-reporter to run
         to report the recent changes.  If we're running as root, we want to
-        change to the "landscape" user and "landscape" group. We also want to
-        deinitialize Smart to let the reporter run smart-update cleanly.
+        change to the "landscape" user and "landscape" group.
         """
 
         # We are running as root
@@ -644,10 +643,6 @@
         # The order matters (first smart then gid and finally uid)
         self.mocker.order()
 
-        # Deinitialize smart
-        facade_mock = self.mocker.patch(self.facade)
-        facade_mock.deinit()
-
         # We want to return a known gid
         grnam_mock = self.mocker.replace("grp.getgrnam")
         grnam_mock("landscape")
@@ -1328,9 +1323,9 @@
 
     def test_change_package_locks(self):
         """
-        If C{AptFacade} is used, the L{PackageChanger.handle_tasks}
-        method fails the activity, since it can't add or remove locks because
-        apt doesn't support this.
+        The L{PackageChanger.handle_tasks} method fails
+        change-package-locks activities, since it can't add or remove
+        locks because apt doesn't support this.
         """
         self.store.add_task("changer", {"type": "change-package-locks",
                                         "create": [("foo", ">=", "1.0")],

=== modified file 'landscape/package/tests/test_reporter.py'
--- landscape/package/tests/test_reporter.py	2012-05-08 23:21:07 +0000
+++ landscape/package/tests/test_reporter.py	2012-05-16 08:19:20 +0000
@@ -892,20 +892,13 @@
         result = self.reporter.detect_packages_changes()
         return result.addCallback(got_result)
 
-    def test_detect_changes_considers_packages_and_locks_changes(self):
+    def test_detect_changes_considers_packages_changes(self):
         """
-        The L{PackageReporter.detect_changes} method considers both package and
-        package locks changes. It also releases smart locks by calling the
-        L{SmartFacade.deinit} method.
+        The L{PackageReporter.detect_changes} method package changes.
         """
         reporter_mock = self.mocker.patch(self.reporter)
         reporter_mock.detect_packages_changes()
         self.mocker.result(succeed(True))
-        reporter_mock.detect_package_locks_changes()
-        self.mocker.result(succeed(True))
-
-        facade_mock = self.mocker.patch(self.facade)
-        facade_mock.deinit()
 
         self.mocker.replay()
         return self.reporter.detect_changes()
@@ -918,8 +911,6 @@
         """
         reporter_mock = self.mocker.patch(self.reporter)
         reporter_mock.detect_packages_changes()
-        self.mocker.result(succeed(False))
-        reporter_mock.detect_package_locks_changes()
         self.mocker.result(succeed(True))
         callback = self.mocker.mock()
         callback()
@@ -1001,16 +992,19 @@
         This is done in the reporter so that we know it happens when
         no other reporter is possibly running at the same time.
         """
+        self._add_system_package("foo")
+        self.facade.reload_channels()
+        [foo] = self.facade.get_packages_by_name("foo")
+        foo_hash = self.facade.get_package_hash(foo)
+        self.facade.set_package_hold(foo)
+        self.facade.reload_channels()
         message_store = self.broker_service.message_store
         message_store.set_accepted_types(["package-locks"])
-        self.store.set_hash_ids({HASH1: 3, HASH2: 4})
+        self.store.set_hash_ids({foo_hash: 3, HASH2: 4})
         self.store.add_available([1])
         self.store.add_available_upgrades([2])
         self.store.add_installed([2])
         self.store.add_locked([3])
-        self.store.add_package_locks([("name1", None, None)])
-        if self.facade.supports_package_locks:
-            self.facade.set_package_lock("name1")
         request1 = self.store.add_hash_id_request(["hash3"])
         request2 = self.store.add_hash_id_request(["hash4"])
 
@@ -1023,12 +1017,6 @@
         self.assertEqual(self.store.get_available_upgrades(), [2])
         self.assertEqual(self.store.get_available(), [1])
         self.assertEqual(self.store.get_installed(), [2])
-        # XXX: Don't check get_locked() and get_package_locks() until
-        # package locks are implemented in AptFacade.
-        if not isinstance(self.facade, AptFacade):
-            self.assertEqual(self.store.get_locked(), [3])
-            self.assertEqual(
-                self.store.get_package_locks(), [("name1", "", "")])
         self.assertEqual(self.store.get_hash_id_request(request1.id).id,
                          request1.id)
 
@@ -1039,7 +1027,7 @@
         def check_result(result):
 
             # The hashes should not go away.
-            hash1 = self.store.get_hash_id(HASH1)
+            hash1 = self.store.get_hash_id(foo_hash)
             hash2 = self.store.get_hash_id(HASH2)
             self.assertEqual([hash1, hash2], [3, 4])
 
@@ -1048,12 +1036,9 @@
 
             # After running the resychronize task, detect_packages_changes is
             # called, and the existing known hashes are made available.
-            self.assertEqual(self.store.get_available(), [3, 4])
-            self.assertEqual(self.store.get_installed(), [])
-            # XXX: Don't check get_locked() until package locks are
-            # implemented in AptFacade.
-            if not isinstance(self.facade, AptFacade):
-                self.assertEqual(self.store.get_locked(), [3])
+            self.assertEqual(self.store.get_available(), [4])
+            self.assertEqual(self.store.get_installed(), [3])
+            self.assertEqual(self.store.get_locked(), [3])
 
             # The two original hash id requests should be still there, and
             # a new hash id request should also be detected for HASH3.
@@ -1066,18 +1051,11 @@
                 elif request.id == request2.id:
                     self.assertEqual(request.hashes, ["hash4"])
                 elif not new_request_found:
-                    self.assertEqual(request.hashes, [HASH3])
+                    self.assertEqual(request.hashes, [HASH3, HASH1])
                 else:
                     self.fail("Unexpected hash-id request!")
             self.assertEqual(requests_count, 3)
 
-            # XXX: Don't check for package-locks messages until package
-            # locks are implemented in AptFacade.
-            if not isinstance(self.facade, AptFacade):
-                self.assertMessages(message_store.get_pending_messages(),
-                                    [{"type": "package-locks",
-                                      "created": [("name1", "", "")]}])
-
         deferred.addCallback(check_result)
         return deferred
 

=== modified file 'landscape/package/tests/test_store.py'
--- landscape/package/tests/test_store.py	2011-07-06 21:11:43 +0000
+++ landscape/package/tests/test_store.py	2012-05-16 08:19:20 +0000
@@ -1,6 +1,5 @@
 import threading
 import time
-import sys
 
 import sqlite3
 from landscape.tests.helpers import LandscapeTest
@@ -356,11 +355,9 @@
 
         database = sqlite3.connect(filename)
         cursor = database.cursor()
-        for table in ["package_locks", "locked"]:
-            query = "pragma table_info(%s)" % table
-            cursor.execute(query)
-            result = cursor.fetchall()
-            self.assertTrue(len(result) > 0)
+        cursor.execute("pragma table_info(locked)")
+        result = cursor.fetchall()
+        self.assertTrue(len(result) > 0)
 
     def test_add_and_get_locked(self):
         """
@@ -401,103 +398,6 @@
         self.store1.clear_locked()
         self.assertEqual(self.store2.get_locked(), [])
 
-    def test_get_package_locks_with_no_lock(self):
-        """
-        L{PackageStore.get_package_locks} returns an empty list if no package
-        locks are stored.
-        """
-        self.assertEqual(self.store1.get_package_locks(), [])
-
-    def test_add_package_locks(self):
-        """
-        L{PackageStore.add_package_locks} adds a package lock to the store.
-        """
-        self.store1.add_package_locks([("name", "", "")])
-        self.assertEqual(self.store2.get_package_locks(),
-                         [("name", "", "")])
-
-    def test_add_package_locks_idempotence(self):
-        """
-        The operation of adding a lock is idempotent.
-        """
-        self.store1.add_package_locks([("name", "", "")])
-        self.store1.add_package_locks([("name", "", "")])
-        self.assertEqual(self.store2.get_package_locks(),
-                         [("name", "", "")])
-
-    def test_add_package_locks_with_none(self):
-        """
-        If None, package locks relation and version values are automatically
-        converted to empty strings.
-        """
-        self.store1.add_package_locks([("name", None, None)])
-        self.assertEqual(self.store2.get_package_locks(),
-                         [("name", "", "")])
-
-    def test_add_package_locks_multiple_times(self):
-        """
-        L{PackageStore.add_package_locks} can be called multiple times and
-        with multiple locks each time.
-        """
-        self.store1.add_package_locks([("name1", "", "")])
-        self.store1.add_package_locks([("name2", "<", "0.2"),
-                                       ("name3", "", "")])
-        self.assertEqual(sorted(self.store2.get_package_locks()),
-                         sorted([("name1", "", ""),
-                                 ("name2", "<", "0.2"),
-                                 ("name3", "", "")]))
-
-    def test_add_package_locks_without_name(self):
-        """
-        It's not possible to add a package lock without a name.
-        """
-        if sys.version_info >= (2, 5):
-            sqlite_error = sqlite3.IntegrityError
-        else:
-            sqlite_error = sqlite3.OperationalError
-        self.assertRaises(sqlite_error,
-                          self.store1.add_package_locks,
-                          [(None, None, None)])
-
-    def test_remove_package_locks(self):
-        """
-        L{PackageStore.remove_package_locks} removes a package lock from
-        the store.
-        """
-        self.store1.add_package_locks([("name1", "", "")])
-        self.store1.remove_package_locks([("name1", "", "")])
-        self.assertEqual(self.store2.get_package_locks(), [])
-
-    def test_remove_package_locks_multiple_times(self):
-        """
-        L{PackageStore.remove_package_locks} can be called multiple times and
-        with multiple locks each time.
-        """
-        self.store1.add_package_locks([("name1", "", ""),
-                                       ("name2", "<", "0.2"),
-                                       ("name3", "", "")])
-        self.store1.remove_package_locks([("name1", "", "")])
-        self.store1.remove_package_locks([("name2", "<", "0.2"),
-                                          ("name3", "", "")])
-        self.assertEqual(self.store2.get_package_locks(), [])
-
-    def test_remove_package_locks_without_matching_lock(self):
-        """
-        It's fine to remove a non-existent lock.
-        """
-        self.store1.remove_package_locks([("name", "", "")])
-        self.assertEqual(self.store2.get_package_locks(), [])
-
-    def test_clear_package_locks(self):
-        """
-        L{PackageStore.clear_package_locks} removes all package locks
-        from the store.
-        """
-        self.store1.add_package_locks([("name1", "", ""),
-                                       ("name2", "<", "0.2")])
-        self.store1.clear_package_locks()
-        self.assertEqual(self.store2.get_package_locks(), [])
-
     def test_add_hash_id_request(self):
         hashes = ("ha\x00sh1", "ha\x00sh2")
         request1 = self.store1.add_hash_id_request(hashes)