Merge lp:~benji/landscape-client/bug-1428826-restore-register-function-public-API into lp:~landscape/landscape-client/trunk

[Benji York]

Instead of running client configuration as an independent executable
(which would have been preferable), the client charm reaches inside and
calls the register() function directly. That has resulted in a
dependency on register()'s API which was not enforced via testing or
documented in register()'s docstring.

This branch restores the API the charm depends on, adds documentation as
to its meaning and importance, and adds tests that will warn us if the
API is broken in the future.

Instead of running client configuration as an independent executable (which would have been preferable), the client charm reaches inside and calls the register() function directly. That has resulted in a dependency on register()'s API which was not enforced via testing or documented in register()'s docstring.

This branch restores the API the charm depends on, adds documentation as to its meaning and importance, and adds tests that will warn us if the API is broken in the future.

[Björn Tillenius (bjornt)]

+1. I have some minor inline comment on style.

review: Approve

[lp:~benji/landscape-client/bug-1428826-restore-register-function-public-API]

814. By Benji York on 2015-03-09

A few small review changes:

- tweak a test so as to record the fact that a function got called
  and then assert that it didn't instead of making it call
  self.fail() when called
- add some commentary about the charm's use of register() in an
  additional place

[Adam Collard (adam-collard)]

I don't see how this change will make the charm work

Charm code hooks/common.py ~ line 171

            register(config, on_error=error_handler.flag_error)

This branch will fall over because there is no reactor being passed, the docstring suggests it should only be passed if you're doing crazy things, but it's a positional argument (!).

review: Needs Fixing

[lp:~benji/landscape-client/bug-1428826-restore-register-function-public-API]

815. By Benji York on 2015-03-10

make providing a reactor optional (for the client charm)

[Adam Collard (adam-collard)]

Confirmed that a deployment of this branch with trunk landscape-client charm worked fine

cat > client-config.yaml <<EOF
landscape-client:
  origin: lp:~benji/landscape-client/bug-1428826-restore-register-function-public-API
  account-name: adam-collard
EOF

juju deploy landscape-client --config client-config.yaml
juju add-relation landscape-client postgresql

Minor docstring fixes inline from previous comment

review: Approve

[lp:~benji/landscape-client/bug-1428826-restore-register-function-public-API]

816. By Benji York on 2015-03-10

fix docstring/comment bugs

=== modified file 'landscape/configuration.py'
--- landscape/configuration.py	2015-03-04 14:22:17 +0000
+++ landscape/configuration.py	2015-03-10 16:53:34 +0000
@@ -640,28 +640,32 @@
     raise SystemExit
 
 
-def register(config, reactor, connector_factory=RemoteBrokerConnector,
-        got_connection=got_connection, max_retries=14, results=None):
+def register(config, reactor=None, connector_factory=RemoteBrokerConnector,
+        got_connection=got_connection, max_retries=14, on_error=None,
+        results=None):
     """Instruct the Landscape Broker to register the client.
 
     The broker will be instructed to reload its configuration and then to
     attempt a registration.
 
-    @param reactor: The reactor to use.  Please only pass reactor when you
-        have totally mangled everything with mocker.  Otherwise bad things
-        will happen.
+    @param reactor: The reactor to use.  This parameter is optional because
+        the client charm does not pass it.
+    @param connector_factory: A callable that accepts a reactor and a
+        configuration object and returns a new remote broker connection.  Used
+        primarily for dependency injection.
+    @param got_connection: The handler to trigger when the remote broker
+        connects.  Used primarily for dependency injection.
     @param max_retries: The number of times to retry connecting to the
         landscape client service.  The delay between retries is calculated
-        by Twisted and increases geometrically.  The default of 14 results in
-        a total wait time of about 70 seconds.
-
-        initialDelay = 0.05
-        factor =  1.62
-        maxDelay = 30
-        max_retries = 14
-
-        0.05 * (1 - 1.62 ** 14) / (1 - 1.62) = 69 seconds
+        by Twisted and increases geometrically.
+    @param on_error: A callable that will be passed a non-zero positive
+        integer argument in the case that some error occurs.  This is a legacy
+        API provided for use by the client charm.
+    @param results: This parameter provides a mechanism to pre-seed the result
+        of registering.  Used for testing.
     """
+    if reactor is None:
+        reactor = LandscapeReactor()
     if results is None:
         results = []
     add_result = results.append
@@ -675,7 +679,13 @@
 
     assert len(results) == 1, "We expect exactly one result."
     # Results will be things like "success" or "ssl-error".
-    return results[0]
+    result = results[0]
+
+    # If there was an error and the caller requested that errors be reported
+    # to the on_error callable, then do so.
+    if result != "success" and on_error is not None:
+        on_error(1)
+    return result
 
 
 def report_registration_outcome(what_happened, print=print):
 
=== modified file 'landscape/tests/test_configuration.py'
--- landscape/tests/test_configuration.py	2015-03-04 14:22:17 +0000
+++ landscape/tests/test_configuration.py	2015-03-10 16:53:34 +0000
@@ -10,10 +10,7 @@
 from twisted.internet.defer import succeed
 
 from landscape.broker.registration import InvalidCredentialsError
-from landscape.tests.helpers import FakeBrokerServiceHelper
 from landscape.broker.tests.helpers import RemoteBrokerHelper
-from landscape.lib.amp import MethodCallError
-from landscape.lib.fetch import HTTPCodeError, PyCurlError
 from landscape.configuration import (
     print_text, LandscapeSetupScript, LandscapeSetupConfiguration,
     register, setup, main, setup_init_script_and_start_client,
@@ -21,7 +18,10 @@
     ImportOptionError, store_public_key_data,
     bootstrap_tree, got_connection, success, failure, exchange_failure,
     handle_registration_errors, done, got_error, report_registration_outcome)
+from landscape.lib.amp import MethodCallError
+from landscape.lib.fetch import HTTPCodeError, PyCurlError
 from landscape.sysvconfig import SysVConfig, ProcessError
+from landscape.tests.helpers import FakeBrokerServiceHelper
 from landscape.tests.helpers import LandscapeTest, EnvironSaverHelper
 from landscape.tests.mocker import ANY, MATCH, CONTAINS, expect
 
@@ -2010,6 +2010,32 @@
         # We ask for retries because networks aren't reliable.
         self.assertEqual(99, connector.max_retries)
 
+    def test_register_without_reactor(self):
+        """If no reactor is passed, a LandscapeReactor will be instantiated.
+
+        This behaviour is exclusively for compatability with the client charm
+        which does not pass in a reactor.
+        """
+
+        def connector_factory(reactor, config):
+            return FauxConnector(reactor, self.config)
+
+        reactor_mock = self.mocker.replace(
+            "landscape.reactor.LandscapeReactor", passthrough=False)
+        # The mock acts as both the constructor...
+        reactor_mock()
+        self.mocker.result(reactor_mock)
+        # ...and the constructed reactor itself.
+        reactor_mock.run()
+        self.mocker.replay()
+
+        # We pre-seed a success because no actual result will be generated.
+        register(self.config, connector_factory=connector_factory,
+            results=["success"])
+        # The reactor mock being run is what this test asserts, which is
+        # verified by the test infrastructure, so there are no assertions
+        # here.
+
     def test_got_connection(self):
         """got_connection() adds deferreds and callbacks."""
 
@@ -2088,6 +2114,43 @@
             'handle_registration_errors',
             faux_remote.register_deferred.errbacks[0].__name__)
 
+    def test_register_with_on_error_and_an_error(self):
+        """A caller-provided on_error callable will be called if errors occur.
+
+        The on_error parameter is provided for the client charm which calls
+        register() directly and provides on_error as a keyword argument.
+        """
+        def faux_got_connection(add_result, remote, connector, reactor):
+            add_result("something bad")
+
+        on_error_was_called = []
+
+        def on_error(status):
+            # A positive number is provided for the status.
+            self.assertGreater(status, 0)
+            on_error_was_called.append(True)
+
+        self.reactor.call_later(1, self.reactor.stop)
+        register(self.config, reactor=self.reactor, on_error=on_error,
+            got_connection=faux_got_connection)
+        self.assertTrue(on_error_was_called)
+
+    def test_register_with_on_error_and_no_error(self):
+        """A caller-provided on_error callable will not be called if no error.
+        """
+        def faux_got_connection(add_result, remote, connector, reactor):
+            add_result("success")
+
+        on_error_was_called = []
+
+        def on_error(status):
+            on_error_was_called.append(True)
+
+        self.reactor.call_later(1, self.reactor.stop)
+        register(self.config, reactor=self.reactor, on_error=on_error,
+            got_connection=faux_got_connection)
+        self.assertFalse(on_error_was_called)
+
     def test_register_happy_path(self):
         """A successful result provokes no exceptions."""
         def faux_got_connection(add_result, remote, connector, reactor):