Merge lp:~therve/landscape-client/duplicate-sources into lp:~landscape/landscape-client/trunk

[Thomas Herve]

It's almost a trivial, but I wanted at least a second look.

[Free Ekanayaka (free.ekanayaka)]

As mentioned on IRC, I believe we should not leave the _landscape-internal-facade.list file around when our job is done. Fixing that should make this one unnecessary.

review: Needs Information

[Geoff Teale (tealeg)]

Not withstanding Free's comment above, the change itself looks good to me.

review: Approve

[Free Ekanayaka (free.ekanayaka)]

Okay, the thing I don't like is that it introduces logic that we wouldn't strictly need if we fixed the unused _landscape-internal-facade.list (which imo we should do, because admins can be picky about conffiles), so people reading that code in the future might wonder why it's there. Said that, +1!

review: Approve

=== modified file 'landscape/package/facade.py'
--- landscape/package/facade.py	2012-03-06 15:59:11 +0000
+++ landscape/package/facade.py	2012-03-12 16:03:19 +0000
@@ -252,6 +252,10 @@
         sources_line = "deb %s %s" % (url, codename)
         if components:
             sources_line += " %s" % " ".join(components)
+        if os.path.exists(sources_file_path):
+            current_content = read_file(sources_file_path).split("\n")
+            if sources_line in current_content:
+                return
         sources_line += "\n"
         append_file(sources_file_path, sources_line)
 

=== modified file 'landscape/package/tests/test_facade.py'
--- landscape/package/tests/test_facade.py	2012-03-06 15:59:11 +0000
+++ landscape/package/tests/test_facade.py	2012-03-12 16:03:19 +0000
@@ -170,8 +170,23 @@
 
         If no components are given, nothing is written after the dist.
         """
-        self.facade.add_channel_apt_deb(
-            "http://example.com/ubuntu", "lucid")
+        self.facade.add_channel_apt_deb("http://example.com/ubuntu", "lucid")
+        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_no_duplicate(self):
+        """
+        C{add_channel_apt_deb} doesn't put duplicate lines in the landscape
+        internal apt sources list.
+        """
+        self.facade.add_channel_apt_deb("http://example.com/ubuntu", "lucid")
+        self.facade.add_channel_apt_deb("http://example.com/ubuntu", "lucid")
+        self.facade.add_channel_apt_deb("http://example.com/ubuntu", "lucid")
         list_filename = (
             self.apt_root +
             "/etc/apt/sources.list.d/_landscape-internal-facade.list")