Merge lp:~free.ekanayaka/landscape-client/auto-approve into lp:~landscape/landscape-client/trunk

[Free Ekanayaka]

[Kapil Thangavelu (hazmat)]

Nice branch, looks good +1

Some comments,

1) Spelling typo

204 + def test_perform_changes_with_allow_install_policy_and_removals(self):
205 + """
206 + The C{POLICY_ALLOW_INSTALLS} policy doens't allow additional packages

2) Pep8 (unrelated to branch but in a modified file), test_changer.py, line 721, is trailing whitespace.

3) This might be stylistic, but i find side effects in conditionals to be a bit hard to parse, although the test coverage is fine.

review: Approve

[Free Ekanayaka (free.ekanayaka)]

Thanks Kapil, all points were addressed.

[lp:~free.ekanayaka/landscape-client/auto-approve]

211. By Free Ekanayaka on 2010-02-08

Remove conditional with side-effects (Kapil [3])

212. By Free Ekanayaka on 2010-02-08

Fix typo and pep8 (Kapil [1], [2])

[Thomas Herve (therve)]

[1]
+ @param reset: iF C{True} all existing marks will be reset.

Typo: if

[2] For the sake of completeness, I would a POLICY_ALLOW_ALL_CHANGES, even if we don't expose it the new UI.

+1!

review: Approve

[Free Ekanayaka (free.ekanayaka)]

Thanks Thomas! [1] and [2] fixed.

[lp:~free.ekanayaka/landscape-client/auto-approve]

213. By Free Ekanayaka on 2010-02-09

Fix typo and add POLICY_ALLOW_ALL_CHANGES for allowing both installs and removals (Thomas [1], [2])

=== modified file 'landscape/__init__.py'
--- landscape/__init__.py	2010-02-01 09:09:54 +0000
+++ landscape/__init__.py	2010-02-08 21:50:24 +0000
@@ -21,6 +21,7 @@
 #
 # 3.3:
 #  * Add "binaries" field to "change-packages"
+#  * Add "policy" field to "change-packages"
 #  * Add new "change-package-locks" client accepted message type.
 #
 CLIENT_API = "3.3"

=== modified file 'landscape/package/changer.py'
--- landscape/package/changer.py	2010-02-08 08:22:09 +0000
+++ landscape/package/changer.py	2010-02-08 21:50:24 +0000
@@ -19,7 +19,8 @@
 SUCCESS_RESULT = 1
 ERROR_RESULT = 100
 DEPENDENCY_ERROR_RESULT = 101
-
+POLICY_STRICT = 0
+POLICY_ALLOW_INSTALLS = 1
 
 # The amount of time to wait while we have unknown package data before
 # reporting an error to the server in response to an operation.
@@ -139,7 +140,7 @@
         else:
             raise PackageTaskError()
 
-    def init_channels(self, binaries):
+    def init_channels(self, binaries=()):
         """Initialize the Smart channels as needed.
 
         @param binaries: A possibly empty list of 3-tuples of the form
@@ -163,14 +164,16 @@
 
         self._facade.ensure_channels_reloaded()
 
-    def mark_packages(self, upgrade=False, install=(), remove=()):
+    def mark_packages(self, upgrade=False, install=(), remove=(), reset=True):
         """Mark packages for upgrade, installation or removal.
 
         @param upgrade: If C{True} mark all installed packages for upgrade.
         @param install: A list of package ids to be marked for installation.
         @param remove: A list of package ids to be marked for removal.
+        @param reset: iF C{True} all existing marks will be reset.
         """
-        self._facade.reset_marks()
+        if reset:
+            self._facade.reset_marks()
 
         if upgrade:
             for package in self._facade.get_packages():
@@ -188,9 +191,12 @@
                     raise UnknownPackageData(hash)
                 mark_func(package)
 
-    def change_packages(self):
+    def change_packages(self, policy):
         """Perform the requested changes.
 
+        @param policy: A value indicating what to do in case additional changes
+            beside the ones explicitly requested are needed in order to fulfill
+            the request (see L{complement_changes}).
         @return: A L{ChangePackagesResult} holding the details about the
             outcome of the requested changes.
         """
@@ -200,31 +206,56 @@
             DependencyError, TransactionError, SmartError)
 
         result = ChangePackagesResult()
-        try:
-            result.text = self._facade.perform_changes()
-        except (TransactionError, SmartError), exception:
-            result.code = ERROR_RESULT
-            result.text = exception.args[0]
-        except DependencyError, exception:
-            result.code = DEPENDENCY_ERROR_RESULT
-            for package in exception.packages:
-                hash = self._facade.get_package_hash(package)
-                id = self._store.get_hash_id(hash)
-                if id is None:
-                    # Will have to wait until the server lets us know about
-                    # this id.
-                    raise UnknownPackageData(hash)
-                if package.installed:
-                    # Package currently installed. Must remove it.
-                    result.removals.append(id)
+        count = 0
+        while result.code is None:
+            count += 1
+            try:
+                result.text = self._facade.perform_changes()
+            except (TransactionError, SmartError), exception:
+                result.code = ERROR_RESULT
+                result.text = exception.args[0]
+            except DependencyError, exception:
+                for package in exception.packages:
+                    hash = self._facade.get_package_hash(package)
+                    id = self._store.get_hash_id(hash)
+                    if id is None:
+                        # Will have to wait until the server lets us know about
+                        # this id.
+                        raise UnknownPackageData(hash)
+                    if package.installed:
+                        # Package currently installed. Must remove it.
+                        result.removals.append(id)
+                    else:
+                        # Package currently available. Must install it.
+                        result.installs.append(id)
+                if count == 1 and self.may_complement_changes(result, policy):
+                    # Mark all missing packages and try one more iteration
+                    self.mark_packages(install=result.installs,
+                                       remove=result.removals, reset=False)
                 else:
-                    # Package currently available. Must install it.
-                    result.installs.append(id)
-        else:
-            result.code = SUCCESS_RESULT
+                    result.code = DEPENDENCY_ERROR_RESULT
+            else:
+                result.code = SUCCESS_RESULT
 
         return result
 
+    def may_complement_changes(self, result, policy):
+        """Decide whether or not we should complement the given changes.
+
+        @param result: A L{PackagesResultObject} holding the details about the
+            missing dependencies needed to complement the given changes.
+        @param policy: It can be one of the following values:
+            - L{POLICY_STRICT}, no additional packages will be marked.
+            - L{POLICY_ALLOW_INSTALLS}, if only additional installs are missing
+                they will be marked for installation.
+        @return: A boolean indicating whether the given policy allows to
+            complement the changes and retry.
+        """
+        if policy == POLICY_ALLOW_INSTALLS:
+            if result.installs and not result.removals:
+                return True
+        return False
+
     def handle_change_packages(self, message):
         """Handle a C{change-packages} message."""
 
@@ -233,7 +264,7 @@
                            message.get("install", ()),
                            message.get("remove", ()))
 
-        result = self.change_packages()
+        result = self.change_packages(message.get("policy", POLICY_STRICT))
 
         response = {"type": "change-packages-result",
                    "operation-id": message.get("operation-id")}

=== modified file 'landscape/package/tests/test_changer.py'
--- landscape/package/tests/test_changer.py	2010-02-05 19:05:47 +0000
+++ landscape/package/tests/test_changer.py	2010-02-08 21:50:24 +0000
@@ -9,11 +9,13 @@
 from smart.cache import Provides
 
 from landscape.package.changer import (
-    PackageChanger, main, find_changer_command, UNKNOWN_PACKAGE_DATA_TIMEOUT)
+    PackageChanger, main, find_changer_command, UNKNOWN_PACKAGE_DATA_TIMEOUT,
+    SUCCESS_RESULT, DEPENDENCY_ERROR_RESULT, POLICY_ALLOW_INSTALLS)
 from landscape.package.store import PackageStore
 from landscape.package.facade import (
     DependencyError, TransactionError, SmartError)
-from landscape.package.changer import PackageChangerConfiguration
+from landscape.package.changer import (
+    PackageChangerConfiguration, ChangePackagesResult)
 
 from landscape.tests.mocker import ANY
 from landscape.tests.helpers import (
@@ -306,6 +308,101 @@
                                   "type": "change-packages-result"}])
         return result.addCallback(got_result)
 
+    def test_perform_changes_with_allow_install_policy(self):
+        """
+        The C{POLICY_ALLOW_INSTALLS} policy the makes the changer mark
+        the missing packages for installation.
+        """
+        self.store.set_hash_ids({HASH1: 1})
+        self.facade.reload_channels()
+        package1 = self.facade.get_packages_by_name("name1")[0]
+
+        self.mocker.order()
+        self.facade.perform_changes = self.mocker.mock()
+        self.facade.perform_changes()
+        self.mocker.throw(DependencyError([package1]))
+
+        self.facade.mark_install = self.mocker.mock()
+        self.facade.mark_install(package1)
+        self.facade.perform_changes()
+        self.mocker.result("success")
+        self.mocker.replay()
+
+        result = self.changer.change_packages(POLICY_ALLOW_INSTALLS)
+
+        self.assertEquals(result.code, SUCCESS_RESULT)
+        self.assertEquals(result.text, "success")
+        self.assertEquals(result.installs, [1])
+        self.assertEquals(result.removals, [])
+
+    def test_perform_changes_with_allow_install_policy_and_removals(self):
+        """
+        The C{POLICY_ALLOW_INSTALLS} policy doesn't allow additional packages
+        to be removed.
+        """
+        self.store.set_hash_ids({HASH1: 1, HASH2: 2})
+        self.set_pkg1_installed()
+        self.facade.reload_channels()
+
+        package1 = self.facade.get_packages_by_name("name1")[0]
+        package2 = self.facade.get_packages_by_name("name2")[0]
+        self.facade.perform_changes = self.mocker.mock()
+        self.facade.perform_changes()
+        self.mocker.throw(DependencyError([package1, package2]))
+        self.mocker.replay()
+
+        result = self.changer.change_packages(POLICY_ALLOW_INSTALLS)
+
+        self.assertEquals(result.code, DEPENDENCY_ERROR_RESULT)
+        self.assertEquals(result.text, None)
+        self.assertEquals(result.installs, [2])
+        self.assertEquals(result.removals, [1])
+
+    def test_perform_changes_with_max_retries(self):
+        """
+        After having complemented the requested changes to handle a dependency
+        error, the L{PackageChanger.change_packages} will try to perform the
+        requested changes again only once.
+        """
+        self.store.set_hash_ids({HASH1: 1, HASH2: 2})
+        self.facade.reload_channels()
+
+        package1 = self.facade.get_packages_by_name("name1")[0]
+        package2 = self.facade.get_packages_by_name("name2")[0]
+
+        self.facade.perform_changes = self.mocker.mock()
+        self.facade.perform_changes()
+        self.mocker.throw(DependencyError([package1]))
+        self.facade.perform_changes()
+        self.mocker.throw(DependencyError([package2]))
+        self.mocker.replay()
+
+        result = self.changer.change_packages(POLICY_ALLOW_INSTALLS)
+
+        self.assertEquals(result.code, DEPENDENCY_ERROR_RESULT)
+        self.assertEquals(result.text, None)
+        self.assertEquals(result.installs, [1, 2])
+        self.assertEquals(result.removals, [])
+
+    def test_handle_change_packages_with_policy(self):
+        """
+        The C{change-packages} message can have an optional C{policy}
+        field that will be passed to the C{perform_changes} method.
+        """
+        self.store.set_hash_ids({HASH1: 1})
+        self.store.add_task("changer",
+                            {"type": "change-packages",
+                             "install": [1],
+                             "policy": POLICY_ALLOW_INSTALLS,
+                             "operation-id": 123})
+        self.changer.change_packages = self.mocker.mock()
+        self.changer.change_packages(POLICY_ALLOW_INSTALLS)
+        result = ChangePackagesResult()
+        result.code = SUCCESS_RESULT
+        self.mocker.result(result)
+        self.mocker.replay()
+        return self.changer.handle_tasks()
+
     def test_transaction_error(self):
         """
         In this case, the package we're trying to install declared some
@@ -621,7 +718,7 @@
                                                ".run_task_handler",
                                                passthrough=False)
         setsid = self.mocker.replace("os.setsid")
-        
+
         setsid()
         run_task_handler(PackageChanger, ["ARGS"])
         self.mocker.result("RESULT")