Code review comment for lp:~waigani/juju-core/get-oldConfig-from-state
Revision history for this message
| Jesse Meek (waigani) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
73416ba
(Get the code!)
Please take a look.
https:/
File environs/
https:/
environs/
On 2014/03/17 12:15:56, axw wrote:
> No need to type assert, since EnvironProvider implements
state.ConfigVal
> Just "return provider, nil".
Done.
https:/
environs/
errors.
On 2014/03/17 12:15:56, axw wrote:
> Lose the second return
Done.
https:/
File juju/conn_test.go (right):
https:/
juju/conn_
allow us to set an invalid config
On 2014/03/17 12:15:56, axw wrote:
> Please leave a space after //, preferably end sentence with a .
Done.
https:/
File state/api/
https:/
state/api/
s.st.UpdateEnvi
On 2014/03/17 12:15:56, axw wrote:
> check error
Done.
https:/
state/api/
cf.AllAttrs(
On 2014/03/17 12:15:56, axw wrote:
> leftover logging
Done.
https:/
File state/state.go (right):
https:/
state/state.go:279: type ValidateFn func(updateAttrs
map[string]
error
On 2014/03/17 12:15:56, axw wrote:
> This could do with a better name. Out of context (e.g. in godoc), it
wouldn't
> really be meaningful. I suggest ValidateConfigFunc (Func, rather than
Fn, to be
> consistent with function types in Go's standard library).
Done.
https:/
state/state.go:316: //use validCfg to update settings
On 2014/03/17 12:15:56, axw wrote:
> Just drop this comment
Done.
https:/
File state/state_test.go (right):
https:/
state/state_
found {
On 2014/03/17 12:15:56, axw wrote:
> All we really care about is: did the callback get called with the
appropriate
> arguments, and does its result have the appropriate effect?
> So there's some missing coverage there: there's no validation of the
input, and
> there's no test for a non-nil validator with a nil result (i.e. has no
effect on
> normal operation).
Done.
https:/
state/state_
bson.D{{"name", "foo"}}}})
On 2014/03/17 12:15:56, axw wrote:
> What is this doing here? Is this because UpdateEnvironConfig won't
change the
> "name" attribute? If it's needed, you should probably be using
> settings.UpdateId.
Done. Changed to UpdateId. As "name" has been removed, settings errors
out in UpdateEnvironConig. I'm not sure if this is the best way around
it, but it is a way.
https:/
File worker/
https:/
worker/
TestProvisionin
On 2014/03/17 12:15:56, axw wrote:
> As before, reinstate until we can guarantee config validity.
Done.
https:/