Landscape Client

Merge lp://staging/~tealeg/landscape-client/package-monitor-scoped-resynch into lp://staging/~landscape/landscape-client/trunk

package-monitor-scoped-resynch
Merge into trunk

Proposed by Geoff Teale on 2013-07-16

Status:

Merged

Approved by:

Geoff Teale on 2013-07-25

Approved revision:

723

Merged at revision:

713

Proposed branch:

lp://staging/~tealeg/landscape-client/package-monitor-scoped-resynch

Merge into:

lp://staging/~landscape/landscape-client/trunk

Diff against target:

178 lines (+76/-56)

3 files modified

landscape/monitor/packagemonitor.py (+1/-0)
landscape/monitor/tests/test_packagemonitor.py (+75/-15)
landscape/monitor/tests/test_usermonitor.py (+0/-41)

To merge this branch:

bzr merge lp://staging/~tealeg/landscape-client/package-monitor-scoped-resynch

Medium

Fix Released

Link a bug report

Reviewer	Date Requested	Status
Björn Tillenius (community)	2013-07-16	Approve on 2013-07-25
Jerry Seutter (community)	2013-07-16	Approve on 2013-07-16
Review via email: mp+175014@code.staging.launchpad.net

This proposal supersedes a proposal from 2013-07-12.

Commit message

This branch makes the PackageMonitor plugin only respond to resynchronize-clients events when they are either "package" or global scope.

Description of the change

This branch makes the PackageMonitor plugin only respond to resynchronize-clients events when they are either "package" or global scope.

Revision history for this message

Jerry Seutter (jseutter) wrote on 2013-07-09: Posted in a previous version of this proposal

+1, looks good.

landscape/monitor/tests/test_packagemonitor.py:233:80: E501 line too long (80 characters)

review: Approve

Revision history for this message

Jerry Seutter (jseutter) wrote on 2013-07-09: Posted in a previous version of this proposal

In other branches the call signature for _resynchronize uses scopes:
def _resynchronize(self, scopes):

Can you update this changeset to use scopes instead of scope?

Revision history for this message

Geoff Teale (tealeg) wrote on 2013-07-10: Posted in a previous version of this proposal

> +1, looks good.
>
> landscape/monitor/tests/test_packagemonitor.py:233:80: E501 line too long (80
> characters)

Done.

Revision history for this message

Geoff Teale (tealeg) wrote on 2013-07-10: Posted in a previous version of this proposal

> In other branches the call signature for _resynchronize uses scopes:
> def _resynchronize(self, scopes):
>
> Can you update this changeset to use scopes instead of scope?

Done.

Revision history for this message

Björn Tillenius (bjornt) wrote on 2013-07-10: Posted in a previous version of this proposal

[1]

+ if len(scopes) == 0 or self.scope in scopes:

This makes me a bit sad. Having this in every _resynchronize() doesn't
doesn't look good.

I see that we register the resynchronize method with something like
this:

self.registry.reactor.call_on("resynchronize", self._resynchronize)

I'd suggest doing something like this instead:

self.registry.register_resynchronize(self.scope, self._resynchronize)

That way self._resynchronize would be called only for the relevant
scope, which makes the method easier to read.

review: Needs Fixing

Revision history for this message

Geoff Teale (tealeg) wrote on 2013-07-10: Posted in a previous version of this proposal

> [1]
>
> + if len(scopes) == 0 or self.scope in scopes:
>
> This makes me a bit sad. Having this in every _resynchronize() doesn't
> doesn't look good.
>
> I see that we register the resynchronize method with something like
> this:
>
> self.registry.reactor.call_on("resynchronize", self._resynchronize)
>
> I'd suggest doing something like this instead:
>
> self.registry.register_resynchronize(self.scope, self._resynchronize)
>
> That way self._resynchronize would be called only for the relevant
> scope, which makes the method easier to read.

Hi Bjorn, would you be happy for me to make that change as a follow on branch? It effects multiple branches here and it would be cleaner to tidy it all up in one swoop.

Revision history for this message

Björn Tillenius (bjornt) wrote on 2013-07-11: Posted in a previous version of this proposal

On Wed, Jul 10, 2013 at 05:37:53PM -0000, Geoff Teale wrote:
> > [1]
> >
> > + if len(scopes) == 0 or self.scope in scopes:
> >
> > This makes me a bit sad. Having this in every _resynchronize() doesn't
> > doesn't look good.
> >
> > I see that we register the resynchronize method with something like
> > this:
> >
> > self.registry.reactor.call_on("resynchronize", self._resynchronize)
> >
> > I'd suggest doing something like this instead:
> >
> > self.registry.register_resynchronize(self.scope, self._resynchronize)
> >
> > That way self._resynchronize would be called only for the relevant
> > scope, which makes the method easier to read.
>
> Hi Bjorn, would you be happy for me to make that change as a follow on
> branch? It effects multiple branches here and it would be cleaner to
> tidy it all up in one swoop.

Sure, that would be fine, if it's easier.

Revision history for this message

Geoff Teale (tealeg) wrote on 2013-07-11: Posted in a previous version of this proposal

> > Hi Bjorn, would you be happy for me to make that change as a follow on
> > branch? It effects multiple branches here and it would be cleaner to
> > tidy it all up in one swoop.
>
> Sure, that would be fine, if it's easier.

OK, I'll add a clean up branch at the end, I have some other calls to tidy up across all branches too.

lp://staging/~tealeg/landscape-client/package-monitor-scoped-resynch updated on 2013-07-16

722. By Geoff Teale on 2013-07-16: merge forwards and resolve conflicts.

Revision history for this message

Jerry Seutter (jseutter) wrote on 2013-07-16:

review: Approve

Revision history for this message

Björn Tillenius (bjornt) wrote on 2013-07-25:

Looks good, +1

[1]

+ def test_not_resynchronize_with_other_scope(self):
+ """
+ If a 'resynchronize' reactor event is fired with an irrelevant scope,
+ the package monitor should not respond to this.
+ """
+ self.monitor.add(self.package_monitor)
+ self.createReporterTask()
+
+ disk_scope = ["disk"]
+ self.monitor.reactor.fire("resynchronize", disk_scope)
+
+ # The next task should *not* be the resynchronize message.
+ self.assertSingleReporterTask(
+ {'ids': [None], 'request-id': 1, 'type': 'package-ids'}, 1)

Where does that dict on the last line come from? Ok, after reading the
diff I can see that it's from createReporterTask(), but if someone looks
at the test only, it's hard to see.

It would be better to either explicitly add that task you expect to be
there, or make createReportTask() return the task and id, so that it's
clear that you expect the existing task to still be there, and no a new
one.

review: Approve

lp://staging/~tealeg/landscape-client/package-monitor-scoped-resynch updated on 2013-07-25