Landscape Client

Merge lp://staging/~therve/landscape-client/resync-reboot into lp://staging/~landscape/landscape-client/trunk

  1. resync-reboot
  2. Merge into trunk
Proposed by Thomas Herve
Status: Merged
Approved by: Kevin McDermott
Approved revision: 302
Merged at revision: 302
Proposed branch: lp://staging/~therve/landscape-client/resync-reboot
Merge into: lp://staging/~landscape/landscape-client/trunk
Diff against target: 215 lines (+37/-29)
11 files modified
landscape/monitor/aptpreferences.py (+1/-1)
landscape/monitor/computerinfo.py (+0/-4)
landscape/monitor/hardwareinventory.py (+0/-4)
landscape/monitor/mountinfo.py (+3/-7)
landscape/monitor/networkdevice.py (+0/-5)
landscape/monitor/plugin.py (+4/-0)
landscape/monitor/processorinfo.py (+0/-4)
landscape/monitor/rebootrequired.py (+1/-1)
landscape/monitor/tests/test_aptpreferences.py (+15/-0)
landscape/monitor/tests/test_rebootrequired.py (+11/-0)
landscape/monitor/usermonitor.py (+2/-3)
To merge this branch: bzr merge lp://staging/~therve/landscape-client/resync-reboot
Reviewer Review Type Date Requested Status
Kevin McDermott (community) Approve
Free Ekanayaka (community) Approve
Review via email: mp+43354@code.staging.launchpad.net

Description of the change

I started the branch just adding the resync to RebootRequired, but thought that it should require a more systematic approach. So I put the logic in the Plugin class itself, and kept the tests. It seems safer to me this way (although this is a tricky change).

To post a comment you must log in.
Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

Makes sense to me, just need some more info.

[1]

+ The "resynchronize" reactor message cause the plugin to send fresh

s/cause/causes/

(2 times)

[2]

+ def _resynchronize(self):
+ self.registry.persist.remove(self.persist_name)
+

Are we really sure that we want this for all plugins? I can't think of any bad effect, but it is probably worth double check this.

[3]

- changes = UserChanges(self._persist, self._provider)
- changes.clear()

Looking at the implementation of UserChanges.clear, it looks like this is removing behavior? Is it intended?

review: Needs Information
Revision history for this message
Thomas Herve (therve) wrote :

> [2]
>
> + def _resynchronize(self):
> + self.registry.persist.remove(self.persist_name)
> +
>
> Are we really sure that we want this for all plugins? I can't think of any bad
> effect, but it is probably worth double check this.

Yes, I think we want it. The goal of resync is to wipe out state of the client. Keeping persisting files around doesn't sound good to me.

> [3]
>
> - changes = UserChanges(self._persist, self._provider)
> - changes.clear()
>
> Looking at the implementation of UserChanges.clear, it looks like this is
> removing behavior? Is it intended?

Which behavior are you talking about? The new resync will remove more things, but I think it's sane (previously, only "users" and "groups" were removed, now "username" and "name" are too).

Revision history for this message
Free Ekanayaka (free.ekanayaka) wrote :

Thanks for explaining, +1!

[2]

Okay, agreed.

[3]

Oh, I see. That's fine then.

review: Approve
Revision history for this message
Kevin McDermott (bigkevmcd) wrote :

Makes sense to make this happen in the superclass, +1

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: