Code review comment for lp:~bjornt/landscape-client/apt-channel-api

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.