Merge lp:~free.ekanayaka/landscape-charm/install-from-deb into lp:~landscape/landscape-charm/trunk

[Free Ekanayaka]

This branch adds support for installing landscape-server packages built from a local tarball.

This branch adds support for installing landscape-server packages built from a local tarball.

To test it:

bzr branch lp:~free.ekanayaka/landscape-charm/install-from-deb
mkdir trusty
cd trusty
ln -s ../install-from-deb landscape-server
cd ..
# Let's build a tarball from a branch that hasn't landed yet
bzr branch lp:~free.ekanayaka/landscape/pingserver-head
cd pingserver-head
make build
cd standalone
make standalone-package-source
mv landscape-server_*.tar.gz ../../install-from-deb
cd ../..
# Copy this https://pastebin.canonical.com/127051/ to deployer.yaml
juju-deployer -vdW -w 180 -c deployer.yaml landscape

Then notice that the deployed package was built from the tarball, and
the pingserver actually answers HEAD requests.

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 240
Branch: lp:~free.ekanayaka/landscape-charm/install-from-deb
Jenkins: https://ci.lscape.net/job/latch-test/148/

review: Approve (test results)

[Adam Collard (adam-collard)]

review: Needs Fixing

[lp:~free.ekanayaka/landscape-charm/install-from-deb]

241. By Free Ekanayaka on 2015-03-09

Use hashlib instead of os.system

242. By Free Ekanayaka on 2015-03-09

Address review comments

[🤖 Landscape Builder (landscape-builder)]

Command: make ci-test
Result: Success
Revno: 242
Branch: lp:~free.ekanayaka/landscape-charm/install-from-deb
Jenkins: https://ci.lscape.net/job/latch-test/172/

review: Approve (test results)

[Björn Tillenius (bjornt)]

+1 with Adam's comments addressed. Ideally I would have like some more functional tests, since checking subprocess call parameters is really fragile. But I appreciate that issuing real commands isn't trivial.

review: Approve

[Free Ekanayaka (free.ekanayaka)]

> +1 with Adam's comments addressed. Ideally I would have like some more
> functional tests, since checking subprocess call parameters is really fragile.
> But I appreciate that issuing real commands isn't trivial.

Right. Note that this code is only meaningful for development, is not production code.

[Adam Collard (adam-collard)]

+1 thanks

review: Approve

=== added file 'hooks/config-changed'
--- hooks/config-changed	1970-01-01 00:00:00 +0000
+++ hooks/config-changed	2015-03-09 11:56:41 +0000
@@ -0,0 +1,9 @@
+#!/usr/bin/python
+import sys
+
+from lib.config import ConfigHook
+
+
+if __name__ == "__main__":
+    hook = ConfigHook()
+    sys.exit(hook())

=== renamed file 'hooks/lib/install.py' => 'hooks/lib/apt.py'
--- hooks/lib/install.py	2015-01-28 11:53:18 +0000
+++ hooks/lib/apt.py	2015-03-09 11:56:41 +0000
@@ -1,31 +1,138 @@
+import glob
+import hashlib
+import os
+import shutil
+import subprocess
+
 from charmhelpers import fetch
+from charmhelpers.core import hookenv
 
-from lib.hook import Hook, HookError
+from lib.hook import HookError
 
 PACKAGES = ("landscape-server",)
-
-
-class InstallHook(Hook):
-    """Execute install hook logic."""
-
-    def __init__(self, fetch=fetch, **kwargs):
-        super(InstallHook, self).__init__(**kwargs)
+PACKAGES_DEV = ("dpkg-dev", "pbuilder")
+TARBALL = "landscape-server_*.tar.gz"
+
+# XXX Default options taken from charmhelpers, there's no way to just
+#     extend them.
+DEFAULT_INSTALL_OPTIONS = ("--option=Dpkg::Options::=--force-confold",)
+
+# Shell commands to build the debs and publish them in a local repository
+BUILD_LOCAL_ARCHIVE = """
+dch -v 9999:$(dpkg-parsechangelog|grep ^Version:|cut -d ' ' -f 2) \
+    development --distribution $(lsb_release -cs) &&
+/usr/lib/pbuilder/pbuilder-satisfydepends &&
+dpkg-buildpackage -us -uc &&
+mv ../*.deb . &&
+dpkg-scanpackages -m . /dev/null > Packages &&
+cat Packages | bzip2 -9 > Packages.bz2 &&
+cat Packages | gzip -9 > Packages.gz &&
+dpkg-scansources . > Sources &&
+cat Sources | bzip2 -9 > Sources.bz2 &&
+cat Sources | gzip -9 > Sources.gz &&
+apt-ftparchive release . > Release
+"""
+
+
+class Apt(object):
+    """Perform APT-related tasks as setting sources and installing packages.
+
+    This is a thin facade around C{charmhelpers.fetch}, offering some
+    additional features, like building Landscape packages from a local
+    tarball.
+    """
+
+    def __init__(self, hookenv=hookenv, fetch=fetch, subprocess=subprocess):
+        self._hookenv = hookenv
         self._fetch = fetch
-
-    def _run(self):
-        self._configure_sources()
-        self._install_packages()
-
-    def _configure_sources(self):
+        self._subprocess = subprocess
+
+    def set_sources(self):
         """Configure the extra APT sources to use."""
+        needs_update = False
+        if self._set_remote_source():
+            needs_update = True
+        if self._set_local_source():
+            needs_update = True
+        if needs_update:
+            self._fetch.apt_update(fatal=True)
+
+    def install_packages(self):
+        """Install the needed packages."""
+        options = list(DEFAULT_INSTALL_OPTIONS)
+        if self._get_local_tarball() is not None:
+            # We don't sign the locally built repository, so we need to tell
+            # apt-get that we don't care.
+            options.append("--allow-unauthenticated")
+        packages = self._fetch.filter_installed_packages(PACKAGES)
+        self._fetch.apt_install(packages, options=options, fatal=True)
+
+    def _set_remote_source(self):
+        """Set the remote APT repository to use, if new or changed."""
         config = self._hookenv.config()
         source = config.get("source")
         if not source:
             raise HookError("No source config parameter defined")
+        previous_source = config.previous("source")
+
+        # Check if we're setting the source for the first time, or replacing
+        # an existing value. In the latter case we'll no-op if the value is the
+        # same or take care to remove it from sources.list if it's not.
+        previous_source = config.previous("source")
+        if previous_source is not None:
+            if previous_source == source:
+                return False
+            self._subprocess.check_call(
+                ["add-apt-repository", "--remove", "--yes", previous_source])
+
         self._fetch.add_source(source, config.get("key"))
-        self._fetch.apt_update(fatal=True)
-
-    def _install_packages(self):
-        """Install the needed packages."""
-        packages = self._fetch.filter_installed_packages(PACKAGES)
+
+        return True
+
+    def _set_local_source(self):
+        """Set the local APT repository for the Landscape tarball, if any."""
+        tarball = self._get_local_tarball()
+        if tarball is None:
+            return False
+
+        if not self._is_tarball_new(tarball):
+            return False
+
+        packages = self._fetch.filter_installed_packages(PACKAGES_DEV)
         self._fetch.apt_install(packages, fatal=True)
+
+        build_dir = os.path.join(self._hookenv.charm_dir(), "build")
+        shutil.rmtree(build_dir, ignore_errors=True)
+        os.mkdir(build_dir)
+
+        self._subprocess.check_call(
+            ["tar", "--strip=1", "-xf", tarball], cwd=build_dir)
+        self._subprocess.check_call(
+            BUILD_LOCAL_ARCHIVE, shell=True, cwd=build_dir)
+
+        self._fetch.add_source("deb file://%s/ ./" % build_dir)
+
+        return True
+
+    def _get_local_tarball(self):
+        """Return the local Landscape tarball if any, C{None} otherwise."""
+        matches = glob.glob(os.path.join(self._hookenv.charm_dir(), TARBALL))
+        return matches[0] if matches else None
+
+    def _is_tarball_new(self, tarball):
+        """Check if this is a new tarball and we need to build it."""
+        with open(tarball, "r") as fd:
+            digest = hashlib.md5(fd.read()).hexdigest()
+
+        md5sum = tarball + ".md5sum"
+        if os.path.exists(md5sum):
+            with open(md5sum, "r") as fd:
+                if fd.read() == digest:
+                    # The checksum matches, so it's not a new tarball
+                    return False
+
+        # Update the md5sum file, since this is a new tarball.
+        with open(md5sum, "w") as fd:
+            fd.write(digest)
+
+        return True

=== added file 'hooks/lib/config.py'
--- hooks/lib/config.py	1970-01-01 00:00:00 +0000
+++ hooks/lib/config.py	2015-03-09 11:56:41 +0000
@@ -0,0 +1,23 @@
+import subprocess
+
+from charmhelpers import fetch
+from charmhelpers.core import hookenv
+
+from lib.hook import Hook
+from lib.apt import Apt
+
+
+class ConfigHook(Hook):
+    """Execute config-changed hook logic."""
+
+    def __init__(self, hookenv=hookenv, fetch=fetch, subprocess=subprocess):
+        super(ConfigHook, self).__init__(hookenv=hookenv)
+        self._fetch = fetch
+        self._subprocess = subprocess
+
+    def _run(self):
+        # Re-set APT sources, if the have changed.
+        apt = Apt(
+            hookenv=self._hookenv, fetch=self._fetch,
+            subprocess=self._subprocess)
+        apt.set_sources()

=== added file 'hooks/lib/install.py'
--- hooks/lib/install.py	1970-01-01 00:00:00 +0000
+++ hooks/lib/install.py	2015-03-09 11:56:41 +0000
@@ -0,0 +1,24 @@
+import subprocess
+
+from charmhelpers import fetch
+from charmhelpers.core import hookenv
+
+from lib.hook import Hook
+from lib.apt import Apt
+
+
+class InstallHook(Hook):
+    """Execute install hook logic."""
+
+    def __init__(self, hookenv=hookenv, fetch=fetch, subprocess=subprocess):
+        super(InstallHook, self).__init__(hookenv=hookenv)
+        self._fetch = fetch
+        self._subprocess = subprocess
+
+    def _run(self):
+        # Set APT sources and install Landscape packages
+        apt = Apt(
+            hookenv=self._hookenv, fetch=self._fetch,
+            subprocess=self._subprocess)
+        apt.set_sources()
+        apt.install_packages()

=== modified file 'hooks/lib/tests/helpers.py'
--- hooks/lib/tests/helpers.py	2015-01-29 09:55:59 +0000
+++ hooks/lib/tests/helpers.py	2015-03-09 11:56:41 +0000
@@ -26,7 +26,7 @@
         #     upstream.
         charm_dir = self.useFixture(TempDir())
         self.useFixture(EnvironmentVariable("CHARM_DIR", charm_dir.path))
-        self.hookenv = HookenvStub()
+        self.hookenv = HookenvStub(charm_dir.path)
 
         if self.with_hookenv_monkey_patch:
             self._monkey_patch_hookenv()

=== modified file 'hooks/lib/tests/stubs.py'
--- hooks/lib/tests/stubs.py	2015-03-02 12:39:25 +0000
+++ hooks/lib/tests/stubs.py	2015-03-09 11:56:41 +0000
@@ -9,10 +9,11 @@
     unit = "landscape-server/0"
     relid = None
 
-    def __init__(self):
+    def __init__(self, charm_dir):
         self.messages = []
         self.relations = {}
         self._config = Config()
+        self._charm_dir = charm_dir
 
     def config(self):
         return self._config
@@ -58,6 +59,9 @@
     def relation_set(self, rid=None, relation_settings=None, **kwargs):
         self.relations[rid] = relation_settings
 
+    def charm_dir(self):
+        return self._charm_dir
+
 
 class FetchStub(object):
     """Provide a testable stub for C{charmhelpers.fetch}."""
@@ -78,8 +82,8 @@
         self.filtered.append(packages)
         return packages
 
-    def apt_install(self, packages, fatal=False):
-        self.installed.append((packages, fatal))
+    def apt_install(self, packages, options=None, fatal=False):
+        self.installed.append((packages, options, fatal))
 
 
 class ClusterStub(object):

=== renamed file 'hooks/lib/tests/test_install.py' => 'hooks/lib/tests/test_apt.py'
--- hooks/lib/tests/test_install.py	2015-01-21 20:37:36 +0000
+++ hooks/lib/tests/test_apt.py	2015-03-09 11:56:41 +0000
@@ -1,37 +1,110 @@
-from charmhelpers.core.hookenv import ERROR
+import os
 
-from lib.install import InstallHook, PACKAGES
-from lib.tests.stubs import FetchStub
+from lib.apt import Apt, PACKAGES, BUILD_LOCAL_ARCHIVE, DEFAULT_INSTALL_OPTIONS
+from lib.hook import HookError
+from lib.tests.stubs import FetchStub, SubprocessStub
 from lib.tests.helpers import HookenvTest
 
 
-class InstallHookTest(HookenvTest):
+class AptTest(HookenvTest):
 
     def setUp(self):
-        super(InstallHookTest, self).setUp()
+        super(AptTest, self).setUp()
         self.fetch = FetchStub()
-        self.hook = InstallHook(
-            fetch=self.fetch,
-            hookenv=self.hookenv)
+        self.subprocess = SubprocessStub()
+        self.apt = Apt(
+            hookenv=self.hookenv, fetch=self.fetch, subprocess=self.subprocess)
 
     def test_no_source(self):
         """
-        If no APT source is defined the install hook logs an error
-        message and exists with code 1.
-        """
-        self.assertEqual(1, self.hook())
-        self.assertEqual(
-            ("No source config parameter defined", ERROR),
-            self.hookenv.messages[-1])
-
-    def test_add_source(self):
-        """
-        The install hook adds the configured APT source and refreshes it.
-        """
-        self.hookenv.config()["source"] = "ppa:landscape/14.10"
-        self.assertEqual(0, self.hook())
-        self.assertEqual([("ppa:landscape/14.10", None)], self.fetch.sources)
-        self.assertEqual([True], self.fetch.updates)
+        If no APT source is defined, we fail with a L{HookError}.
+        """
+        with self.assertRaises(HookError) as error:
+            self.apt.set_sources()
+        self.assertEqual(
+            "No source config parameter defined", str(error.exception))
+
+    def test_set_sources(self):
+        """
+        The C{set_sources} method adds the configured APT source and
+        refreshes it.
+        """
+        self.hookenv.config()["source"] = "ppa:landscape/14.10"
+        self.apt.set_sources()
+        self.assertEqual([("ppa:landscape/14.10", None)], self.fetch.sources)
+        self.assertEqual([True], self.fetch.updates)
+
+    def test_set_sources_not_changed(self):
+        """
+        The C{set_sources} method is a no-op if the source config hasn't
+        changed.
+        """
+        config = self.hookenv.config()
+        config["source"] = "ppa:landscape/14.10"
+        config.save()
+        config.load_previous()
+        self.apt.set_sources()
+        self.assertEqual([], self.fetch.sources)
+        self.assertEqual([], self.fetch.updates)
+
+    def test_set_sources_replace(self):
+        """
+        The C{set_sources} method removes any previous source before setting
+        the new one.
+        """
+        config = self.hookenv.config()
+        config["source"] = "ppa:landscape/14.10"
+        config.save()
+        config.load_previous()
+        config["source"] = "ppa:landscape/15.01"
+        self.apt.set_sources()
+        self.assertEqual(
+            ["add-apt-repository", "--remove", "--yes", "ppa:landscape/14.10"],
+            self.subprocess.calls[0][0])
+        self.assertEqual([("ppa:landscape/15.01", None)], self.fetch.sources)
+        self.assertEqual([True], self.fetch.updates)
+
+    def test_local_tarball(self):
+        """
+        If a Landscape tarball is found, the C{set_sources} method builds local
+        repository with the relevant deb packages.
+        """
+        self.hookenv.config()["source"] = "ppa:landscape/14.10"
+        tarball = os.path.join(
+            self.hookenv.charm_dir(), "landscape-server_1.2.3.tar.gz")
+        with open(tarball, "w") as fd:
+            fd.write("")
+        self.apt.set_sources()
+
+        build_dir = os.path.join(self.hookenv.charm_dir(), "build")
+
+        self.assertEqual(
+            [(["tar", "--strip=1", "-xf", tarball],  {"cwd": build_dir}),
+             (BUILD_LOCAL_ARCHIVE, {"shell": True, "cwd": build_dir})],
+            self.subprocess.calls)
+
+        self.assertEqual(
+            [("ppa:landscape/14.10", None),
+             ("deb file://%s/build/ ./" % self.hookenv.charm_dir(), None)],
+            self.fetch.sources)
+
+    def test_local_tarball_not_new(self):
+        """
+        If the landscape tarball hasn't changed, it won't be built.
+        """
+        self.hookenv.config()["source"] = "ppa:landscape/14.10"
+        tarball = os.path.join(
+            self.hookenv.charm_dir(), "landscape-server_1.2.3.tar.gz")
+        with open(tarball, "w") as fd:
+            fd.write("data")
+        self.apt.set_sources()
+
+        # Reset the recorded sources and subprocess calls and run again
+        self.subprocess.calls[:] = []
+        self.fetch.sources[:] = []
+        self.apt.set_sources()
+        self.assertEqual([], self.subprocess.calls)
+        self.assertEqual([("ppa:landscape/14.10", None)], self.fetch.sources)
 
     def test_packages(self):
         """
@@ -41,9 +114,24 @@
 
     def test_install(self):
         """
-        The install hook installs the required packages.
+        The C{install_packages} method installs the required packages.
         """
         self.hookenv.config()["source"] = "ppa:landscape/14.10"
-        self.assertEqual(0, self.hook())
+        self.apt.install_packages()
         self.assertEqual([PACKAGES], self.fetch.filtered)
-        self.assertEqual([(PACKAGES, True)], self.fetch.installed)
+        options = list(DEFAULT_INSTALL_OPTIONS)
+        self.assertEqual([(PACKAGES, options, True)], self.fetch.installed)
+
+    def test_install_with_local_tarball(self):
+        """
+        The C{install_packages} method allows unauthenticated packages if we
+        have a locally built repository.
+        """
+        tarball = os.path.join(
+            self.hookenv.charm_dir(), "landscape-server_1.2.3.tar.gz")
+        with open(tarball, "w") as fd:
+            fd.write("")
+        self.hookenv.config()["source"] = "ppa:landscape/14.10"
+        self.apt.install_packages()
+        options = list(DEFAULT_INSTALL_OPTIONS) + ["--allow-unauthenticated"]
+        self.assertEqual([(PACKAGES, options, True)], self.fetch.installed)

=== added file 'hooks/lib/tests/test_config.py'
--- hooks/lib/tests/test_config.py	1970-01-01 00:00:00 +0000
+++ hooks/lib/tests/test_config.py	2015-03-09 11:56:41 +0000
@@ -0,0 +1,25 @@
+from lib.tests.helpers import HookenvTest
+from lib.tests.stubs import FetchStub, SubprocessStub
+from lib.config import ConfigHook
+
+
+class ConfigHookTest(HookenvTest):
+
+    def setUp(self):
+        super(ConfigHookTest, self).setUp()
+        self.fetch = FetchStub()
+        self.subprocess = SubprocessStub()
+        self.hook = ConfigHook(
+            hookenv=self.hookenv, fetch=self.fetch, subprocess=self.subprocess)
+
+    def test_run(self):
+        """
+        The L{ConfigHook} re-configures APT sources if the have changed.
+        """
+        config = self.hookenv.config()
+        config["source"] = "ppa:landscape/14.10"
+        config.save()
+        config.load_previous()
+        config["source"] = "ppa:landscape/15.01"
+        self.assertEqual(0, self.hook())
+        self.assertTrue(len(self.fetch.sources) > 0)

=== added file 'hooks/lib/tests/test_install.py'
--- hooks/lib/tests/test_install.py	1970-01-01 00:00:00 +0000
+++ hooks/lib/tests/test_install.py	2015-03-09 11:56:41 +0000
@@ -0,0 +1,23 @@
+from lib.tests.helpers import HookenvTest
+from lib.tests.stubs import FetchStub, SubprocessStub
+from lib.install import InstallHook
+
+
+class InstallHookTest(HookenvTest):
+
+    def setUp(self):
+        super(InstallHookTest, self).setUp()
+        self.fetch = FetchStub()
+        self.subprocess = SubprocessStub()
+        self.hook = InstallHook(
+            hookenv=self.hookenv, fetch=self.fetch, subprocess=self.subprocess)
+
+    def test_run(self):
+        """
+        The L{InstallHook} configures APT sources and install the needed
+        packages.
+        """
+        self.hookenv.config()["source"] = "ppa:landscape/14.10"
+        self.assertEqual(0, self.hook())
+        self.assertNotEqual([], self.fetch.sources)
+        self.assertNotEqual([], self.fetch.installed)