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 cmd/juju/
https:/
cmd/juju/
conn.State.
On 2014/03/10 03:41:54, axw wrote:
> Just nil is fine.
Done.
https:/
File cmd/juju/
https:/
cmd/juju/
registered provider for "foo"`)
On 2014/03/10 03:41:54, axw wrote:
> 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.
Done.
https:/
File environs/config.go (right):
https:/
environs/
(EnvironProvider, error) {
On 2014/03/10 03:41:54, axw wrote:
> 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.
Sorry about that. This branch is actually three squashed together. These
changes are in response to William's review of one of those branches.
https:/
File juju/conn.go (right):
https:/
juju/conn.go:155: return c.State.
On 2014/03/10 03:41:54, axw wrote:
> Just pass secrets here, and don't bother updating attrs in the loop.
Done. I couldn't cast or assert secrets (which is map[string]string) to
map[string]
type instead.
https:/
File state/api/
https:/
state/api/
an attribute.
On 2014/03/10 03:41:54, axw wrote:
> 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.
Done. TODO removed.
https:/
state/api/
an attribute.
On 2014/03/10 03:41:54, axw wrote:
> 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.
On 2014/03/10 03:41:54, axw wrote:
> 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.
Done. UpdateEnvironConfig now takes an additionalValid
https:/
state/apiserver
loop
On 2014/03/10 03:41:54, axw wrote:
> 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).
Done. I also returned your TODO comment in state.go. Thanks for showing
me the standard.
https:/
state/apiserver
c.api.state.
On 2014/03/10 03:41:54, axw wrote:
> The second argument can simply be nil.
Done. I changed all calls.
https:/
File state/apiserver
https:/
state/apiserver
keys are global, common to all users.
On 2014/03/10 03:41:54, axw wrote:
> This comment didn't need to be deleted
I'm confused. The comment is still there on line 312?
https:/
File state/state.go (left):
https:/
state/state.go:265: // TODO(axw) 2013-12-6 #1167616
On 2014/03/10 03:41:54, axw wrote:
> The TODO will not be resolved until the settings-
assertion is in
> place. Please do not delete it until then,.
Done.
https:/
File state/state.go (right):
https:/
state/state.go:259: // DELETE ME
On 2014/03/10 03:41:54, axw wrote:
> DELETE ME
Done.
https:/
state/state.go:305: oldConfig, err := config.
existingAttrs)
On 2014/03/10 03:41:54, axw wrote:
> I think there's no reason not to inline settings.Map() here.
Done.
https:/
File state/testing/
https:/
state/testing/
UpdateEnvironConfig
On 2014/03/10 03:41:54, axw wrote:
> Please either do it or create a TODO(waigaini) with accompanying
issue.
Done. UpdateConfig replaced.
https:/
File worker/
https:/
worker/
current config and
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?
https:/
File worker/
https:/
worker/
attrs["
On 2014/03/10 03:41:54, axw wrote:
> Again, don't delete until UpdateEnvironConfig is fixed. Open a state
conn with
> nil validator here if you have to.
Done.
https:/
File worker/
(right):
https:/
worker/
attrs := map[string]
On 2014/03/10 03:41:54, axw wrote:
> The conventional way to write this is
> attrs := make(map[
Ah. There are a lot of map[string]
others). Shall I put off changing these to another day/branch?
https:/