Code review comment for lp:~waigani/juju-core/get-oldConfig-from-state
Revision history for this message
| Andrew Wilkins (axwalk) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
73416ba
(Get the code!)
I haven't reviewed everything, this should keep you going for a while
though.
In general: we should not be dropping any tests that are not
*guaranteed* to never work. Since UpdateEnvironConfig is not completely
fixed yet, the tests should remain. This may involve hacking around
validation with a nil policy.
https:/
File cmd/juju/
https:/
cmd/juju/
conn.State.
Just nil is fine.
https:/
File cmd/juju/
https:/
cmd/juju/
registered provider for "foo"`)
This has nothing to do with attribute mutability, so doesn't belong
here. Why not just change the value in the map to a real provider type?
You will need to make sure the provider is registered by importing its
package.
https:/
File environs/config.go (right):
https:/
environs/
(EnvironProvider, error) {
Making cosmetic changes is fine, but it's best not to mix cosmetic
changes (or refactoring) with functional changes. Otherwise I'm
scrutinising everything to make sure there's no functional change, and
the review takes longer than necessary.
https:/
File environs/
https:/
environs/
[]string{})
Just pass in the one attribute in a new map. FYI, you can make maps with
map literals like so:
map[
SSLHostnameVeri
https:/
File juju/conn.go (right):
https:/
juju/conn.go:155: return c.State.
Just pass secrets here, and don't bother updating attrs in the loop.
https:/
File juju/conn_test.go (right):
https:/
juju/conn_
possible, as
Well, that's only true for the dummy provider. Real providers don't have
defaults for secrets. For a real provider you'd have a different
problem: validation would fail because a config without the secrets is
invalid.
In this test it makes sense to disable validation while you're removing
the attribute.
https:/
File state/api/
https:/
state/api/
an attribute.
I don't think deferral is warranted here. Not sure it's really necessary
to add the test either, though: firewaller doesn't care about attributes
being removed.
https:/
File state/apiserver
https:/
state/apiserver
c.api.state.
This is a bit ugly, requiring another load. I wonder if we could pass an
additional validation function down to UpdateEnvironCo
Just a thought, not blocking.
https:/
state/apiserver
loop
We have a standard format for TODOs:
// TODO(user) YYYY-MM-DD #issue
// A brief summary of the issue here.
Please file an issue if there's not one already (I think the one I
referred to in state.go is suitable, so just refer to that).
https:/
state/apiserver
c.api.state.
The second argument can simply be nil.
https:/
File state/apiserver
https:/
state/apiserver
keys are global, common to all users.
This comment didn't need to be deleted
https:/
File state/state.go (left):
https:/
state/state.go:265: // TODO(axw) 2013-12-6 #1167616
The TODO will not be resolved until the settings-
assertion is in place. Please do not delete it until then,.
https:/
File state/state.go (right):
https:/
state/state.go:259: // DELETE ME
DELETE ME
https:/
state/state.go:305: oldConfig, err := config.
existingAttrs)
I think there's no reason not to inline settings.Map() here.
https:/
File state/testing/
https:/
state/testing/
UpdateEnvironConfig
Please either do it or create a TODO(waigaini) with accompanying issue.
https:/
File worker/
https:/
worker/
current config and
This shouldn't go until UpdateEnvironConfig is completely fixed.
https:/
File worker/
https:/
worker/
Winner!
https:/
File worker/
https:/
worker/
attrs["
Again, don't delete until UpdateEnvironConfig is fixed. Open a state
conn with nil validator here if you have to.
https:/
File worker/
(right):
https:/
worker/
attrs := map[string]
The conventional way to write this is
attrs := make(map[
https:/