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

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