Merge lp:~bjornt/landscape-client/apt-channel-api into lp:~landscape/landscape-client/trunk
- apt-channel-api
- Merge into trunk
Status: | Merged | ||||
---|---|---|---|---|---|
Approved by: | Free Ekanayaka | ||||
Approved revision: | 384 | ||||
Merged at revision: | 366 | ||||
Proposed branch: | lp:~bjornt/landscape-client/apt-channel-api | ||||
Merge into: | lp:~landscape/landscape-client/trunk | ||||
Diff against target: |
304 lines (+194/-5) 5 files modified
landscape/lib/fs.py (+13/-0) landscape/lib/tests/test_fs.py (+23/-1) landscape/package/facade.py (+42/-0) landscape/package/tests/helpers.py (+1/-0) landscape/package/tests/test_facade.py (+115/-4) |
||||
To merge this branch: | bzr merge lp:~bjornt/landscape-client/apt-channel-api | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Free Ekanayaka (community) | Approve | ||
Alberto Donato (community) | Approve | ||
Review via email: mp+77313@code.launchpad.net |
Commit message
Description of the change
Add support for adding and removing deb URLs in AptFacade.
add_channel_
add_channel() was omitted, since it's only used by SmartFacade
internally.
I filed bug 861345 for implementing add_channel_
to do some extra work to make sure apt can use the dir.
Free Ekanayaka (free.ekanayaka) wrote : | # |
Nice work, +1!
[1]
+ sources_file_path = sources_dir + "/landscape-
Please use os.path.join().
Also, wdyt of prefixing the file with an underscore to signal it's a "private" file? like "_landscape-
[2]
+ add_channel_
Please write it with epydoc syntax, "C{add_
[3]
+ self.facade.
+ "http://
It'd be nice to make the components parameter optional in add_channel_
[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.
+ "http://
+ self.facade.
+ "http://
+ sources_list = SourcesList()
+ for entry in sources_list:
+ if "disabled" in entry.uri:
+ entry.set_
+ 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(
+ """Remove all the configured channels."""
+ sources_list = SourcesList()
+ for entry in sources_list:
+ entry.set_
The only use case for reset_channels() in the server code, canonical/
for entry in sources_list:
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.
- 385. By Björn Tillenius
-
Merge trunk.
- 386. By Björn Tillenius
-
Use os.path.join.
- 387. By Björn Tillenius
-
Use read_file().
- 388. By Björn Tillenius
-
Add append_file() and use it to get rid of with statements.
- 389. By Björn Tillenius
-
Rename .list file to make it more obvious that it's private.
- 390. By Björn Tillenius
-
Epydoc syntax.
Björn Tillenius (bjornt) wrote : | # |
On Wed, Sep 28, 2011 at 12:56:28PM -0000, Alberto Donato wrote:
> Review: Approve
>
> Nice, +1!
>
> #1:
> + sources_file_path = sources_dir + "/landscape-
>
> #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:/
Björn Tillenius (bjornt) wrote : | # |
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-
>
> Please use os.path.join().
Sure.
> Also, wdyt of prefixing the file with an underscore to signal it's a
> "private" file? like "_landscape-
> using a dot as prefix won't work because libapt ignores it).
Yes, that's a good idea.
> [2]
>
> + add_channel_
>
> Please write it with epydoc syntax, "C{add_
> the other occurrences.
Done.
> [3]
>
> + self.facade.
> + "http://
>
> It'd be nice to make the components parameter optional in
> add_channel_
> 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.
> + "http://
> + self.facade.
> + "http://
> + sources_list = SourcesList()
> + for entry in sources_list:
> + if "disabled" in entry.uri:
> + entry.set_
> + 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(
> + """Remove all the configured channels."""
> + sources_list = SourcesList()
> + for entry in sources_list:
> + entry.set_
>
> The only use case for reset_channels() in the server code,
> canonical/
> 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_
>
> 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:/
Preview Diff
1 | === modified file 'landscape/lib/fs.py' |
2 | --- landscape/lib/fs.py 2010-04-01 08:28:22 +0000 |
3 | +++ landscape/lib/fs.py 2011-09-29 12:39:24 +0000 |
4 | @@ -14,6 +14,19 @@ |
5 | fd.close() |
6 | |
7 | |
8 | +def append_file(path, content): |
9 | + """Append a file with the given content. |
10 | + |
11 | + The file is created, if it doesn't exist already. |
12 | + |
13 | + @param path: The path to the file. |
14 | + @param content: The content to be written in the file at the end. |
15 | + """ |
16 | + fd = open(path, "a") |
17 | + fd.write(content) |
18 | + fd.close() |
19 | + |
20 | + |
21 | def read_file(path, limit=None): |
22 | """Return the content of the given file. |
23 | |
24 | |
25 | === modified file 'landscape/lib/tests/test_fs.py' |
26 | --- landscape/lib/tests/test_fs.py 2011-07-05 05:09:11 +0000 |
27 | +++ landscape/lib/tests/test_fs.py 2011-09-29 12:39:24 +0000 |
28 | @@ -1,6 +1,8 @@ |
29 | +import os |
30 | + |
31 | from landscape.tests.helpers import LandscapeTest |
32 | |
33 | -from landscape.lib.fs import read_file, touch_file |
34 | +from landscape.lib.fs import append_file, read_file, touch_file |
35 | |
36 | |
37 | class ReadFileTest(LandscapeTest): |
38 | @@ -59,3 +61,23 @@ |
39 | touch_file(path) |
40 | touch_file(path) |
41 | self.assertFileContent(path, "") |
42 | + |
43 | + |
44 | +class AppendFileTest(LandscapeTest): |
45 | + |
46 | + def test_append_existing_file(self): |
47 | + """ |
48 | + The L{append_file} function appends contents to an existing file. |
49 | + """ |
50 | + existing_file = self.makeFile("foo bar") |
51 | + append_file(existing_file, " baz") |
52 | + self.assertFileContent(existing_file, "foo bar baz") |
53 | + |
54 | + def test_append_no_file(self): |
55 | + """ |
56 | + The L{append_file} function creates a new file if one doesn't |
57 | + exist already. |
58 | + """ |
59 | + new_file = os.path.join(self.makeDir(), "new_file") |
60 | + append_file(new_file, "contents") |
61 | + self.assertFileContent(new_file, "contents") |
62 | |
63 | === modified file 'landscape/package/facade.py' |
64 | --- landscape/package/facade.py 2011-09-26 12:20:46 +0000 |
65 | +++ landscape/package/facade.py 2011-09-29 12:39:24 +0000 |
66 | @@ -1,3 +1,5 @@ |
67 | +import os |
68 | + |
69 | from smart.transaction import ( |
70 | Transaction, PolicyInstall, PolicyUpgrade, PolicyRemove, Failed) |
71 | from smart.const import INSTALL, REMOVE, UPGRADE, ALWAYS, NEVER |
72 | @@ -5,7 +7,10 @@ |
73 | import smart |
74 | |
75 | import apt |
76 | +import apt_pkg |
77 | +from aptsources.sourceslist import SourcesList |
78 | |
79 | +from landscape.lib.fs import append_file |
80 | from landscape.package.skeleton import build_skeleton |
81 | |
82 | |
83 | @@ -51,6 +56,43 @@ |
84 | def reload_channels(self): |
85 | """Reload the channels and update the cache.""" |
86 | self._cache.open(None) |
87 | + self._cache.update() |
88 | + self._cache.open(None) |
89 | + |
90 | + def add_channel_apt_deb(self, url, codename, components): |
91 | + """Add a deb URL which points to a repository. |
92 | + |
93 | + @param url: The base URL of the repository. |
94 | + @param codename: The dist in the repository. |
95 | + @param components: The components to be included. |
96 | + """ |
97 | + sources_dir = apt_pkg.config.find_dir("Dir::Etc::sourceparts") |
98 | + sources_file_path = os.path.join( |
99 | + sources_dir, "_landscape-internal-facade.list") |
100 | + sources_line = "deb %s %s" % (url, codename) |
101 | + if components: |
102 | + sources_line += " %s" % " ".join(components) |
103 | + sources_line += "\n" |
104 | + append_file(sources_file_path, sources_line) |
105 | + |
106 | + def get_channels(self): |
107 | + """Return a list of channels configured. |
108 | + |
109 | + A channel is a deb line in sources.list or sources.list.d. It's |
110 | + represented by a dict with baseurl, distribution, components, |
111 | + and type keys. |
112 | + """ |
113 | + sources_list = SourcesList() |
114 | + return [{"baseurl": entry.uri, "distribution": entry.dist, |
115 | + "components": " ".join(entry.comps), "type": entry.type} |
116 | + for entry in sources_list if not entry.disabled] |
117 | + |
118 | + def reset_channels(self): |
119 | + """Remove all the configured channels.""" |
120 | + sources_list = SourcesList() |
121 | + for entry in sources_list: |
122 | + entry.set_enabled(False) |
123 | + sources_list.save() |
124 | |
125 | |
126 | class SmartFacade(object): |
127 | |
128 | === modified file 'landscape/package/tests/helpers.py' |
129 | --- landscape/package/tests/helpers.py 2011-09-29 10:08:01 +0000 |
130 | +++ landscape/package/tests/helpers.py 2011-09-29 12:39:24 +0000 |
131 | @@ -16,6 +16,7 @@ |
132 | # auto-create them, which causing the paths to be printed to stdout. |
133 | test_case.dpkg_dir = self._create_sub_dir(test_case, "var/lib/dpkg") |
134 | self._create_sub_dir(test_case, "etc/apt") |
135 | + self._create_sub_dir(test_case, "etc/apt/sources.list.d") |
136 | self._create_sub_dir(test_case, "var/cache/apt/archives/partial") |
137 | self._create_sub_dir(test_case, "var/lib/apt/lists/partial") |
138 | test_case.dpkg_status = os.path.join(test_case.dpkg_dir, "status") |
139 | |
140 | === modified file 'landscape/package/tests/test_facade.py' |
141 | --- landscape/package/tests/test_facade.py 2011-09-29 08:20:24 +0000 |
142 | +++ landscape/package/tests/test_facade.py 2011-09-29 12:39:24 +0000 |
143 | @@ -8,12 +8,15 @@ |
144 | from smart.cache import Provides |
145 | from smart.const import NEVER, ALWAYS |
146 | |
147 | +from aptsources.sourceslist import SourcesList |
148 | + |
149 | from twisted.internet import reactor |
150 | from twisted.internet.defer import Deferred |
151 | from twisted.internet.utils import getProcessOutputAndValue |
152 | |
153 | import smart |
154 | |
155 | +from landscape.lib.fs import append_file, read_file |
156 | from landscape.package.facade import ( |
157 | TransactionError, DependencyError, ChannelError, SmartError) |
158 | |
159 | @@ -30,8 +33,7 @@ |
160 | |
161 | def _add_system_package(self, name): |
162 | """Add a package to the dpkg status file.""" |
163 | - with open(self.dpkg_status, "a") as status_file: |
164 | - status_file.write(textwrap.dedent("""\ |
165 | + append_file(self.dpkg_status, textwrap.dedent("""\ |
166 | Package: %s |
167 | Status: install ok installed |
168 | Priority: optional |
169 | @@ -46,10 +48,22 @@ |
170 | |
171 | """ % name)) |
172 | |
173 | + def _add_package_to_deb_dir(self, path, name, version="1.0"): |
174 | + """Add fake package information to a directory. |
175 | + |
176 | + There will only be basic information about the package |
177 | + available, so that get_packages() have something to return. |
178 | + There won't be an actual package in the dir. |
179 | + """ |
180 | + package_stanza = "Package: %(name)s\nVersion: %(version)s\n\n" |
181 | + append_file( |
182 | + os.path.join(path, "Packages"), |
183 | + package_stanza % {"name": name, "version": version}) |
184 | + |
185 | def test_no_system_packages(self): |
186 | """ |
187 | If the dpkg status file is empty, not packages are reported by |
188 | - get_packages(). |
189 | + C{get_packages()}. |
190 | """ |
191 | self.facade.reload_channels() |
192 | self.assertEqual([], self.facade.get_packages()) |
193 | @@ -57,7 +71,7 @@ |
194 | def test_get_system_packages(self): |
195 | """ |
196 | If the dpkg status file contains some packages, those packages |
197 | - are reported by get_packages(). |
198 | + are reported by C{get_packages()}. |
199 | """ |
200 | self._add_system_package("foo") |
201 | self._add_system_package("bar") |
202 | @@ -66,6 +80,103 @@ |
203 | ["bar", "foo"], |
204 | sorted(package.name for package in self.facade.get_packages())) |
205 | |
206 | + def test_add_channel_apt_deb_without_components(self): |
207 | + """ |
208 | + C{add_channel_apt_deb()} adds a new deb URL to a file in |
209 | + sources.list.d. |
210 | + |
211 | + If no components are given, nothing is written after the dist. |
212 | + """ |
213 | + self.facade.add_channel_apt_deb( |
214 | + "http://example.com/ubuntu", "lucid", None) |
215 | + list_filename = ( |
216 | + self.apt_root + |
217 | + "/etc/apt/sources.list.d/_landscape-internal-facade.list") |
218 | + sources_contents = read_file(list_filename) |
219 | + self.assertEqual( |
220 | + "deb http://example.com/ubuntu lucid\n", |
221 | + sources_contents) |
222 | + |
223 | + def test_add_channel_apt_deb_with_components(self): |
224 | + """ |
225 | + C{add_channel_apt_deb()} adds a new deb URL to a file in |
226 | + sources.list.d. |
227 | + |
228 | + If components are given, they are included after the dist. |
229 | + """ |
230 | + self.facade.add_channel_apt_deb( |
231 | + "http://example.com/ubuntu", "lucid", ["main", "restricted"]) |
232 | + list_filename = ( |
233 | + self.apt_root + |
234 | + "/etc/apt/sources.list.d/_landscape-internal-facade.list") |
235 | + sources_contents = read_file(list_filename) |
236 | + self.assertEqual( |
237 | + "deb http://example.com/ubuntu lucid main restricted\n", |
238 | + sources_contents) |
239 | + |
240 | + def test_get_channels_with_no_channels(self): |
241 | + """ |
242 | + If no deb URLs have been added, C{get_channels()} returns an empty list. |
243 | + """ |
244 | + self.assertEqual([], self.facade.get_channels()) |
245 | + |
246 | + def test_get_channels_with_channels(self): |
247 | + """ |
248 | + If deb URLs have been added, a list of dict is returned with |
249 | + information about the channels. |
250 | + """ |
251 | + self.facade.add_channel_apt_deb( |
252 | + "http://example.com/ubuntu", "lucid", ["main", "restricted"]) |
253 | + self.assertEqual([{"baseurl": "http://example.com/ubuntu", |
254 | + "distribution": "lucid", |
255 | + "components": "main restricted", |
256 | + "type": "deb"}], |
257 | + self.facade.get_channels()) |
258 | + |
259 | + def test_get_channels_with_disabled_channels(self): |
260 | + """ |
261 | + C{get_channels()} doesn't return disabled deb URLs. |
262 | + """ |
263 | + self.facade.add_channel_apt_deb( |
264 | + "http://enabled.example.com/ubuntu", "lucid", ["main"]) |
265 | + self.facade.add_channel_apt_deb( |
266 | + "http://disabled.example.com/ubuntu", "lucid", ["main"]) |
267 | + sources_list = SourcesList() |
268 | + for entry in sources_list: |
269 | + if "disabled" in entry.uri: |
270 | + entry.set_enabled(False) |
271 | + sources_list.save() |
272 | + self.assertEqual([{"baseurl": "http://enabled.example.com/ubuntu", |
273 | + "distribution": "lucid", |
274 | + "components": "main", |
275 | + "type": "deb"}], |
276 | + self.facade.get_channels()) |
277 | + |
278 | + def test_reset_channels(self): |
279 | + """ |
280 | + C{reset_channels()} disables all the configured deb URLs. |
281 | + """ |
282 | + self.facade.add_channel_apt_deb( |
283 | + "http://1.example.com/ubuntu", "lucid", ["main", "restricted"]) |
284 | + self.facade.add_channel_apt_deb( |
285 | + "http://2.example.com/ubuntu", "lucid", ["main", "restricted"]) |
286 | + self.facade.reset_channels() |
287 | + self.assertEqual([], self.facade.get_channels()) |
288 | + |
289 | + def test_reload_includes_added_channels(self): |
290 | + """ |
291 | + When reloading the channels, C{get_packages()} returns the packages |
292 | + in the channel. |
293 | + """ |
294 | + deb_dir = self.makeDir() |
295 | + self._add_package_to_deb_dir(deb_dir, "foo") |
296 | + self._add_package_to_deb_dir(deb_dir, "bar") |
297 | + self.facade.add_channel_apt_deb("file://%s" % deb_dir, "./", None) |
298 | + self.facade.reload_channels() |
299 | + self.assertEqual( |
300 | + ["bar", "foo"], |
301 | + sorted(package.name for package in self.facade.get_packages())) |
302 | + |
303 | |
304 | class SmartFacadeTest(LandscapeTest): |
305 |
Nice, +1!
#1: internal- facade. list"
+ sources_file_path = sources_dir + "/landscape-
#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)