Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(268)

Issue 10180043: state: Unit.WatchConfigSettings

Can't Edit
Can't Publish+Mail
Start Review
Created:
8 years, 5 months ago by fwereade
Modified:
8 years, 5 months ago
Reviewers:
mp+168578, thumper, wallyworld, rog
Visibility:
Public.

Description

state: Unit.WatchConfigSettings state.Settings is bad, because it allows any client the ability to read/change the associated document without any sort of validation; and sending them down a channel is independently bad because the documents don't contain the right data for the unit context. So, I just made it an EntityWatcher instead. Works just as well, is rather simpler, doesn't expose massive potential bugs. Win! https://code.launchpad.net/~fwereade/juju-core/config-4-state-unit-settings-watcher/+merge/168578 Requires: https://code.launchpad.net/~fwereade/juju-core/config-3-state-unit-settings-rename/+merge/168577 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 2

Patch Set 2 : state: Unit.WatchConfigSettings #

Unified diffs Side-by-side diffs Delta from patch set Stats (+102 lines, -99 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M state/service_test.go View 1 chunk +0 lines, -78 lines 0 comments Download
M state/unit_test.go View 2 chunks +75 lines, -0 lines 0 comments Download
M state/watcher.go View 1 2 chunks +21 lines, -17 lines 0 comments Download
M worker/uniter/filter.go View 2 chunks +4 lines, -4 lines 0 comments Download

Messages

Total messages: 5
fwereade
Please take a look.
8 years, 5 months ago (2013-06-11 01:56:13 UTC) #1
thumper
On 2013/06/11 01:56:13, fwereade wrote: > Please take a look. LGTM
8 years, 5 months ago (2013-06-11 05:19:29 UTC) #2
wallyworld
LGTM
8 years, 5 months ago (2013-06-11 08:11:28 UTC) #3
rog
LGTM https://codereview.appspot.com/10180043/diff/1/state/watcher.go File state/watcher.go (right): https://codereview.appspot.com/10180043/diff/1/state/watcher.go#newcode1038 state/watcher.go:1038: // TODO(fwereade): this could be much smarter; if ...
8 years, 5 months ago (2013-06-11 12:13:27 UTC) #4
fwereade
8 years, 5 months ago (2013-06-12 00:30:20 UTC) #5
*** Submitted:

state: Unit.WatchConfigSettings

state.Settings is bad, because it allows any client the ability to
read/change the associated document without any sort of validation;
and sending them down a channel is independently bad because the
documents don't contain the right data for the unit context.

So, I just made it an EntityWatcher instead. Works just as well, is
rather simpler, doesn't expose massive potential bugs. Win!

R=thumper, wallyworld, rog
CC=
https://codereview.appspot.com/10180043

https://codereview.appspot.com/10180043/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/10180043/diff/1/state/watcher.go#newcode1038
state/watcher.go:1038: // TODO(fwereade): this could be much smarter; if it
were, uniter.Filter
On 2013/06/11 12:13:27, rog wrote:
> perhaps say a tiny bit about how this could be smarter?
> it's not obvious to me, at any rate.

The restrictions are described in the preceding para -- it might be nice if it
incorporated a watch on the unit's charm url and switched source. But otoh it's
kind of a hassle to deal with and filter works well enough at the moment.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b