Code review comment for lp:~therve/landscape-client/sources-list-plugin

Nice work, nothing really blocking so feel free to address what you think is worth. +1

[1]

+ deferred.addCallback(self._handle_process_error)
+ deferred.addCallback(lambda ignore, path=path: os.unlink(path))

Probably not I huge deal, but I suspect that the key file is not removed if the process fails?

[2]

+ if not line.strip() or line.startswith("#"):

I think comments can start also after some blanks, so maybe:

+ if not line.strip() or line.strip().startswith("#"):

[3]

+class SourcesList(ManagerPlugin):

and

+ "repositories", self._wrap_handle_repositories)

It would be nice for the plugin and message name to match a bit more (that's often the cases for other plugins/messages). I'd suggest "class AptSources" and "apt-sources-replace" or something. The name "reposiories" sounds a bit generic, in case we want to add other operations later. This not blocking though.

[4]

+class ProcessError(Exception):
+ """Exception raised when running a process fails."""

and

+ def run_process(self, command, args):

This seems generic enough that it could be made a function under landscape.lib.process, like:

class ProcessError(Exception):
    """Exception raised when running a process fails."""

    def __init__(self, out, err):
        super(ProcessError, self).__init__("%s\n%s" % (out, err))

def run_process(self, command, args):
    """Run the process in an asynchronous fashion."""

    def handle_process_error(result):
        """
        Turn a failed process command (code != 0) to a C{ProcessError}.
        """
        out, err, code = result
        if code:
            raise ProcessError(out, err)
        else:
            return result

    def handle_process_failure(failure):
        """
        Turn a signaled process command to a C{ProcessError}.
        """
        out, err, signal = failure.value
        raise ProcessError(out, err)

    deferred = getProcessOutputAndValue(command, args)
    deferred.addCallback(handle_process_error)
    deferred.addErrback(handle_process_failure)
    return deferred

You could test run_process separately, and have the tests for SourcesList simply shows that ProcessError failures are handled correctly:

    def test_failed_import_no_changes(self):
        """
        If the C{apt-key} command failed for some reasons, the current
        repositories aren't changed.
        """

        def run_process(command, args):
            return fail(ProcessError("nok", "some error"))

        self.sourceslist.run_process = run_process

        sources = file(self.sourceslist.SOURCES_LIST, "w")
        sources.write("oki\n\ndoki\n#comment\n")
        sources.close()

        self.manager.dispatch_message(
            {"type": "repositories", "sources": [], "gpg-keys": ["key"],
             "operation-id": 1})

        self.assertEqual(
            "oki\n\ndoki\n#comment\n",
            file(self.sourceslist.SOURCES_LIST).read())

        service = self.broker_service
        self.assertMessages(service.message_store.get_pending_messages(),
                            [{"type": "operation-result",
                              'result-text': u"ProcessError: nok\nsome error",
                              "status": FAILED, "operation-id": 1}])