Merge lp:~bjornt/landscape-client-charm/update-source into lp:landscape-client-charm

[Björn Tillenius]

Add the new origin/source and upgrade landscape-client if the origin is
changed after the deploy.

Before origin was read only during the install hook, and we ignored any
changes to it after that.

Add the new origin/source and upgrade landscape-client if the origin is
changed after the deploy.

Before origin was read only during the install hook, and we ignored any
changes to it after that.

Most of the changes made are related to testing. I had to copy some
stubs from another charm and pass then along the hooks. It's not ideal,
but it's hard to make it better at this point.

I also removed the use of JujuBroker from the hooks I modified, since
that was written before charmhelpers existed.

Testing instructions:

    juju deploy ubuntu
    juju deploy --series xenial ./
    juju add-relation ubuntu landscape-client

    Wait for ubuntu and landscape-client to be deployed
    (landscape-client will enter a blocked state)

    Optionally: juju config landscape-client account-name=$youraccount
    juju config landscape-client origin=ppa:landscape/trunk

    Confirm that the 17.02 version of landscape-client has been
    installed on the unit.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 67
Branch: lp:~bjornt/landscape-client-charm/update-source
Jenkins: https://ci.lscape.net/job/latch-test-trusty/742/

review: Approve (test results)

[Alberto Donato (ack)]

Nice, +1

A couple of nits inline.

review: Approve

[Free Ekanayaka (free.ekanayaka)]

+1

review: Approve

[🤖 Landscape Builder (landscape-builder)]

Attempt to merge into lp:landscape-client-charm failed due to conflicts:

text conflict in hooks/install.py

[lp:~bjornt/landscape-client-charm/update-source]

70. By Björn Tillenius on 2017-03-14

merge trunk, resolve conflicts.

71. By Björn Tillenius on 2017-03-14

Lint.

[🤖 Landscape Builder (landscape-builder)]

review: Abstain (executing tests)

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 71
Branch: lp:~bjornt/landscape-client-charm/update-source
Jenkins: https://ci.lscape.net/job/latch-test-trusty/848/

review: Approve (test results)

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2016-05-25 10:10:43 +0000
+++ hooks/hooks.py	2017-03-14 09:50:28 +0000
@@ -10,10 +10,14 @@
 from shutil import rmtree
 from subprocess import CalledProcessError
 
+from charmhelpers import fetch
+from charmhelpers.core import hookenv
+
 from common import (
     write_json_file, JujuBroker, LandscapeBroker, chown)
 from ceph import (
     get_ceph_client_path, write_ceph_client_keyring, write_ceph_client_config)
+from install import install_landscape_client
 
 
 CERT_FILE = "/etc/ssl/certs/landscape_server_ca.crt"
@@ -26,29 +30,32 @@
 
 @hooks.hook("registration-relation-joined", "registration-relation-changed")
 def registration_relation(juju_broker=JUJU_BROKER,
+                          hookenv=hookenv, fetch=fetch,
                           landscape_broker=LANDSCAPE_BROKER):
     data = juju_broker.get_relation_config()
     if "ping-url" in data:
-        config_changed(relation_data=data, juju_broker=juju_broker,
+        config_changed(relation_data=data, hookenv=hookenv, fetch=fetch,
                        landscape_broker=landscape_broker)
 
 
 @hooks.hook("config-changed")
-def config_changed(relation_data=None, juju_broker=JUJU_BROKER,
+def config_changed(relation_data=None, hookenv=hookenv, fetch=fetch,
                    landscape_broker=LANDSCAPE_BROKER):
     """
     @param relation_data: data from the context of a relation, it should
                           match the data in the config
     """
-    juju_broker.log("In config-changed for %s" % landscape_broker.local_unit)
+    hookenv.log("In config-changed for %s" % landscape_broker.local_unit)
     if relation_data is None:
         relation_data = {}
-    service_config = juju_broker.get_service_config()
+    service_config = hookenv.config()
+    if service_config.changed("origin"):
+        install_landscape_client(service_config["origin"], fetch=fetch)
     # Only update client config with service configs that have been set
-    service_config = dict(
+    set_config = dict(
         (key, value.strip()) for key, value in service_config.iteritems()
         if value is not None)
-    relation_data.update(service_config)
+    relation_data.update(set_config)
     if relation_data.get("ssl-public-key", "").startswith("base64:"):
         _write_certificate(relation_data["ssl-public-key"][7:], CERT_FILE)
         relation_data["ssl-public-key"] = CERT_FILE

=== modified file 'hooks/install.py'
--- hooks/install.py	2017-03-03 23:03:48 +0000
+++ hooks/install.py	2017-03-14 09:50:28 +0000
@@ -13,30 +13,29 @@
 
 from charmhelpers.core.hookenv import (
     Hooks, UnregisteredHookError, log)
-from charmhelpers.fetch import (
-    apt_install, add_source, apt_update)
-# TODO fix charhelpers to expose this retry command logic somewhere nice.
-from charmhelpers.fetch.ubuntu import (_run_apt_command)
+from charmhelpers import fetch
 from charmhelpers.core.hookenv import config
 
 
 hooks = Hooks()
 
 
+def install_landscape_client(origin, fetch=fetch):
+    """Update the origin and install/upgrade landscape-client."""
+    if origin not in ["distro", None]:
+        add_apt_source(origin, fetch=fetch)
+
+    fetch.apt_update()
+    fetch.apt_install(["landscape-client"])
+
+
 @hooks.hook("install")
 def install():
     charm_config = config()
-    apt_install(["apt-transport-https", "wget"])
-
-    origin = charm_config.get("origin")
-    if origin == "distro":
-        origin = None
-
-    if origin is not None:
-        add_apt_source(origin)
-
-    apt_update()
-    apt_install(["landscape-client"])
+    fetch.apt_install(["apt-transport-https", "wget"])
+
+    install_landscape_client(charm_config.get("origin"))
+
     data_path = charm_config.get("data-path")
     if not data_path:
         data_path = "/var/lib/landscape/client"
@@ -51,7 +50,7 @@
 def build_from_launchpad(url):
     """The charm will install the code from the passed lp branch.
     """
-    apt_install(["devscripts", "bzr", "pbuilder"], fatal=True)
+    fetch.apt_install(["devscripts", "bzr", "pbuilder"], fatal=True)
     subprocess.check_call(["rm", "-rf", "landscape-client-source"])
     subprocess.check_call(["bzr", "branch", url, "landscape-client-source"])
     os.chdir("landscape-client-source")
@@ -64,11 +63,11 @@
                      glob("../landscape-common_*.deb")[0]])
     # The _run_with_retries will ensure the command is retried in case we can't
     # acquire the lock for some reason.
-    _run_apt_command(["apt-get", "-f", "install"], fatal=True)
+    fetch._run_apt_command(["apt-get", "-f", "install"], fatal=True)
     os.chdir("..")
 
 
-def add_apt_source(url):
+def add_apt_source(url, fetch=fetch):
     """Add an apt source entry, with passed in key.
 
     This is a thin wrapper over the charmhelper "add_source" method to allow
@@ -87,7 +86,7 @@
         if key == "":
             log("Archive key for '%s' is empty.", level="WARNING")
 
-    return add_source(url, key)
+    return fetch.add_source(url, key)
 
 if __name__ == '__main__':
     try:

=== added file 'hooks/stubs.py'
--- hooks/stubs.py	1970-01-01 00:00:00 +0000
+++ hooks/stubs.py	2017-03-14 09:50:28 +0000
@@ -0,0 +1,81 @@
+import os.path
+from collections import namedtuple
+
+from charmhelpers.core.hookenv import (
+    Config,
+    CRITICAL,
+    ERROR,
+    WARNING,
+    INFO,
+    DEBUG,
+)
+
+LEVELS = (CRITICAL, ERROR, WARNING, INFO, DEBUG)
+
+
+class HookenvStub:
+    """Testable stub for charmhelpers.core.hookenv."""
+
+    def __init__(self, charm_dir):
+        self.messages = []
+        self.port = None
+        self._config = Config()
+        self._charm_dir = charm_dir
+        self.action_messages = []
+        self.action_details = []
+        self.statuses = []
+
+    def config(self):
+        return self._config
+
+    def charm_dir(self):
+        return self._charm_dir
+
+    def log(self, message, level=None):
+        if level is not None:
+            assert level in LEVELS
+        self.messages.append((message, level))
+
+    def open_port(self, port):
+        self.port = port
+
+    def action_fail(self, message):
+        self.action_messages.append(message)
+
+    def action_set(self, details):
+        self.action_details.append(details)
+
+    def status_set(self, status, message):
+        self.statuses.append((status, message.decode("utf-8")))
+
+    def local_unit(self):
+        return os.path.basename(self._charm_dir) + "/0"
+
+
+Update = namedtuple("Update", ["fatal"])
+Install = namedtuple("Install", ["packages", "options", "fatal"])
+
+
+class FetchStub:
+    """Testable stub for charmhelpers.fetch."""
+
+    def __init__(self, needs_update=False):
+        self.needs_update = needs_update
+        self.updates = []
+        self.installs = []
+        self.package_status = {}
+        self.sources = []
+
+    def apt_update(self, fatal=False):
+        self.needs_update = False
+        self.updates.append(Update(fatal))
+
+    def apt_install(self, packages, options=None, fatal=False):
+        if self.needs_update:
+            raise AssertionError("APT cache wasn't updated.")
+        for package in packages:
+            self.package_status[package] = "install"
+        self.installs.append(Install(packages, options, fatal))
+
+    def add_source(self, url, key=None):
+        self.sources.append((url, key))

=== modified file 'hooks/test_hooks.py'
--- hooks/test_hooks.py	2016-05-25 10:37:20 +0000
+++ hooks/test_hooks.py	2017-03-14 09:50:28 +0000
@@ -10,6 +10,7 @@
 from landscape.lib.fs import read_file
 import hooks
 from common import LandscapeBroker, JujuBroker, write_json_file
+from stubs import FetchStub, HookenvStub
 
 
 class FakeJujuBroker(JujuBroker):
@@ -66,6 +67,10 @@
         self.landscape_broker.config.write()
         self.workload_state = None
         self.workload_message = None
+        self.charm_dir = self.mkdtemp()
+        os.environ["CHARM_DIR"] = self.charm_dir
+        self.hookenv = HookenvStub(self.charm_dir)
+        self.fetch = FetchStub(needs_update=True)
 
         def status_set(workload_state, message):
             self.workload_state = workload_state
@@ -82,6 +87,12 @@
         self.addCleanup(rmtree, directory)
         return directory
 
+    def config(self):
+        """Return the base application config."""
+        config = self.hookenv.config()
+        config.update({"origin": "distro"})
+        return config
+
 
 class MigrateOldJujuInfoTest(HookTestCase):
 
@@ -427,12 +438,13 @@
         A base64 encoded SSL cert in the configuration is decoded.
         """
         self.touch_juju_info_file()
-        self.juju_broker.commands["config-get"] = {
+        config = self.config()
+        config.update({
             "ssl-public-key": "base64:%s" % "foo".encode("base64"),
             "account-name": "standalone",
-            "ping-url": "http://127.0.0.1"}
+            "ping-url": "http://127.0.0.1"})
 
-        hooks.config_changed(juju_broker=self.juju_broker,
+        hooks.config_changed(hookenv=self.hookenv, fetch=self.fetch,
                              landscape_broker=self.landscape_broker)
         self.assertEqual(
             hooks.CERT_FILE, self.landscape_broker.config.ssl_public_key)
@@ -447,12 +459,13 @@
         If account-name is not set, the unit transitions to the blocked state.
         """
         self.touch_juju_info_file()
-        self.juju_broker.commands["config-get"] = {
+        config = self.config()
+        config.update({
             "ssl-public-key": "base64:%s" % "foo".encode("base64"),
             "account-name": "",
-            "ping-url": "http://127.0.0.1"}
+            "ping-url": "http://127.0.0.1"})
 
-        hooks.config_changed(juju_broker=self.juju_broker,
+        hooks.config_changed(hookenv=self.hookenv, fetch=self.fetch,
                              landscape_broker=self.landscape_broker)
         self.assertEqual("blocked", self.workload_state)
         self.assertEqual(
@@ -464,12 +477,13 @@
         If the juju-info file hasn't been written yet, the unit transitions to
         the maintenance status.
         """
-        self.juju_broker.commands["config-get"] = {
+        config = self.config()
+        config.update({
             "ssl-public-key": "base64:%s" % "foo".encode("base64"),
             "account-name": "standalone",
-            "ping-url": "http://127.0.0.1"}
+            "ping-url": "http://127.0.0.1"})
 
-        hooks.config_changed(juju_broker=self.juju_broker,
+        hooks.config_changed(hookenv=self.hookenv, fetch=self.fetch,
                              landscape_broker=self.landscape_broker)
         self.assertEqual("maintenance", self.workload_state)
         self.assertEqual(
@@ -478,12 +492,13 @@
 
     def test_whitespace_removed_from_config_settings(self):
         """Leading and trailing whitespace is removed from config settings"""
-        self.juju_broker.commands["config-get"] = {
+        config = self.config()
+        config.update({
             "ssl-public-key": " base64:%s" % "foo".encode("base64"),
             "account-name": " standalone\t",
             "registration-key": " mykey\n",
-            "ping-url": " \thttp://127.0.0.1\n "}
-        hooks.config_changed(juju_broker=self.juju_broker,
+            "ping-url": " \thttp://127.0.0.1\n "})
+        hooks.config_changed(hookenv=self.hookenv, fetch=self.fetch,
                              landscape_broker=self.landscape_broker)
         self.assertEqual(
             hooks.CERT_FILE, self.landscape_broker.config.ssl_public_key)
@@ -496,6 +511,73 @@
         self.assertEqual(
             "http://127.0.0.1", self.landscape_broker.config.ping_url)
 
+    def test_update_origin(self):
+        """"
+        If origin is set, the origin will be added as an apt source and
+        landscape-client will be installed.
+        """
+        config = self.config()
+        config.update({"origin": "ppa:landscape-client/stable"})
+        hooks.config_changed(hookenv=self.hookenv, fetch=self.fetch,
+                             landscape_broker=self.landscape_broker)
+        self.assertEqual(
+            [("ppa:landscape-client/stable", None)], self.fetch.sources)
+        [install] = self.fetch.installs
+        self.assertEqual(["landscape-client"], install.packages)
+
+    def test_update_origin_twice(self):
+        """"
+        If origin hasn't been changed since the last time the
+        config-changed hook ran, the origin won't be added again, and
+        landscape-client won't be installed again.
+        """
+        config = self.config()
+        config.update({"origin": "ppa:landscape-client/stable"})
+        hooks.config_changed(hookenv=self.hookenv, fetch=self.fetch,
+                             landscape_broker=self.landscape_broker)
+        # Saving and loading the config is taken care of by
+        # Hooks.execute(), which we can't use in the test.
+        config.save()
+        config.load_previous()
+        hooks.config_changed(hookenv=self.hookenv, fetch=self.fetch,
+                             landscape_broker=self.landscape_broker)
+        # The single origin and the landscape-client install were added
+        # by the first config-changed hook execution.
+        self.assertEqual(
+            [("ppa:landscape-client/stable", None)], self.fetch.sources)
+        self.assertEqual(1, len(self.fetch.installs))
+        [install] = self.fetch.installs
+        self.assertEqual(["landscape-client"], install.packages)
+
+    def test_update_origin_with_key(self):
+        """
+        If the origin contains a key, the key will be added together
+        with the apt source.
+        """
+        config = self.config()
+        config.update({"origin": "ppa:landscape-client/stable|key-id"})
+        hooks.config_changed(hookenv=self.hookenv, fetch=self.fetch,
+                             landscape_broker=self.landscape_broker)
+        self.assertEqual(
+            [("ppa:landscape-client/stable", "key-id")], self.fetch.sources)
+
+    def test_update_origin_with_account_name(self):
+        """
+        If the account-name is set and the client already registered,
+        updating the origin will result in the apt source being added
+        and landscape-client installed/upgraded.
+        """
+        config = self.config()
+        config.update({
+            "account-name": "example",
+            "origin": "ppa:landscape-client/stable"})
+        hooks.config_changed(hookenv=self.hookenv, fetch=self.fetch,
+                             landscape_broker=self.landscape_broker)
+        self.assertEqual(
+            [("ppa:landscape-client/stable", None)], self.fetch.sources)
+        [install] = self.fetch.installs
+        self.assertEqual(["landscape-client"], install.packages)
+
 
 class RegistrationRelationJoinedTest(HookTestCase):
     """Test for the registration-relation-joined hook."""
@@ -508,6 +590,7 @@
         self.addCleanup(setattr, hooks, "CERT_FILE", hooks.CERT_FILE)
         tmp_dir = self.mkdtemp()
         hooks.CERT_FILE = "%s/cert" % tmp_dir
+        self.config()
 
     def test_ssl_cert_decoded(self):
         """
@@ -518,6 +601,7 @@
             "account-name": "standalone",
             "ping-url": "http://127.0.0.1/ping"}
         hooks.registration_relation(juju_broker=self.juju_broker,
+                                    hookenv=self.hookenv, fetch=self.fetch,
                                     landscape_broker=self.landscape_broker)
         self.assertEqual(
             hooks.CERT_FILE, self.landscape_broker.config.ssl_public_key)
@@ -533,6 +617,7 @@
             "account-name": "standalone",
             "ping-url": "http://127.0.0.1/ping"}
         hooks.registration_relation(juju_broker=self.juju_broker,
+                                    hookenv=self.hookenv, fetch=self.fetch,
                                     landscape_broker=self.landscape_broker)
         self.assertEqual("/etc/ssl/my-cert.pem",
                          self.landscape_broker.config.ssl_public_key)