Code review comment for lp:~jseutter/landscape-client/autodiscover-config

Looks good, only a couple of points about style. +1!

[1]

+def discover_server(resolver=None, autodiscover_srv_query_string="",
+ autodiscover_a_query_string=""):

and

+ lookup_deferred = discover_server(
+ None, self._autodiscover_srv_query_string,
+ self._autodiscover_a_query_string)

It feels a bit weird to pass None as first argument, the casual reader is probably going to wonder what that means, especially considering that this is the only use of discover_server() in non-test code.

If the resolver argument is used for testing only, please consider moving it at the end of the arguments list:

def discover_server(autodiscover_srv_query_string="",
                    autodiscover_a_query_string="", resolver=None):

and use it like:

+ lookup_deferred = discover_server(
+ self._autodiscover_srv_query_string,
+ self._autodiscover_a_query_string)

[2]

Not really related to this branch, but while I was reading some context, I've noticed that the ping method has grown a bit:

    def ping(self):
        """Ask the question.

        Hits the URL previously specified with the insecure_id gotten
        from the identity.

        @return: A deferred resulting in True if there are messages
            and False otherwise.
        """
        def handle_result(result):
            if result is None:
                logging.warning("Autodiscovery failed. Reverting to previous "
                                "settings.")
            else:
                result = "http://%s/ping" % result
            return result

        def do_rest(result):
            if result is not None:
                self.url = result

            insecure_id = self._identity.insecure_id
            if insecure_id is not None:
                headers = {"Content-Type": "application/x-www-form-urlencoded"}
                data = urllib.urlencode({"insecure_id": insecure_id})
                page_deferred = defer.Deferred()

                def errback(type, value, tb):
                    page_deferred.errback(Failure(value, type, tb))
                self._reactor.call_in_thread(page_deferred.callback, errback,
                                             self.get_page, self.url,
                                             post=True, data=data,
                                             headers=headers)
                page_deferred.addCallback(self._got_result)
                return page_deferred
            return defer.succeed(False)

        if self._server_autodiscover:
            lookup_deferred = discover_server(
                None, self._autodiscover_srv_query_string,
                self._autodiscover_a_query_string)
            lookup_deferred.addCallback(handle_result)
            lookup_deferred.addCallback(do_rest)
            return lookup_deferred
        else:
            return do_rest(None)

What do you think of splitting it in methods like:

    def ping(self):
        """Ask the question.

        Hits the URL previously specified with the insecure_id gotten
        from the identity.

        @return: A deferred resulting in True if there are messages
            and False otherwise.
        """
        if self._server_autodiscover:
            deferred = discover_server(
                None, self._autodiscover_srv_query_string,
                self._autodiscover_a_query_string)
            deferred.addCallback(self._discover_server_success)
        else:
            deferred = defer.succeed(None)
        deferred.addCallback(self._do_ping)
        return deferred

    def _discover_server_success(self, result):
        if result is None:
            logging.warning("Autodiscovery failed. Reverting to previous "
                            "settings.")
        else:
            result = "http://%s/ping" % result
        return result

    def _do_ping(self, url):
        if url is not None:
            self.url = url

        insecure_id = self._identity.insecure_id
        if insecure_id is not None:
            headers = {"Content-Type": "application/x-www-form-urlencoded"}
            data = urllib.urlencode({"insecure_id": insecure_id})
            page_deferred = defer.Deferred()

            def errback(type, value, tb):
                page_deferred.errback(Failure(value, type, tb))
            self._reactor.call_in_thread(page_deferred.callback, errback,
                                         self.get_page, self.url,
                                         post=True, data=data,
                                         headers=headers)
            page_deferred.addCallback(self._got_result)
            return page_deferred
        return defer.succeed(False)

Note that also renamed do_rest to do_ping, as it's not super clear what "rest" is.