Merge lp://staging/~tealeg/landscape-client/scoped_session_ids into lp://staging/~landscape/landscape-client/trunk

Nice work in keeping these branches small, it really helps. +1!

#1

         # When re-synchronisation occurs we don't want any previous messages
         # being sent to the server, dropping the existing session_ids means
         # that messages sent with those IDs will be dropped by the broker.
- self._message_store.drop_session_ids()
+ self._message_store.drop_session_ids(scopes)

Please expand the comment mentioning that only the session IDs for the specified scopes will be dropped (or all of them if scopes is None).

#2

- def get_session_id(self):
+ def get_session_id(self, scope=None):

There should be a @param entry for the new parameter, in the method docstring.

#3

- def drop_session_ids(self, scope=None):
+ def drop_session_ids(self, scopes=None):
         """
         Drop all session ids.
         """

Please update the method docstring and document the new parameter.

#4

+ test_session_id = self.successResultOf(
+ self.client.broker.get_session_id(scope="test"))

Opinionated nitpick: since this is using 2 lines anyways, I find it generally better to split it like this:

        deferred = self.client.broker.get_session_id(scope="test")
        test_session_id = self.successResultOf(deferred)

because imo there's less parsing work going on in one's mind when reading the code. This is really minor (and I guess a counter argument could be made), so your call.

#5

+ global_scope = []
+ self.reactor.fire("resynchronize-clients", global_scope)

This is probably a leftover, you can just:

        self.reactor.fire("resynchronize-clients")

which defaults to the global scope.

#6

+ self.assertMessages(messages, [{"type": "empty"}])

This doesn't test which one of the 2 sessions IDs generated the message exactly, it'd be nice to use 2 different message types I guess. Maybe it's not needed from a strict unit-test point of view, since I believe test_drop_multiple_scopes from test_store.py covers that. Your call.

I was going to add comments similar to free's #1 , #2 and #3, but he did already.

# WARNING #
This branch will break trunk pretty badly until all other plugin branches have landed. This branch must therefore land *last*, since the plugins branches have a backwards-compatible behavior (they treat resynchronize events as global events by default).

I will disapprove this branch until all plugin branches have landed, and test pass again.

I will retract my previous statement - this *can* land first, with the following fix: https://pastebin.canonical.com/94185/

So, +1, and sorry for the drama :)

> I will retract my previous statement - this *can* land first, with the
> following fix: https://pastebin.canonical.com/94185/
>
> So, +1, and sorry for the drama :)

No trouble, I love a drama queen, me :-)

Done.

No proposals found for merge of https://code.launchpad.net/~tealeg/landscape-client/broker_handles_scoped_resynch into https://code.launchpad.net/~landscape/landscape-client/trunk.