Merge lp:~bjornt/landscape-client/apt-channel-api into lp:~landscape/landscape-client/trunk

[Björn Tillenius]

Add support for adding and removing deb URLs in AptFacade.

add_channel_apt_deb() and reset_channels() were implemented.
add_channel() was omitted, since it's only used by SmartFacade
internally.

I filed bug 861345 for implementing add_channel_deb_dir, since we have
to do some extra work to make sure apt can use the dir.

[Alberto Donato (ack)]

Nice, +1!

#1:
+ sources_file_path = sources_dir + "/landscape-internal-facade.list"

#2:
+ with open(path + "/Packages", "a") as packages:

these should use os.path.join()

#3:
+ with open(list_filename, "r") as sources:
+ sources_contents = sources.read()

you can use read_file() from landscape.lib.fs here (there are multiple tests with similar code)

review: Approve

[Free Ekanayaka (free.ekanayaka)]

Nice work, +1!

[1]

+ sources_file_path = sources_dir + "/landscape-internal-facade.list"

Please use os.path.join().

Also, wdyt of prefixing the file with an underscore to signal it's a "private" file? like "_landscape-internal-facade.list" (unfortunately using a dot as prefix won't work because libapt ignores it).

[2]

+ add_channel_apt_deb() adds a new deb URL to a file in

Please write it with epydoc syntax, "C{add_channel_apt_deb}". Same for the other occurrences.

[3]

+ self.facade.add_channel_apt_deb(
+ "http://example.com/ubuntu", "lucid", None)

It'd be nice to make the components parameter optional in add_channel_apt_deb. I understand that it's not optional in the SmartFacade, but it should be compatible.

[4]

+ If deb URLs have been added, a list of dict is returned with
+ information about the channels.

The second line is indented too much.

[5]

+ self.facade.add_channel_apt_deb(
+ "http://enabled.example.com/ubuntu", "lucid", ["main"])
+ self.facade.add_channel_apt_deb(
+ "http://disabled.example.com/ubuntu", "lucid", ["main"])
+ sources_list = SourcesList()
+ for entry in sources_list:
+ if "disabled" in entry.uri:
+ entry.set_enabled(False)
+ sources_list.save()

Doh, SourcesList() is nice and convenient, just too bad that it relies on global state! Anyway, good idea to use it, I didn't know or recall about it.

[6]

+ def reset_channels(self):
+ """Remove all the configured channels."""
+ sources_list = SourcesList()
+ for entry in sources_list:
+ entry.set_enabled(False)

The only use case for reset_channels() in the server code, canonical/landscape/scripts/hashids.py, where we update the lists of pre-canned hash->id maps. I think it's pretty safe to remove the entry entirely for that, like:

        for entry in sources_list:
            sources_list.remove(entry)

which I think would fit more with the "reset" term. If we need a "disable" behavior we could add a disable_channels() method.

Anyway, this is really minor, so your call.

review: Approve

[lp:~bjornt/landscape-client/apt-channel-api]

385. By Björn Tillenius on 2011-09-29

Merge trunk.

386. By Björn Tillenius on 2011-09-29

Use os.path.join.

387. By Björn Tillenius on 2011-09-29

Use read_file().

388. By Björn Tillenius on 2011-09-29

Add append_file() and use it to get rid of with statements.

389. By Björn Tillenius on 2011-09-29

Rename .list file to make it more obvious that it's private.

390. By Björn Tillenius on 2011-09-29

Epydoc syntax.

[Björn Tillenius (bjornt)]

On Wed, Sep 28, 2011 at 12:56:28PM -0000, Alberto Donato wrote:
> Review: Approve
>
> Nice, +1!
>
> #1:
> + sources_file_path = sources_dir + "/landscape-internal-facade.list"
>
> #2:
> + with open(path + "/Packages", "a") as packages:
>
> these should use os.path.join()

Sure.

> #3:
> + with open(list_filename, "r") as sources:
> + sources_contents = sources.read()
>
> you can use read_file() from landscape.lib.fs here (there are multiple
> tests with similar code)

Sure. I added append_file() as well, and replaced the with statements
that appended contents to files.

--
Björn Tillenius | https://launchpad.net/~bjornt

[Björn Tillenius (bjornt)]

On Wed, Sep 28, 2011 at 01:00:27PM -0000, Free Ekanayaka wrote:
> Review: Approve
>
> Nice work, +1!
>
> [1]
>
> + sources_file_path = sources_dir + "/landscape-internal-facade.list"
>
> Please use os.path.join().

Sure.

> Also, wdyt of prefixing the file with an underscore to signal it's a
> "private" file? like "_landscape-internal-facade.list" (unfortunately
> using a dot as prefix won't work because libapt ignores it).

Yes, that's a good idea.

> [2]
>
> + add_channel_apt_deb() adds a new deb URL to a file in
>
> Please write it with epydoc syntax, "C{add_channel_apt_deb}". Same for
> the other occurrences.

Done.

> [3]
>
> + self.facade.add_channel_apt_deb(
> + "http://example.com/ubuntu", "lucid", None)
>
> It'd be nice to make the components parameter optional in
> add_channel_apt_deb. I understand that it's not optional in the
> SmartFacade, but it should be compatible.

Sure, I made it optional

> [4]
>
> + If deb URLs have been added, a list of dict is returned with
> + information about the channels.
>
> The second line is indented too much.

Fixed.

> [5]
>
> + self.facade.add_channel_apt_deb(
> + "http://enabled.example.com/ubuntu", "lucid", ["main"])
> + self.facade.add_channel_apt_deb(
> + "http://disabled.example.com/ubuntu", "lucid", ["main"])
> + sources_list = SourcesList()
> + for entry in sources_list:
> + if "disabled" in entry.uri:
> + entry.set_enabled(False)
> + sources_list.save()
>
> Doh, SourcesList() is nice and convenient, just too bad that it relies on global state! Anyway, good idea to use it, I didn't know or recall about it.
>
> [6]
>
> + def reset_channels(self):
> + """Remove all the configured channels."""
> + sources_list = SourcesList()
> + for entry in sources_list:
> + entry.set_enabled(False)
>
> The only use case for reset_channels() in the server code,
> canonical/landscape/scripts/hashids.py, where we update the lists of
> pre-canned hash->id maps. I think it's pretty safe to remove the entry
> entirely for that, like:
>
> for entry in sources_list:
> sources_list.remove(entry)
>
> which I think would fit more with the "reset" term. If we need a
> "disable" behavior we could add a disable_channels() method.

I agree. However, it doesn't quite work ;) I tried doing this, but for
some reason it doesn't remove the entries. So rather than spending time
on debuggin, I chose to simply disable them.

--
Björn Tillenius | https://launchpad.net/~bjornt

=== modified file 'landscape/lib/fs.py'
--- landscape/lib/fs.py	2010-04-01 08:28:22 +0000
+++ landscape/lib/fs.py	2011-09-29 12:39:24 +0000
@@ -14,6 +14,19 @@
     fd.close()
 
 
+def append_file(path, content):
+    """Append a file with the given content.
+
+    The file is created, if it doesn't exist already.
+
+    @param path: The path to the file.
+    @param content: The content to be written in the file at the end.
+    """
+    fd = open(path, "a")
+    fd.write(content)
+    fd.close()
+
+
 def read_file(path, limit=None):
     """Return the content of the given file.
 
 
=== modified file 'landscape/lib/tests/test_fs.py'
--- landscape/lib/tests/test_fs.py	2011-07-05 05:09:11 +0000
+++ landscape/lib/tests/test_fs.py	2011-09-29 12:39:24 +0000
@@ -1,6 +1,8 @@
+import os
+
 from landscape.tests.helpers import LandscapeTest
 
-from landscape.lib.fs import read_file, touch_file
+from landscape.lib.fs import append_file, read_file, touch_file
 
 
 class ReadFileTest(LandscapeTest):
@@ -59,3 +61,23 @@
         touch_file(path)
         touch_file(path)
         self.assertFileContent(path, "")
+
+
+class AppendFileTest(LandscapeTest):
+
+    def test_append_existing_file(self):
+        """
+        The L{append_file} function appends contents to an existing file.
+        """
+        existing_file = self.makeFile("foo bar")
+        append_file(existing_file, " baz")
+        self.assertFileContent(existing_file, "foo bar baz")
+
+    def test_append_no_file(self):
+        """
+        The L{append_file} function creates a new file if one doesn't
+        exist already.
+        """
+        new_file = os.path.join(self.makeDir(), "new_file")
+        append_file(new_file, "contents")
+        self.assertFileContent(new_file, "contents")
 
=== modified file 'landscape/package/facade.py'
--- landscape/package/facade.py	2011-09-26 12:20:46 +0000
+++ landscape/package/facade.py	2011-09-29 12:39:24 +0000
@@ -1,3 +1,5 @@
+import os
+
 from smart.transaction import (
     Transaction, PolicyInstall, PolicyUpgrade, PolicyRemove, Failed)
 from smart.const import INSTALL, REMOVE, UPGRADE, ALWAYS, NEVER
@@ -5,7 +7,10 @@
 import smart
 
 import apt
+import apt_pkg
+from aptsources.sourceslist import SourcesList
 
+from landscape.lib.fs import append_file
 from landscape.package.skeleton import build_skeleton
 
 
@@ -51,6 +56,43 @@
     def reload_channels(self):
         """Reload the channels and update the cache."""
         self._cache.open(None)
+        self._cache.update()
+        self._cache.open(None)
+
+    def add_channel_apt_deb(self, url, codename, components):
+        """Add a deb URL which points to a repository.
+
+        @param url: The base URL of the repository.
+        @param codename: The dist in the repository.
+        @param components: The components to be included.
+        """
+        sources_dir = apt_pkg.config.find_dir("Dir::Etc::sourceparts")
+        sources_file_path = os.path.join(
+            sources_dir, "_landscape-internal-facade.list")
+        sources_line = "deb %s %s" % (url, codename)
+        if components:
+            sources_line += " %s" % " ".join(components)
+        sources_line += "\n"
+        append_file(sources_file_path, sources_line)
+
+    def get_channels(self):
+        """Return a list of channels configured.
+
+        A channel is a deb line in sources.list or sources.list.d. It's
+        represented by a dict with baseurl, distribution, components,
+        and type keys.
+        """
+        sources_list = SourcesList()
+        return [{"baseurl": entry.uri, "distribution": entry.dist,
+                 "components": " ".join(entry.comps), "type": entry.type}
+                for entry in sources_list if not entry.disabled]
+
+    def reset_channels(self):
+        """Remove all the configured channels."""
+        sources_list = SourcesList()
+        for entry in sources_list:
+            entry.set_enabled(False)
+        sources_list.save()
 
 
 class SmartFacade(object):
 
=== modified file 'landscape/package/tests/helpers.py'
--- landscape/package/tests/helpers.py	2011-09-29 10:08:01 +0000
+++ landscape/package/tests/helpers.py	2011-09-29 12:39:24 +0000
@@ -16,6 +16,7 @@
         # auto-create them, which causing the paths to be printed to stdout.
         test_case.dpkg_dir = self._create_sub_dir(test_case, "var/lib/dpkg")
         self._create_sub_dir(test_case, "etc/apt")
+        self._create_sub_dir(test_case, "etc/apt/sources.list.d")
         self._create_sub_dir(test_case, "var/cache/apt/archives/partial")
         self._create_sub_dir(test_case, "var/lib/apt/lists/partial")
         test_case.dpkg_status = os.path.join(test_case.dpkg_dir, "status")
 
=== modified file 'landscape/package/tests/test_facade.py'
--- landscape/package/tests/test_facade.py	2011-09-29 08:20:24 +0000
+++ landscape/package/tests/test_facade.py	2011-09-29 12:39:24 +0000
@@ -8,12 +8,15 @@
 from smart.cache import Provides
 from smart.const import NEVER, ALWAYS
 
+from aptsources.sourceslist import SourcesList
+
 from twisted.internet import reactor
 from twisted.internet.defer import Deferred
 from twisted.internet.utils import getProcessOutputAndValue
 
 import smart
 
+from landscape.lib.fs import append_file, read_file
 from landscape.package.facade import (
     TransactionError, DependencyError, ChannelError, SmartError)
 
@@ -30,8 +33,7 @@
 
     def _add_system_package(self, name):
         """Add a package to the dpkg status file."""
-        with open(self.dpkg_status, "a") as status_file:
-            status_file.write(textwrap.dedent("""\
+        append_file(self.dpkg_status, textwrap.dedent("""\
                 Package: %s
                 Status: install ok installed
                 Priority: optional
@@ -46,10 +48,22 @@
 
                 """ % name))
 
+    def _add_package_to_deb_dir(self, path, name, version="1.0"):
+        """Add fake package information to a directory.
+
+        There will only be basic information about the package
+        available, so that get_packages() have something to return.
+        There won't be an actual package in the dir.
+        """
+        package_stanza = "Package: %(name)s\nVersion: %(version)s\n\n"
+        append_file(
+            os.path.join(path, "Packages"),
+            package_stanza % {"name": name, "version": version})
+
     def test_no_system_packages(self):
         """
         If the dpkg status file is empty, not packages are reported by
-        get_packages().
+        C{get_packages()}.
         """
         self.facade.reload_channels()
         self.assertEqual([], self.facade.get_packages())
@@ -57,7 +71,7 @@
     def test_get_system_packages(self):
         """
         If the dpkg status file contains some packages, those packages
-        are reported by get_packages().
+        are reported by C{get_packages()}.
         """
         self._add_system_package("foo")
         self._add_system_package("bar")
@@ -66,6 +80,103 @@
             ["bar", "foo"],
             sorted(package.name for package in self.facade.get_packages()))
 
+    def test_add_channel_apt_deb_without_components(self):
+        """
+        C{add_channel_apt_deb()} adds a new deb URL to a file in
+        sources.list.d.
+
+        If no components are given, nothing is written after the dist.
+        """
+        self.facade.add_channel_apt_deb(
+            "http://example.com/ubuntu", "lucid", None)
+        list_filename = (
+            self.apt_root +
+            "/etc/apt/sources.list.d/_landscape-internal-facade.list")
+        sources_contents = read_file(list_filename)
+        self.assertEqual(
+            "deb http://example.com/ubuntu lucid\n",
+            sources_contents)
+
+    def test_add_channel_apt_deb_with_components(self):
+        """
+        C{add_channel_apt_deb()} adds a new deb URL to a file in
+        sources.list.d.
+
+        If components are given, they are included after the dist.
+        """
+        self.facade.add_channel_apt_deb(
+            "http://example.com/ubuntu", "lucid", ["main", "restricted"])
+        list_filename = (
+            self.apt_root +
+            "/etc/apt/sources.list.d/_landscape-internal-facade.list")
+        sources_contents = read_file(list_filename)
+        self.assertEqual(
+            "deb http://example.com/ubuntu lucid main restricted\n",
+            sources_contents)
+
+    def test_get_channels_with_no_channels(self):
+        """
+        If no deb URLs have been added, C{get_channels()} returns an empty list.
+        """
+        self.assertEqual([], self.facade.get_channels())
+
+    def test_get_channels_with_channels(self):
+        """
+        If deb URLs have been added, a list of dict is returned with
+            information about the channels.
+        """
+        self.facade.add_channel_apt_deb(
+            "http://example.com/ubuntu", "lucid", ["main", "restricted"])
+        self.assertEqual([{"baseurl": "http://example.com/ubuntu",
+                           "distribution": "lucid",
+                           "components": "main restricted",
+                           "type": "deb"}],
+                         self.facade.get_channels())
+
+    def test_get_channels_with_disabled_channels(self):
+        """
+        C{get_channels()} doesn't return disabled deb URLs.
+        """
+        self.facade.add_channel_apt_deb(
+            "http://enabled.example.com/ubuntu", "lucid", ["main"])
+        self.facade.add_channel_apt_deb(
+            "http://disabled.example.com/ubuntu", "lucid", ["main"])
+        sources_list = SourcesList()
+        for entry in sources_list:
+            if "disabled" in entry.uri:
+                entry.set_enabled(False)
+        sources_list.save()
+        self.assertEqual([{"baseurl": "http://enabled.example.com/ubuntu",
+                           "distribution": "lucid",
+                           "components": "main",
+                           "type": "deb"}],
+                         self.facade.get_channels())
+
+    def test_reset_channels(self):
+        """
+        C{reset_channels()} disables all the configured deb URLs.
+        """
+        self.facade.add_channel_apt_deb(
+            "http://1.example.com/ubuntu", "lucid", ["main", "restricted"])
+        self.facade.add_channel_apt_deb(
+            "http://2.example.com/ubuntu", "lucid", ["main", "restricted"])
+        self.facade.reset_channels()
+        self.assertEqual([], self.facade.get_channels())
+
+    def test_reload_includes_added_channels(self):
+        """
+        When reloading the channels, C{get_packages()} returns the packages
+        in the channel.
+        """
+        deb_dir = self.makeDir()
+        self._add_package_to_deb_dir(deb_dir, "foo")
+        self._add_package_to_deb_dir(deb_dir, "bar")
+        self.facade.add_channel_apt_deb("file://%s" % deb_dir, "./", None)
+        self.facade.reload_channels()
+        self.assertEqual(
+            ["bar", "foo"],
+            sorted(package.name for package in self.facade.get_packages()))
+
 
 class SmartFacadeTest(LandscapeTest):