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

Issue 10083047: state: Service ConfigSettings methods

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

Description

state: Service ConfigSettings methods Config, SetConfig, and SetConfigYAML methods have been dropped in favour of ConfigSettings and UpdateConfigSettings, which use sensible types. Clients are expected to parse their own damn data and supply a sensible format. https://code.launchpad.net/~fwereade/juju-core/config-6-state-service-sane-methods/+merge/168580 Requires: https://code.launchpad.net/~fwereade/juju-core/config-5-state-service-config-yaml/+merge/168579 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 12

Patch Set 2 : state: Service ConfigSettings methods #

Unified diffs Side-by-side diffs Delta from patch set Stats (+261 lines, -365 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/cmd_test.go View 1 chunk +2 lines, -2 lines 0 comments Download
M cmd/juju/config_test.go View 1 5 chunks +11 lines, -12 lines 0 comments Download
M cmd/juju/deploy.go View 1 chunk +3 lines, -4 lines 0 comments Download
M cmd/juju/deploy_test.go View 1 chunk +6 lines, -0 lines 0 comments Download
M cmd/juju/set.go View 3 chunks +40 lines, -32 lines 0 comments Download
M juju/conn.go View 1 chunk +15 lines, -13 lines 0 comments Download
M juju/conn_test.go View 2 chunks +4 lines, -4 lines 0 comments Download
M state/api/client.go View 1 chunk +0 lines, -1 line 0 comments Download
M state/api/params/params_test.go View 1 chunk +1 line, -1 line 0 comments Download
M state/apiserver/client.go View 2 chunks +18 lines, -2 lines 0 comments Download
M state/apiserver/client_test.go View 2 chunks +4 lines, -4 lines 0 comments Download
M state/megawatcher_internal_test.go View 12 chunks +13 lines, -20 lines 0 comments Download
M state/service.go View 2 chunks +12 lines, -39 lines 0 comments Download
M state/service_test.go View 7 chunks +69 lines, -151 lines 0 comments Download
M state/statecmd/config_test.go View 1 chunk +3 lines, -1 line 0 comments Download
M state/statecmd/deploy_test.go View 2 chunks +4 lines, -4 lines 0 comments Download
M state/statecmd/get.go View 1 chunk +26 lines, -31 lines 0 comments Download
M state/unit_test.go View 4 chunks +7 lines, -7 lines 0 comments Download
M worker/uniter/context_test.go View 1 1 chunk +3 lines, -4 lines 0 comments Download
M worker/uniter/filter_test.go View 3 chunks +17 lines, -29 lines 0 comments Download
M worker/uniter/uniter_test.go View 1 chunk +1 line, -4 lines 0 comments Download

Messages

Total messages: 5
fwereade
Please take a look.
8 years, 5 months ago (2013-06-11 02:04:10 UTC) #1
mue
LGTM, cleaner interface now.
8 years, 5 months ago (2013-06-11 09:50:28 UTC) #2
rog
https://codereview.appspot.com/10083047/diff/1/cmd/juju/deploy.go File cmd/juju/deploy.go (right): https://codereview.appspot.com/10083047/diff/1/cmd/juju/deploy.go#newcode95 cmd/juju/deploy.go:95: return errors.New("must deploy at least one unit") personally i ...
8 years, 5 months ago (2013-06-11 12:31:23 UTC) #3
rog
LGTM https://codereview.appspot.com/10083047/diff/1/juju/conn.go File juju/conn.go (right): https://codereview.appspot.com/10083047/diff/1/juju/conn.go#newcode219 juju/conn.go:219: if err := svc.UpdateConfigSettings(settings); err != nil { ...
8 years, 5 months ago (2013-06-11 12:53:12 UTC) #4
fwereade
8 years, 5 months ago (2013-06-12 01:17:31 UTC) #5
*** Submitted:

state: Service ConfigSettings methods

Config, SetConfig, and SetConfigYAML methods have been dropped in favour of
ConfigSettings and UpdateConfigSettings, which use sensible types. Clients
are expected to parse their own damn data and supply a sensible format.

R=mue, rog
CC=
https://codereview.appspot.com/10083047

https://codereview.appspot.com/10083047/diff/1/cmd/juju/deploy.go
File cmd/juju/deploy.go (right):

https://codereview.appspot.com/10083047/diff/1/cmd/juju/deploy.go#newcode95
cmd/juju/deploy.go:95: return errors.New("must deploy at least one unit")
On 2013/06/11 12:31:23, rog wrote:
> personally i don't see why we require >0 units.

I'm trying not to get sidetracked with such questions here ;).

https://codereview.appspot.com/10083047/diff/1/state/apiserver/client.go
File state/apiserver/client.go (right):

https://codereview.appspot.com/10083047/diff/1/state/apiserver/client.go#newc...
state/apiserver/client.go:70: changes, err :=
ch.Config().ParseSettingsYAML([]byte(p.Config), p.ServiceName)
On 2013/06/11 12:53:12, rog wrote:
> we should check with the GUI folks that they haven't put in a workaround for
the
> current behaviour before this branch lands.

Agreed before embarking on this course of action -- they just pass it through
directly when they actually use it, so they're not affected.

https://codereview.appspot.com/10083047/diff/1/state/service.go
File state/service.go (right):

https://codereview.appspot.com/10083047/diff/1/state/service.go#newcode729
state/service.go:729: return settings.Map(), nil
On 2013/06/11 12:53:12, rog wrote:
> i'd be tempted to return settings.core to avoid yet another unnecessary map
copy
> (or implement Settings.sharedMap which just returns settings.core,
> amounting to the same thing).
> 
> perhaps the overhead amounts to nothing in the long run though. <hits self>.

I officially do not care about the overhead of in-memory copies of small maps
:).

https://codereview.appspot.com/10083047/diff/1/state/statecmd/get.go
File state/statecmd/get.go (right):

https://codereview.appspot.com/10083047/diff/1/state/statecmd/get.go#newcode52
state/statecmd/get.go:52: info := map[string]interface{}{
On 2013/06/11 12:53:12, rog wrote:
> this should really be a struct, i think.

I resisted the temptation to pull this whole lot into the charm package... I
think I'll leave it out for now.

https://codereview.appspot.com/10083047/diff/1/worker/uniter/context_test.go
File worker/uniter/context_test.go (right):

https://codereview.appspot.com/10083047/diff/1/worker/uniter/context_test.go#...
worker/uniter/context_test.go:582: })
On 2013/06/11 12:53:12, rog wrote:
> c.Assert(err, IsNil)

Gaaah! Thank you.
Sign in to reply to this message.

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