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 worker/
https:/
worker/
current config and
On 2014/03/13 03:07:14, axw wrote:
> On 2014/03/13 01:02:03, waigani wrote:
> > On 2014/03/10 03:41:54, axw wrote:
> > > This shouldn't go until UpdateEnvironConfig is completely fixed.
> >
> > I see that TestInvalidConfig is no longer in trunk, so I assume to
is fine to
> > leave this deleted?
> I just pulled trunk and it is still there. Not sure what you're
looking at...
http://
Done. I confused my working branch with trunk - facepalm.
https:/
File cmd/juju/
https:/
cmd/juju/
"launchpad.
On 2014/03/13 03:07:14, axw wrote:
> dummy will already be imported by the base test suite, but you should
import
> local here.
Done. Yeah, like in the line above - second facepalm.
https:/
File juju/conn.go (right):
https:/
juju/conn.go:146: var secretAttrs map[string]
On 2014/03/13 03:07:14, axw wrote:
> You need to initialise the map, like so:
> secretAttrs := make(map[
Done.
https:/
juju/conn.go:153: secretAttrs[k] = v
On 2014/03/13 03:07:14, axw wrote:
> This will panic, since you didn't initialise the map. I guess you
didn't run the
> tests after this change?
Done. Strange, I guess not. Passes now.
https:/
File juju/conn_test.go (right):
https:/
juju/conn_
resets the "secret"
On 2014/03/13 03:07:14, axw wrote:
> s/vaidates/
Done.
https:/
juju/conn_
On 2014/03/13 03:07:14, axw wrote:
> In normal circumstances it shouldn't be possible, but this is
specifically for a
> test. Not removing it completely misses the point of the test: that if
the
> attributes *aren't* in state, they will be added. This is testing
compatibility
> with legacy code.
> Please put this back to how it was, using a nil policy to override the
removal
> prevention.
Done.
https:/
File state/api/
https:/
state/api/
removing an attribute.
On 2014/03/13 03:07:14, axw wrote:
> ^^
> Adding the test could actually be useful here.
Done.
https:/
File state/apiserver
https:/
state/apiserver
api.state.
On 2014/03/13 03:07:14, axw wrote:
> Can you please add a TODO to pass a validator that ensures the keys
haven't
> changed underfoot? This should have a sort of higher-level transaction
loop.
Done.
https:/
File state/state.go (left):
https:/
state/state.go:265: // TODO(axw) 2013-12-6 #1167616
On 2014/03/13 03:07:14, axw wrote:
> Still not reinstated... did you forget to commit something?
Done.
https:/
File state/state.go (right):
https:/
state/state.go:283: return fmt.Errorf("both updateAttrs and removeAttrs
cannont be empty")
On 2014/03/13 03:07:14, axw wrote:
> I'd be tempted just to return nil here, since there's nothing to do.
It could be
> easy to get into the situation where you compute a bunch of changes to
be made
> (add/modify/remove attributes) and then call UpdateEnvironCo
seems
> onerous to have to check that you've got no changes to make in each
place.
Done.
https:/
File state/state_test.go (left):
https:/
state/state_
On 2014/03/13 03:07:14, axw wrote:
> Please add a test that exercises the new "additionalVali
function of
> UpdateEnvironCo
Done.
https:/
state/state_
On 2014/03/13 03:07:14, axw wrote:
> Remind me why this was deleted please? It's still possible for someone
to come
> along and muck up the attributes in Mongo (e.g. with an buggy script
that
> modified the db directly), just not from within Juju.
Done. Test is replaced.
https:/
File state/state_test.go (right):
https:/
state/state_
On 2014/03/13 03:07:14, axw wrote:
> You've changed the test a bit. It's now checking that the modified
attributes
> are modified, but no longer checking that the existing attributes are
> unmodified, and that no additional attributes were added.
> I think what you should do is get the config first, do an
UpdateEnvironCo
> merge the modified attributes with the existing config and then ensure
that
> EnvironConfig() DeepEquals the merged config.
Done.
https:/
File worker/
https:/
worker/
On 2014/03/13 03:07:14, axw wrote:
> Why are all these tests deleted? We can still have an invalid config
at the
> moment.
Done.
https:/
File worker/
https:/
worker/
gc.ErrorMatches, `no registered provider for "unknown"`)
On 2014/03/13 03:07:14, axw wrote:
> This needs to do what it was doing before. Nil policy.
Done. Wow. I did a lot of really stupid things.
https:/