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

Issue 10172045: state: Service.SetConfigYAML

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+168579, thumper, wallyworld, rog
Visibility:
Public.

Description

state: Service.SetConfigYAML ...now works as intended. Don't get too attached to it though. https://code.launchpad.net/~fwereade/juju-core/config-5-state-service-config-yaml/+merge/168579 Requires: https://code.launchpad.net/~fwereade/juju-core/config-4-state-unit-settings-watcher/+merge/168578 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 6

Patch Set 2 : state: Service.SetConfigYAML #

Unified diffs Side-by-side diffs Delta from patch set Stats (+107 lines, -53 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/config_test.go View 1 2 chunks +6 lines, -7 lines 0 comments Download
M juju/conn_test.go View 2 chunks +9 lines, -3 lines 0 comments Download
M state/apiserver/client_test.go View 3 chunks +5 lines, -7 lines 0 comments Download
M state/apiserver/perm_test.go View 2 chunks +2 lines, -2 lines 0 comments Download
M state/service.go View 3 chunks +40 lines, -27 lines 0 comments Download
M state/service_test.go View 1 chunk +19 lines, -5 lines 0 comments Download
M state/statecmd/deploy_test.go View 2 chunks +24 lines, -2 lines 0 comments Download

Messages

Total messages: 5
fwereade
Please take a look.
8 years, 5 months ago (2013-06-11 01:59:40 UTC) #1
thumper
LGTM - comment about a bug maybe fixed https://codereview.appspot.com/10172045/diff/1/state/service.go File state/service.go (right): https://codereview.appspot.com/10172045/diff/1/state/service.go#newcode778 state/service.go:778: charm, ...
8 years, 5 months ago (2013-06-11 05:22:42 UTC) #2
wallyworld
LGTM
8 years, 5 months ago (2013-06-11 08:16:07 UTC) #3
rog
LGTM, thanks. https://codereview.appspot.com/10172045/diff/1/cmd/juju/config_test.go File cmd/juju/config_test.go (right): https://codereview.appspot.com/10172045/diff/1/cmd/juju/config_test.go#newcode159 cmd/juju/config_test.go:159: path := ctx.AbsPath("testconfig.yaml") err := ioutil.WriteFile( ctx.AbsPath("testconfig.yaml"), ...
8 years, 5 months ago (2013-06-11 12:21:01 UTC) #4
fwereade
8 years, 5 months ago (2013-06-12 01:00:59 UTC) #5
*** Submitted:

state: Service.SetConfigYAML

...now works as intended. Don't get too attached to it though.

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

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

https://codereview.appspot.com/10172045/diff/1/cmd/juju/config_test.go#newcod...
cmd/juju/config_test.go:159: path := ctx.AbsPath("testconfig.yaml")
On 2013/06/11 12:21:01, rog wrote:
> err := ioutil.WriteFile(
>     ctx.AbsPath("testconfig.yaml"),
>     []byte("dummy-service:\n  skill-level: 9000\n  username: admin
>      001\n\n")),
>     0666)
> c.Assert(err, IsNil)
> 
> ?

Why not indeed. Done.

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

https://codereview.appspot.com/10172045/diff/1/state/service.go#newcode748
state/service.go:748: _, err = node.Write()
On 2013/06/11 12:21:01, rog wrote:
> does anything ever use the return from Write?

state.Settings is madness through and through. I suspect that the only reason
for that return value is to effectively expose the algorithm for testing
purposes.

https://codereview.appspot.com/10172045/diff/1/state/service.go#newcode778
state/service.go:778: charm, _, err := s.Charm()
On 2013/06/11 05:22:42, thumper wrote:
> Is lp:1167465 being addressed by this?

Yeah :). Done.
Sign in to reply to this message.

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