Merge lp:~waigani/juju-core/get-oldConfig-from-state into lp:~go-bot/juju-core/trunk

Proposed by Jesse Meek
Status: Merged
Approved by: Jesse Meek
Approved revision: no longer in the source branch.
Merged at revision: 2433
Proposed branch: lp:~waigani/juju-core/get-oldConfig-from-state
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 1846 lines (+475/-413)
46 files modified
cmd/juju/authorisedkeys_test.go (+1/-2)
cmd/juju/environment.go (+2/-20)
cmd/juju/environment_test.go (+2/-1)
cmd/juju/upgradejuju_test.go (+7/-14)
cmd/jujud/machine_test.go (+3/-8)
environs/config.go (+5/-5)
environs/config/config.go (+9/-0)
environs/statepolicy.go (+10/-0)
environs/testing/tools.go (+1/-7)
juju/conn.go (+3/-7)
juju/conn_test.go (+4/-10)
juju/testing/conn.go (+5/-1)
juju/testing/repo.go (+2/-5)
juju/testing/utils.go (+0/-14)
state/api/common/testing/environwatcher.go (+16/-7)
state/api/firewaller/state_test.go (+4/-7)
state/api/keymanager/client_test.go (+1/-2)
state/api/keyupdater/authorisedkeys_test.go (+1/-1)
state/api/logger/logger_test.go (+1/-1)
state/apiserver/client/client.go (+15/-30)
state/apiserver/client/client_test.go (+3/-12)
state/apiserver/keymanager/keymanager.go (+21/-21)
state/apiserver/keymanager/keymanager_test.go (+1/-2)
state/apiserver/keyupdater/authorisedkeys_test.go (+1/-1)
state/apiserver/logger/logger_test.go (+1/-1)
state/apiserver/provisioner/provisioner_test.go (+3/-6)
state/apiserver/rsyslog/rsyslog.go (+2/-10)
state/configvalidator_test.go (+114/-0)
state/conn_test.go (+9/-1)
state/initialize_test.go (+5/-5)
state/machine_test.go (+2/-3)
state/policy.go (+28/-0)
state/state.go (+48/-10)
state/state_test.go (+50/-19)
state/testing/agent.go (+1/-1)
state/testing/config.go (+0/-22)
upgrades/deprecatedattributes.go (+8/-21)
upgrades/deprecatedattributes_test.go (+3/-6)
upgrades/rsyslogport.go (+2/-9)
worker/authenticationworker/worker_test.go (+1/-2)
worker/environ_test.go (+17/-13)
worker/firewaller/firewaller_test.go (+17/-38)
worker/instancepoller/observer_test.go (+12/-15)
worker/machineenvironmentworker/machineenvironmentworker_test.go (+8/-8)
worker/provisioner/provisioner_test.go (+23/-39)
worker/uniter/uniter_test.go (+3/-6)
To merge this branch: bzr merge lp:~waigani/juju-core/get-oldConfig-from-state
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+209569@code.launchpad.net

Commit message

Update and Validate Environ Config in State

The main business logic happens in state/state.go. The bulk of the other
changes are mechanical i.e. updating method calls. There are a few things going on:

-Explicitly add/update or remove attributes with state.UpdateEnvironConf, which
replaces state.SetEnvironConf.
- Add ConfigValidator to state.Policy interface.
- Validate environment configuration within state.
    - build new config from UpdateEnvironConf args, get old config from state.
    - pass both configs to st.validate which use the environ policy
 ConfigValidator to get the correct validation method.
    - use the validated config to make the actual updates in state.
- Update or delete tests that rely on an invalid configuration being set, as
    this is now not possible.

Description of the change

Update and Validate Environ Config in State

The main business logic happens in state/state.go. The bulk of the other
changes are mechanical i.e. updating method calls. There are a few things going on:

-Explicitly add/update or remove attributes with state.UpdateEnvironConf, which
replaces state.SetEnvironConf.
- Add ConfigValidator to state.Policy interface.
- Validate environment configuration within state.
    - build new config from UpdateEnvironConf args, get old config from state.
    - pass both configs to st.validate which use the environ policy
 ConfigValidator to get the correct validation method.
    - use the validated config to make the actual updates in state.
- Update or delete tests that rely on an invalid configuration being set, as
    this is now not possible.

A follow up branch will use txn and a retry loop to address a race condition in
updating state config to ensure that concurrent updates to state config do not corrupt one
another.

https://codereview.appspot.com/70190050/

To post a comment you must log in.
Revision history for this message
Andrew Wilkins (axwalk) wrote :

Some preliminary comments. I've only really looked at state.go.

https://codereview.appspot.com/70190050/diff/1/state/state.go
File state/state.go (left):

https://codereview.appspot.com/70190050/diff/1/state/state.go#oldcode282
state/state.go:282: _, err = settings.Write()
Write isn't really good enough here; this does not address the deleted
TODO. The update to settings should assert that there has been no change
to settings since st.EnvironConfig() was called; otherwise the
validation is not complete.

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

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode259
state/state.go:259: func (st *State)
VaidateUpdateEnvironConfig(updateAttrs map[string]interface{},
removeAttrs []string, oldConfig *config.Config) error {
s/Vaidate/Validate/
also, doesn't need to be exported does it?

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode264
state/state.go:264: // Build new config with attributes to add/update
You can get rid of the comments, if statement, forward declared
newConfig/err vars, and just replace with:

     newConfig, err := oldConfig.Apply(updateAttrs)
     if err != nil {
         return err
     }

Apply doesn't care if updateAttrs is nil or empty.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode274
state/state.go:274: if len(removeAttrs) != 0 {
And then since newConfig exists unconditionally:

     newConfig, err = newConfig.Remove(removeAttrs)
     if err != nil {
         return err
     }

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode304
state/state.go:304: func (st *State) SetEnvironConfig(attrs
map[string]interface{}) error {
Why do we still need SetEnvironConfig? This still has the old race
conditions. *If* it's needed, then SetEnvironConfig and
UpdateEnvironConfig should use a common method that takes a
*state.Settings as input and does the no-concurrent-change check on
that.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode327
state/state.go:327: oldConfig, err := st.EnvironConfig()
This does a readSettings, so you're reading settings twice. I think it'd
be preferable to just do a readSettings and use its Map() to create a
config.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode333
state/state.go:333: err = st.VaidateUpdateEnvironConfig(updateAttrs,
removeAttrs, oldConfig)
Since ValidateUpdateEnvironConfig updates the config with attributes to
validate, I wonder if it's worthwhile even having a separate method.
You're duplicating work here by updating settings.

I think it would be simpler to just:
    1. get settings, and create a config from them
    2. apply changes to get a new config, check validity
    3. do something *like* replaceSettingsOp (see settings.go), but
passing in the *state.Settings

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode351
state/state.go:351: // Add/Update attributes.
These comments aren't really useful, the code speaks for itself.

https://codereview.appspot.com/70190050/

Revision history for this message
Jesse Meek (waigani) wrote :
Download full text (5.6 KiB)

Reviewers: mp+209569_code.launchpad.net, axw,

https://codereview.appspot.com/70190050/diff/1/state/state.go
File state/state.go (left):

https://codereview.appspot.com/70190050/diff/1/state/state.go#oldcode282
state/state.go:282: _, err = settings.Write()
On 2014/03/06 04:19:17, axw wrote:
> Write isn't really good enough here; this does not address the deleted
TODO. The
> update to settings should assert that there has been no change to
settings since
> st.EnvironConfig() was called; otherwise the validation is not
complete.

fwereade wanted a txn retry loop, ensuring the underlying settings had
not been changed, in client.EnvironmentSet
(https://codereview.appspot.com/63790045/diff/40001/state/apiserver/client/client.go#newcode791).
As this is a WIP, I have not done it yet. I can make a TODO in
client.EnvironmentSet and write the txn retry loop in a follow up
branch?

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

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode259
state/state.go:259: func (st *State)
VaidateUpdateEnvironConfig(updateAttrs map[string]interface{},
removeAttrs []string, oldConfig *config.Config) error {
On 2014/03/06 04:19:17, axw wrote:
> s/Vaidate/Validate/
> also, doesn't need to be exported does it?

Good catch!

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode264
state/state.go:264: // Build new config with attributes to add/update
On 2014/03/06 04:19:17, axw wrote:
> You can get rid of the comments, if statement, forward declared
newConfig/err
> vars, and just replace with:

> newConfig, err := oldConfig.Apply(updateAttrs)
> if err != nil {
> return err
> }

> Apply doesn't care if updateAttrs is nil or empty.

Done.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode274
state/state.go:274: if len(removeAttrs) != 0 {
On 2014/03/06 04:19:17, axw wrote:
> And then since newConfig exists unconditionally:

> newConfig, err = newConfig.Remove(removeAttrs)
> if err != nil {
> return err
> }

Done.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode304
state/state.go:304: func (st *State) SetEnvironConfig(attrs
map[string]interface{}) error {
On 2014/03/06 04:19:17, axw wrote:
> Why do we still need SetEnvironConfig? This still has the old race
conditions.
> *If* it's needed, then SetEnvironConfig and UpdateEnvironConfig should
use a
> common method that takes a *state.Settings as input and does the
> no-concurrent-change check on that.

It was needed by testing.ChangeEnvironConfig. ChangeEnvironConfig is
only used in two places, but it is actually easier to use
UpdateEnvironConfig directly instead. So I would be in favour of
deleting SetEnvironConfig and ChangeEnvironConfig.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode327
state/state.go:327: oldConfig, err := st.EnvironConfig()
On 2014/03/06 04:19:17, axw wrote:
> This does a readSettings, so you're reading settings twice. I think
it'd be
> preferable to just do a readSettings and use its Map() to create a
config.

Done.

https://codereview.appspot.com/70190050/diff/1/state/stat...

Read more...

Revision history for this message
Andrew Wilkins (axwalk) wrote :
Download full text (3.5 KiB)

https://codereview.appspot.com/70190050/diff/1/state/state.go
File state/state.go (left):

https://codereview.appspot.com/70190050/diff/1/state/state.go#oldcode282
state/state.go:282: _, err = settings.Write()
On 2014/03/06 08:17:48, waigani wrote:
> On 2014/03/06 04:19:17, axw wrote:
> > Write isn't really good enough here; this does not address the
deleted TODO.
> The
> > update to settings should assert that there has been no change to
settings
> since
> > st.EnvironConfig() was called; otherwise the validation is not
complete.

> fwereade wanted a txn retry loop, ensuring the underlying settings had
not been
> changed, in client.EnvironmentSet

(https://codereview.appspot.com/63790045/diff/40001/state/apiserver/client/client.go#newcode791).
> As this is a WIP, I have not done it yet. I can make a TODO in
> client.EnvironmentSet and write the txn retry loop in a follow up
branch?

The assertion is a prerequisite of the looping. The assertion is what
ensures that the settings haven't been changed concurrently. If the
settings are changed underfoot, then the transaction fails with
txn.ErrAborted.

The assertion must happen here, whereas looping may happen here or in
the caller. There's basically no point in making any of these changes
unless that assertion is in place.

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

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode304
state/state.go:304: func (st *State) SetEnvironConfig(attrs
map[string]interface{}) error {
On 2014/03/06 08:17:48, waigani wrote:
> On 2014/03/06 04:19:17, axw wrote:
> > Why do we still need SetEnvironConfig? This still has the old race
conditions.
> > *If* it's needed, then SetEnvironConfig and UpdateEnvironConfig
should use a
> > common method that takes a *state.Settings as input and does the
> > no-concurrent-change check on that.

> It was needed by testing.ChangeEnvironConfig. ChangeEnvironConfig is
only used
> in two places, but it is actually easier to use UpdateEnvironConfig
directly
> instead. So I would be in favour of deleting SetEnvironConfig and
> ChangeEnvironConfig.

ChangeEnvironConfig can be modified to compute a delta and pass it into
UpdateEnvironConfig. I know it's a bit arse about, but I don't think we
should be exposing ourselves to bugs just to simplify a test. If you can
just get rid of ChangeEnvironConfig, though, that may be better.

https://codereview.appspot.com/70190050/diff/1/state/state.go#newcode333
state/state.go:333: err = st.VaidateUpdateEnvironConfig(updateAttrs,
removeAttrs, oldConfig)
On 2014/03/06 08:17:48, waigani wrote:
> On 2014/03/06 04:19:17, axw wrote:
> > Since ValidateUpdateEnvironConfig updates the config with attributes
to
> > validate, I wonder if it's worthwhile even having a separate method.
You're
> > duplicating work here by updating settings.
> >
> > I think it would be simpler to just:
> > 1. get settings, and create a config from them
> > 2. apply changes to get a new config, check validity
> > 3. do something *like* replaceSettingsOp (see settings.go), but
passing in
> > the *state.Settings

> Do you think something *like* replaceSettingsOp s...

Read more...

Revision history for this message
Jesse Meek (waigani) wrote :
Revision history for this message
Jesse Meek (waigani) wrote :
Revision history for this message
Jesse Meek (waigani) wrote :
Revision history for this message
Andrew Wilkins (axwalk) wrote :
Download full text (7.6 KiB)

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://codereview.appspot.com/70190050/diff/60001/cmd/juju/environment.go
File cmd/juju/environment.go (right):

https://codereview.appspot.com/70190050/diff/60001/cmd/juju/environment.go#newcode166
cmd/juju/environment.go:166: return
conn.State.UpdateEnvironConfig(c.values, []string{})
Just nil is fine.

https://codereview.appspot.com/70190050/diff/60001/cmd/juju/environment_test.go
File cmd/juju/environment_test.go (right):

https://codereview.appspot.com/70190050/diff/60001/cmd/juju/environment_test.go#newcode173
cmd/juju/environment_test.go:173: c.Check(err, gc.ErrorMatches, `no
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://codereview.appspot.com/70190050/diff/60001/environs/config.go
File environs/config.go (right):

https://codereview.appspot.com/70190050/diff/60001/environs/config.go#newcode138
environs/config.go:138: func Provider(providerType string)
(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://codereview.appspot.com/70190050/diff/60001/environs/testing/tools.go
File environs/testing/tools.go (right):

https://codereview.appspot.com/70190050/diff/60001/environs/testing/tools.go#newcode447
environs/testing/tools.go:447: err = st.UpdateEnvironConfig(attrs,
[]string{})
Just pass in the one attribute in a new map. FYI, you can make maps with
map literals like so:
   map[string]interface{}{"ssl-hostname-verification":
SSLHostnameVerification}

https://codereview.appspot.com/70190050/diff/60001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/70190050/diff/60001/juju/conn.go#newcode155
juju/conn.go:155: return c.State.UpdateEnvironConfig(attrs, []string{})
Just pass secrets here, and don't bother updating attrs in the loop.

https://codereview.appspot.com/70190050/diff/60001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/70190050/diff/60001/juju/conn_test.go#newcode152
juju/conn_test.go:152: // Removing the secret from state should not be
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://codereview.appspot.com/70190050/diff/60001/state/api/firewaller/state_test.go
File state/api/firewaller/state_test.go (right):

https://codereview.appspot.com/70190050/diff/60001/...

Read more...

Revision history for this message
Jesse Meek (waigani) wrote :
Download full text (7.9 KiB)

Please take a look.

https://codereview.appspot.com/70190050/diff/60001/cmd/juju/environment.go
File cmd/juju/environment.go (right):

https://codereview.appspot.com/70190050/diff/60001/cmd/juju/environment.go#newcode166
cmd/juju/environment.go:166: return
conn.State.UpdateEnvironConfig(c.values, []string{})
On 2014/03/10 03:41:54, axw wrote:
> Just nil is fine.

Done.

https://codereview.appspot.com/70190050/diff/60001/cmd/juju/environment_test.go
File cmd/juju/environment_test.go (right):

https://codereview.appspot.com/70190050/diff/60001/cmd/juju/environment_test.go#newcode173
cmd/juju/environment_test.go:173: c.Check(err, gc.ErrorMatches, `no
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://codereview.appspot.com/70190050/diff/60001/environs/config.go
File environs/config.go (right):

https://codereview.appspot.com/70190050/diff/60001/environs/config.go#newcode138
environs/config.go:138: func Provider(providerType string)
(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://codereview.appspot.com/70190050/diff/60001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/70190050/diff/60001/juju/conn.go#newcode155
juju/conn.go:155: return c.State.UpdateEnvironConfig(attrs, []string{})
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]interface{}, so I created a new var secretAttrs with that
type instead.

https://codereview.appspot.com/70190050/diff/60001/state/api/firewaller/state_test.go
File state/api/firewaller/state_test.go (right):

https://codereview.appspot.com/70190050/diff/60001/state/api/firewaller/state_test.go#newcode94
state/api/firewaller/state_test.go:94: // TODO [waigani] test removing
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://codereview.appspot.com/70190050/diff/60001/state/api/firewaller/state_test.go#newcode94
state/api/firewaller/state_test.go:94: // TODO [waigani] test removing
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://codereview.appspot.com/70190050/diff/60001/state/apiserver/client/client.go
File state/apiserver/client/client...

Read more...

Revision history for this message
Jesse Meek (waigani) wrote :

Yikes. I really dug a hole for myself with such a big branch. I've
finally merged in trunk, all tests pass and review comments addressed.
Let me know if I've missed anything.

https://codereview.appspot.com/70190050/

Revision history for this message
Andrew Wilkins (axwalk) wrote :
Download full text (7.9 KiB)

Getting there.

https://codereview.appspot.com/70190050/diff/60001/state/apiserver/keymanager/keymanager.go
File state/apiserver/keymanager/keymanager.go (left):

https://codereview.appspot.com/70190050/diff/60001/state/apiserver/keymanager/keymanager.go#oldcode311
state/apiserver/keymanager/keymanager.go:311: // For now, authorised
keys are global, common to all users.
On 2014/03/13 01:02:03, waigani wrote:
> 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?

Huh, I must've been on crack. Never mind.

https://codereview.appspot.com/70190050/diff/60001/worker/environ_test.go
File worker/environ_test.go (left):

https://codereview.appspot.com/70190050/diff/60001/worker/environ_test.go#oldcode49
worker/environ_test.go:49: // Create an invalid config by taking the
current config and
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://bazaar.launchpad.net/~go-bot/juju-core/trunk/view/head:/worker/environ_test.go#L48

https://codereview.appspot.com/70190050/diff/60001/worker/machineenvironmentworker/machineenvironmentworker_test.go
File worker/machineenvironmentworker/machineenvironmentworker_test.go
(right):

https://codereview.appspot.com/70190050/diff/60001/worker/machineenvironmentworker/machineenvironmentworker_test.go#newcode128
worker/machineenvironmentworker/machineenvironmentworker_test.go:128:
attrs := map[string]interface{}{}
On 2014/03/13 01:02:03, waigani wrote:
> On 2014/03/10 03:41:54, axw wrote:
> > The conventional way to write this is
> > attrs := make(map[string]interface{})

> Ah. There are a lot of map[string]interface{}{} around (from my code
and
> others). Shall I put off changing these to another day/branch?

No problems, just leave them.

https://codereview.appspot.com/70190050/diff/80001/cmd/juju/environment_test.go
File cmd/juju/environment_test.go (right):

https://codereview.appspot.com/70190050/diff/80001/cmd/juju/environment_test.go#newcode14
cmd/juju/environment_test.go:14: _
"launchpad.net/juju-core/provider/dummy"
dummy will already be imported by the base test suite, but you should
import local here.

https://codereview.appspot.com/70190050/diff/80001/environs/statepolicy.go
File environs/statepolicy.go (right):

https://codereview.appspot.com/70190050/diff/80001/environs/statepolicy.go#newcode43
environs/statepolicy.go:43: if p, ok :=
provider.(state.ConfigValidator); ok {
No need for the assertion here, this is always going to be true for
Provider. Just go ahead and "return p, nil".

https://codereview.appspot.com/70190050/diff/80001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/70190050/diff/80001/juju/conn.go#newcode146
juju/conn.go:146: var secretAttrs map[string]interface{}
You need to initialise the map, like so:
     secretAttrs := make(map[string]interface{})

https://codereview.appspot.com/70190050/diff/80001/juju...

Read more...

Revision history for this message
Jesse Meek (waigani) wrote :
Download full text (7.1 KiB)

Please take a look.

https://codereview.appspot.com/70190050/diff/60001/worker/environ_test.go
File worker/environ_test.go (left):

https://codereview.appspot.com/70190050/diff/60001/worker/environ_test.go#oldcode49
worker/environ_test.go:49: // Create an invalid config by taking the
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://bazaar.launchpad.net/~go-bot/juju-core/trunk/view/head:/worker/environ_test.go#L48

Done. I confused my working branch with trunk - facepalm.

https://codereview.appspot.com/70190050/diff/80001/cmd/juju/environment_test.go
File cmd/juju/environment_test.go (right):

https://codereview.appspot.com/70190050/diff/80001/cmd/juju/environment_test.go#newcode14
cmd/juju/environment_test.go:14: _
"launchpad.net/juju-core/provider/dummy"
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://codereview.appspot.com/70190050/diff/80001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/70190050/diff/80001/juju/conn.go#newcode146
juju/conn.go:146: var secretAttrs map[string]interface{}
On 2014/03/13 03:07:14, axw wrote:
> You need to initialise the map, like so:
> secretAttrs := make(map[string]interface{})

Done.

https://codereview.appspot.com/70190050/diff/80001/juju/conn.go#newcode153
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://codereview.appspot.com/70190050/diff/80001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/70190050/diff/80001/juju/conn_test.go#newcode153
juju/conn_test.go:153: // UpdateEnvironConfig vaidates the config and
resets the "secret"
On 2014/03/13 03:07:14, axw wrote:
> s/vaidates/validates/

Done.

https://codereview.appspot.com/70190050/diff/80001/juju/conn_test.go#newcode154
juju/conn_test.go:154: // setting.
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://codereview.appspot.com/70190050/diff/80001/state/api/common/testing/environwatcher.go
File state/api/common/testing/environwatcher.go (right):

https://codereview.appspot.com/70190050/diff/80001/state/api/common/testing/environwatcher.go#newcode91
state/api/common/testing/environwatcher.go:91: // TODO [waigani] test
removing an attribute.
On 2014/03/13 03:07:14, axw w...

Read more...

Revision history for this message
Jesse Meek (waigani) wrote :
Revision history for this message
Andrew Wilkins (axwalk) wrote :
Download full text (3.6 KiB)

A few minor things, then it should be good to land.

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go
File environs/statepolicy.go (right):

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go#newcode43
environs/statepolicy.go:43: return provider.(state.ConfigValidator), nil
No need to type assert, since EnvironProvider implements
state.ConfigValidator. Just "return provider, nil".

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go#newcode44
environs/statepolicy.go:44: return nil,
errors.NewNotImplementedError("ConfigValidator")
Lose the second return

https://codereview.appspot.com/70190050/diff/100001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/70190050/diff/100001/juju/conn_test.go#newcode143
juju/conn_test.go:143: //Use a state without a nil policy, which will
allow us to set an invalid config
Please leave a space after //, preferably end sentence with a .

https://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/environwatcher.go
File state/api/common/testing/environwatcher.go (right):

https://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/environwatcher.go#newcode92
state/api/common/testing/environwatcher.go:92: err =
s.st.UpdateEnvironConfig(map[string]interface{}{}, []string{"foo"}, nil)
check error

https://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/environwatcher.go#newcode94
state/api/common/testing/environwatcher.go:94: c.Logf("!!!!! %#v",
cf.AllAttrs()["logging-config"])
leftover logging

https://codereview.appspot.com/70190050/diff/100001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/70190050/diff/100001/state/state.go#newcode279
state/state.go:279: type ValidateFn func(updateAttrs
map[string]interface{}, removeAttrs []string, oldConfig *config.Config)
error
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).

https://codereview.appspot.com/70190050/diff/100001/state/state.go#newcode316
state/state.go:316: //use validCfg to update settings
Just drop this comment

https://codereview.appspot.com/70190050/diff/100001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/70190050/diff/100001/state/state_test.go#newcode1689
state/state_test.go:1689: if v, found := updateAttrs["agent-version"];
found {
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).

https://codereview.appspot.com/70190050/diff/100001/state/state_test.go#newcode1818
state/state_test.go:1818: err = settings.Update(nil, bson.D{{"$set",
bson.D{{"name", "foo"}}}})
What is this doing here? Is this because UpdateEnvironConfig won't
change the "name" attribute? If it's needed, you should probably be
using ...

Read more...

Revision history for this message
Jesse Meek (waigani) wrote :
Download full text (4.2 KiB)

Please take a look.

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go
File environs/statepolicy.go (right):

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go#newcode43
environs/statepolicy.go:43: return provider.(state.ConfigValidator), nil
On 2014/03/17 12:15:56, axw wrote:
> No need to type assert, since EnvironProvider implements
state.ConfigValidator.
> Just "return provider, nil".

Done.

https://codereview.appspot.com/70190050/diff/100001/environs/statepolicy.go#newcode44
environs/statepolicy.go:44: return nil,
errors.NewNotImplementedError("ConfigValidator")
On 2014/03/17 12:15:56, axw wrote:
> Lose the second return

Done.

https://codereview.appspot.com/70190050/diff/100001/juju/conn_test.go
File juju/conn_test.go (right):

https://codereview.appspot.com/70190050/diff/100001/juju/conn_test.go#newcode143
juju/conn_test.go:143: //Use a state without a nil policy, which will
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://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/environwatcher.go
File state/api/common/testing/environwatcher.go (right):

https://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/environwatcher.go#newcode92
state/api/common/testing/environwatcher.go:92: err =
s.st.UpdateEnvironConfig(map[string]interface{}{}, []string{"foo"}, nil)
On 2014/03/17 12:15:56, axw wrote:
> check error

Done.

https://codereview.appspot.com/70190050/diff/100001/state/api/common/testing/environwatcher.go#newcode94
state/api/common/testing/environwatcher.go:94: c.Logf("!!!!! %#v",
cf.AllAttrs()["logging-config"])
On 2014/03/17 12:15:56, axw wrote:
> leftover logging

Done.

https://codereview.appspot.com/70190050/diff/100001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/70190050/diff/100001/state/state.go#newcode279
state/state.go:279: type ValidateFn func(updateAttrs
map[string]interface{}, removeAttrs []string, oldConfig *config.Config)
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://codereview.appspot.com/70190050/diff/100001/state/state.go#newcode316
state/state.go:316: //use validCfg to update settings
On 2014/03/17 12:15:56, axw wrote:
> Just drop this comment

Done.

https://codereview.appspot.com/70190050/diff/100001/state/state_test.go
File state/state_test.go (right):

https://codereview.appspot.com/70190050/diff/100001/state/state_test.go#newcode1689
state/state_test.go:1689: if v, found := updateAttrs["agent-version"];
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 operati...

Read more...

Revision history for this message
Jesse Meek (waigani) wrote :
Revision history for this message
Andrew Wilkins (axwalk) wrote :

On 2014/03/17 22:48:37, waigani wrote:

LGTM. Thanks for slogging through it.

https://codereview.appspot.com/70190050/

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'cmd/juju/authorisedkeys_test.go'
2--- cmd/juju/authorisedkeys_test.go 2014-01-22 22:48:54 +0000
3+++ cmd/juju/authorisedkeys_test.go 2014-03-18 02:38:23 +0000
4@@ -13,7 +13,6 @@
5 jujutesting "launchpad.net/juju-core/juju/testing"
6 keymanagerserver "launchpad.net/juju-core/state/apiserver/keymanager"
7 keymanagertesting "launchpad.net/juju-core/state/apiserver/keymanager/testing"
8- statetesting "launchpad.net/juju-core/state/testing"
9 coretesting "launchpad.net/juju-core/testing"
10 "launchpad.net/juju-core/testing/testbase"
11 sshtesting "launchpad.net/juju-core/utils/ssh/testing"
12@@ -103,7 +102,7 @@
13
14 func (s *keySuiteBase) setAuthorisedKeys(c *gc.C, keys ...string) {
15 keyString := strings.Join(keys, "\n")
16- err := statetesting.UpdateConfig(s.State, map[string]interface{}{"authorized-keys": keyString})
17+ err := s.State.UpdateEnvironConfig(map[string]interface{}{"authorized-keys": keyString}, nil, nil)
18 c.Assert(err, gc.IsNil)
19 envConfig, err := s.State.EnvironConfig()
20 c.Assert(err, gc.IsNil)
21
22=== modified file 'cmd/juju/environment.go'
23--- cmd/juju/environment.go 2013-12-17 18:21:26 +0000
24+++ cmd/juju/environment.go 2014-03-18 02:38:23 +0000
25@@ -162,26 +162,8 @@
26 }
27 defer conn.Close()
28
29- // Here is the magic around setting the attributes:
30- // TODO(thumper): get this magic under test somewhere, and update other call-sites to use it.
31- // Get the existing environment config from the state.
32- oldConfig, err := conn.State.EnvironConfig()
33- if err != nil {
34- return err
35- }
36- // Apply the attributes specified for the command to the state config.
37- newConfig, err := oldConfig.Apply(c.values)
38- if err != nil {
39- return err
40- }
41- // Now validate this new config against the existing config via the provider.
42- provider := conn.Environ.Provider()
43- newProviderConfig, err := provider.Validate(newConfig, oldConfig)
44- if err != nil {
45- return err
46- }
47- // Now try to apply the new validated config.
48- return conn.State.SetEnvironConfig(newProviderConfig, oldConfig)
49+ // Update state config with new values
50+ return conn.State.UpdateEnvironConfig(c.values, nil, nil)
51 }
52
53 func (c *SetEnvironmentCommand) Run(ctx *cmd.Context) error {
54
55=== modified file 'cmd/juju/environment_test.go'
56--- cmd/juju/environment_test.go 2014-02-27 04:43:37 +0000
57+++ cmd/juju/environment_test.go 2014-03-18 02:38:23 +0000
58@@ -11,6 +11,7 @@
59
60 jujutesting "launchpad.net/juju-core/juju/testing"
61 "launchpad.net/juju-core/provider/dummy"
62+ _ "launchpad.net/juju-core/provider/local"
63 "launchpad.net/juju-core/testing"
64 )
65
66@@ -163,7 +164,7 @@
67
68 var immutableConfigTests = map[string]string{
69 "name": "foo",
70- "type": "foo",
71+ "type": "local",
72 "firewall-mode": "global",
73 "state-port": "1",
74 "api-port": "666",
75
76=== modified file 'cmd/juju/upgradejuju_test.go'
77--- cmd/juju/upgradejuju_test.go 2014-03-13 23:30:56 +0000
78+++ cmd/juju/upgradejuju_test.go 2014-03-18 02:38:23 +0000
79@@ -322,20 +322,16 @@
80 }
81
82 // Set up state and environ, and run the command.
83- oldcfg, err := s.State.EnvironConfig()
84- c.Assert(err, gc.IsNil)
85 toolsDir := c.MkDir()
86- cfg, err := oldcfg.Apply(map[string]interface{}{
87+ updateAttrs := map[string]interface{}{
88 "agent-version": test.agentVersion,
89 "tools-metadata-url": "file://" + toolsDir,
90- })
91- c.Assert(err, gc.IsNil)
92- err = s.State.SetEnvironConfig(cfg, oldcfg)
93+ }
94+ err := s.State.UpdateEnvironConfig(updateAttrs, nil, nil)
95 c.Assert(err, gc.IsNil)
96 versions := make([]version.Binary, len(test.tools))
97 for i, v := range test.tools {
98 versions[i] = version.MustParseBinary(v)
99-
100 }
101 if len(versions) > 0 {
102 envtesting.MustUploadFakeToolsVersions(s.Conn.Environ.Storage(), versions...)
103@@ -353,7 +349,7 @@
104 }
105
106 // Check expected changes to environ/state.
107- cfg, err = s.State.EnvironConfig()
108+ cfg, err := s.State.EnvironConfig()
109 c.Check(err, gc.IsNil)
110 agentVersion, ok := cfg.AgentVersion()
111 c.Check(ok, gc.Equals, true)
112@@ -408,14 +404,11 @@
113 func (s *UpgradeJujuSuite) Reset(c *gc.C) {
114 s.JujuConnSuite.Reset(c)
115 envtesting.RemoveTools(c, s.Conn.Environ.Storage())
116- oldcfg, err := s.State.EnvironConfig()
117- c.Assert(err, gc.IsNil)
118- cfg, err := oldcfg.Apply(map[string]interface{}{
119+ updateAttrs := map[string]interface{}{
120 "default-series": "raring",
121 "agent-version": "1.2.3",
122- })
123- c.Assert(err, gc.IsNil)
124- err = s.State.SetEnvironConfig(cfg, oldcfg)
125+ }
126+ err := s.State.UpdateEnvironConfig(updateAttrs, nil, nil)
127 c.Assert(err, gc.IsNil)
128 s.toolsDir = c.MkDir()
129 }
130
131=== modified file 'cmd/jujud/machine_test.go'
132--- cmd/jujud/machine_test.go 2014-03-13 07:54:56 +0000
133+++ cmd/jujud/machine_test.go 2014-03-18 02:38:23 +0000
134@@ -32,7 +32,6 @@
135 "launchpad.net/juju-core/state/api/params"
136 apirsyslog "launchpad.net/juju-core/state/api/rsyslog"
137 charmtesting "launchpad.net/juju-core/state/apiserver/charmrevisionupdater/testing"
138- statetesting "launchpad.net/juju-core/state/testing"
139 "launchpad.net/juju-core/state/watcher"
140 "launchpad.net/juju-core/testing"
141 coretesting "launchpad.net/juju-core/testing"
142@@ -635,7 +634,7 @@
143
144 // Update the keys in the environment.
145 sshKey := sshtesting.ValidKeyOne.Key + " user@host"
146- err := statetesting.UpdateConfig(s.BackingState, map[string]interface{}{"authorized-keys": sshKey})
147+ err := s.BackingState.UpdateEnvironConfig(map[string]interface{}{"authorized-keys": sshKey}, nil, nil)
148 c.Assert(err, gc.IsNil)
149
150 // Wait for ssh keys file to be updated.
151@@ -723,19 +722,15 @@
152
153 s.primeAgent(c, version.Current, state.JobHostUnits)
154 // Make sure there are some proxy settings to write.
155- oldConfig, err := s.State.EnvironConfig()
156- c.Assert(err, gc.IsNil)
157-
158 proxySettings := osenv.ProxySettings{
159 Http: "http proxy",
160 Https: "https proxy",
161 Ftp: "ftp proxy",
162 }
163
164- envConfig, err := oldConfig.Apply(config.ProxyConfigMap(proxySettings))
165- c.Assert(err, gc.IsNil)
166+ updateAttrs := config.ProxyConfigMap(proxySettings)
167
168- err = s.State.SetEnvironConfig(envConfig, oldConfig)
169+ err := s.State.UpdateEnvironConfig(updateAttrs, nil, nil)
170 c.Assert(err, gc.IsNil)
171
172 s.assertJobWithAPI(c, state.JobHostUnits, func(conf agent.Config, st *api.State) {
173
174=== modified file 'environs/config.go'
175--- environs/config.go 2014-03-05 19:41:34 +0000
176+++ environs/config.go 2014-03-18 02:38:23 +0000
177@@ -135,13 +135,13 @@
178 }
179
180 // Provider returns the previously registered provider with the given type.
181-func Provider(typ string) (EnvironProvider, error) {
182- if alias, ok := providerAliases[typ]; ok {
183- typ = alias
184+func Provider(providerType string) (EnvironProvider, error) {
185+ if alias, ok := providerAliases[providerType]; ok {
186+ providerType = alias
187 }
188- p, ok := providers[typ]
189+ p, ok := providers[providerType]
190 if !ok {
191- return nil, fmt.Errorf("no registered provider for %q", typ)
192+ return nil, fmt.Errorf("no registered provider for %q", providerType)
193 }
194 return p, nil
195 }
196
197=== modified file 'environs/config/config.go'
198--- environs/config/config.go 2014-03-14 03:44:08 +0000
199+++ environs/config/config.go 2014-03-18 02:38:23 +0000
200@@ -662,6 +662,15 @@
201 return allAttrs
202 }
203
204+// Remove returns a new configuration that has the attributes of c minus attrs.
205+func (c *Config) Remove(attrs []string) (*Config, error) {
206+ defined := c.AllAttrs()
207+ for _, k := range attrs {
208+ delete(defined, k)
209+ }
210+ return New(NoDefaults, defined)
211+}
212+
213 // Apply returns a new configuration that has the attributes of c plus attrs.
214 func (c *Config) Apply(attrs map[string]interface{}) (*Config, error) {
215 defined := c.AllAttrs()
216
217=== modified file 'environs/statepolicy.go'
218--- environs/statepolicy.go 2014-02-19 06:31:52 +0000
219+++ environs/statepolicy.go 2014-03-18 02:38:23 +0000
220@@ -27,8 +27,18 @@
221 if err != nil {
222 return nil, err
223 }
224+
225 if p, ok := env.(state.Prechecker); ok {
226 return p, nil
227 }
228 return nil, errors.NewNotImplementedError("Prechecker")
229 }
230+
231+func (environStatePolicy) ConfigValidator(providerType string) (state.ConfigValidator, error) {
232+ provider, err := Provider(providerType)
233+ if err != nil {
234+ return nil, err
235+ }
236+
237+ return provider, nil
238+}
239
240=== modified file 'environs/testing/tools.go'
241--- environs/testing/tools.go 2014-03-13 22:47:05 +0000
242+++ environs/testing/tools.go 2014-03-18 02:38:23 +0000
243@@ -442,12 +442,6 @@
244 }}
245
246 func SetSSLHostnameVerification(c *gc.C, st *state.State, SSLHostnameVerification bool) {
247- envConfig, err := st.EnvironConfig()
248- c.Assert(err, gc.IsNil)
249- attrs := envConfig.AllAttrs()
250- attrs["ssl-hostname-verification"] = SSLHostnameVerification
251- newConfig, err := config.New(config.NoDefaults, attrs)
252- c.Assert(err, gc.IsNil)
253- err = st.SetEnvironConfig(newConfig, envConfig)
254+ err := st.UpdateEnvironConfig(map[string]interface{}{"ssl-hostname-verification": SSLHostnameVerification}, nil, nil)
255 c.Assert(err, gc.IsNil)
256 }
257
258=== modified file 'juju/conn.go'
259--- juju/conn.go 2014-03-07 14:15:28 +0000
260+++ juju/conn.go 2014-03-18 02:38:23 +0000
261@@ -13,7 +13,6 @@
262
263 "launchpad.net/juju-core/charm"
264 "launchpad.net/juju-core/environs"
265- "launchpad.net/juju-core/environs/config"
266 "launchpad.net/juju-core/environs/configstore"
267 "launchpad.net/juju-core/errors"
268 "launchpad.net/juju-core/juju/osenv"
269@@ -144,20 +143,17 @@
270 if err != nil {
271 return err
272 }
273+ secretAttrs := make(map[string]interface{})
274 attrs := cfg.AllAttrs()
275 for k, v := range secrets {
276 if _, exists := attrs[k]; exists {
277 // Environment already has secrets. Won't send again.
278 return nil
279 } else {
280- attrs[k] = v
281+ secretAttrs[k] = v
282 }
283 }
284- newcfg, err := config.New(config.NoDefaults, attrs)
285- if err != nil {
286- return err
287- }
288- return c.State.SetEnvironConfig(newcfg, cfg)
289+ return c.State.UpdateEnvironConfig(secretAttrs, nil, nil)
290 }
291
292 // PutCharm uploads the given charm to provider storage, and adds a
293
294=== modified file 'juju/conn_test.go'
295--- juju/conn_test.go 2014-03-13 07:54:56 +0000
296+++ juju/conn_test.go 2014-03-18 02:38:23 +0000
297@@ -140,7 +140,8 @@
298 info, _, err := env.StateInfo()
299 c.Assert(err, gc.IsNil)
300 info.Password = utils.UserPasswordHash("side-effect secret", utils.CompatSalt)
301- st, err := state.Open(info, state.DefaultDialOpts(), environs.NewStatePolicy())
302+ // Use a state without a nil policy, which will allow us to set an invalid config.
303+ st, err := state.Open(info, state.DefaultDialOpts(), state.Policy(nil))
304 c.Assert(err, gc.IsNil)
305 defer assertClose(c, st)
306
307@@ -151,15 +152,8 @@
308
309 // Remove the secret from state, and then make sure it gets
310 // pushed back again.
311- attrs = statecfg.AllAttrs()
312- delete(attrs, "secret")
313- newcfg, err := config.New(config.NoDefaults, attrs)
314- c.Assert(err, gc.IsNil)
315- err = st.SetEnvironConfig(newcfg, statecfg)
316- c.Assert(err, gc.IsNil)
317- statecfg, err = st.EnvironConfig()
318- c.Assert(err, gc.IsNil)
319- c.Assert(statecfg.UnknownAttrs()["secret"], gc.IsNil)
320+ err = st.UpdateEnvironConfig(map[string]interface{}{}, []string{"secret"}, nil)
321+ c.Assert(err, gc.IsNil)
322
323 // Make a new Conn, which will push the secrets.
324 conn, err := juju.NewConn(env)
325
326=== modified file 'juju/testing/conn.go'
327--- juju/testing/conn.go 2014-02-21 03:57:49 +0000
328+++ juju/testing/conn.go 2014-03-18 02:38:23 +0000
329@@ -62,6 +62,7 @@
330 oldHome string
331 oldJujuHome string
332 environ environs.Environ
333+ DummyConfig testing.Attrs
334 }
335
336 const AdminSecret = "dummy-secret"
337@@ -220,7 +221,10 @@
338 }
339
340 func (s *JujuConnSuite) writeSampleConfig(c *gc.C, path string) {
341- attrs := dummy.SampleConfig().Merge(testing.Attrs{
342+ if s.DummyConfig == nil {
343+ s.DummyConfig = dummy.SampleConfig()
344+ }
345+ attrs := s.DummyConfig.Merge(testing.Attrs{
346 "admin-secret": AdminSecret,
347 "agent-version": version.Current.Number.String(),
348 }).Delete("name")
349
350=== modified file 'juju/testing/repo.go'
351--- juju/testing/repo.go 2014-01-28 17:07:55 +0000
352+++ juju/testing/repo.go 2014-03-18 02:38:23 +0000
353@@ -25,11 +25,8 @@
354 s.JujuConnSuite.SetUpTest(c)
355 // Change the environ's config to ensure we're using the one in state,
356 // not the one in the local environments.yaml
357- oldcfg, err := s.State.EnvironConfig()
358- c.Assert(err, gc.IsNil)
359- cfg, err := oldcfg.Apply(map[string]interface{}{"default-series": "precise"})
360- c.Assert(err, gc.IsNil)
361- err = s.State.SetEnvironConfig(cfg, oldcfg)
362+ updateAttrs := map[string]interface{}{"default-series": "precise"}
363+ err := s.State.UpdateEnvironConfig(updateAttrs, nil, nil)
364 c.Assert(err, gc.IsNil)
365 s.RepoPath = os.Getenv("JUJU_REPOSITORY")
366 repoPath := c.MkDir()
367
368=== modified file 'juju/testing/utils.go'
369--- juju/testing/utils.go 2014-01-22 19:28:08 +0000
370+++ juju/testing/utils.go 2014-03-18 02:38:23 +0000
371@@ -6,24 +6,10 @@
372 import (
373 gc "launchpad.net/gocheck"
374
375- "launchpad.net/juju-core/environs/config"
376 "launchpad.net/juju-core/instance"
377 "launchpad.net/juju-core/state"
378- coretesting "launchpad.net/juju-core/testing"
379 )
380
381-// ChangeEnvironConfig applies the given change function
382-// to the attributes from st.EnvironConfig and
383-// sets the state's environment configuration to the result.
384-func ChangeEnvironConfig(c *gc.C, st *state.State, change func(coretesting.Attrs) coretesting.Attrs) {
385- cfg, err := st.EnvironConfig()
386- c.Assert(err, gc.IsNil)
387- newCfg, err := config.New(config.NoDefaults, change(cfg.AllAttrs()))
388- c.Assert(err, gc.IsNil)
389- err = st.SetEnvironConfig(newCfg, cfg)
390- c.Assert(err, gc.IsNil)
391-}
392-
393 // AddStateServerMachine adds a "state server" machine to the state so
394 // that State.Addresses and State.APIAddresses will work. It returns the
395 // added machine. The addresses that those methods will return bear no
396
397=== modified file 'state/api/common/testing/environwatcher.go'
398--- state/api/common/testing/environwatcher.go 2014-03-13 07:54:56 +0000
399+++ state/api/common/testing/environwatcher.go 2014-03-18 02:38:23 +0000
400@@ -76,17 +76,26 @@
401 // Initial event.
402 wc.AssertOneChange()
403
404- // Change the environment configuration, check it's detected.
405- attrs := envConfig.AllAttrs()
406- attrs["type"] = "blah"
407- newConfig, err := config.New(config.NoDefaults, attrs)
408- c.Assert(err, gc.IsNil)
409- err = s.st.SetEnvironConfig(newConfig, envConfig)
410+ // Change the environment configuration by updating an existing attribute, check it's detected.
411+ newAttrs := map[string]interface{}{"logging-config": "juju=ERROR"}
412+ err = s.st.UpdateEnvironConfig(newAttrs, nil, nil)
413+ c.Assert(err, gc.IsNil)
414+ wc.AssertOneChange()
415+
416+ // Change the environment configuration by adding a new attribute, check it's detected.
417+ newAttrs = map[string]interface{}{"foo": "bar"}
418+ err = s.st.UpdateEnvironConfig(newAttrs, nil, nil)
419+ c.Assert(err, gc.IsNil)
420+ wc.AssertOneChange()
421+
422+ // Change the environment configuration by removing an attribute, check it's detected.
423+ err = s.st.UpdateEnvironConfig(map[string]interface{}{}, []string{"foo"}, nil)
424 c.Assert(err, gc.IsNil)
425 wc.AssertOneChange()
426
427 // Change it back to the original config.
428- err = s.st.SetEnvironConfig(envConfig, newConfig)
429+ oldAttrs := map[string]interface{}{"logging-config": envConfig.AllAttrs()["logging-config"]}
430+ err = s.st.UpdateEnvironConfig(oldAttrs, nil, nil)
431 c.Assert(err, gc.IsNil)
432 wc.AssertOneChange()
433
434
435=== modified file 'state/api/firewaller/state_test.go'
436--- state/api/firewaller/state_test.go 2014-03-13 07:54:56 +0000
437+++ state/api/firewaller/state_test.go 2014-03-18 02:38:23 +0000
438@@ -7,7 +7,6 @@
439 jc "github.com/juju/testing/checkers"
440 gc "launchpad.net/gocheck"
441
442- "launchpad.net/juju-core/environs/config"
443 "launchpad.net/juju-core/instance"
444 "launchpad.net/juju-core/state"
445 statetesting "launchpad.net/juju-core/state/testing"
446@@ -81,16 +80,14 @@
447 wc.AssertOneChange()
448
449 // Change the environment configuration, check it's detected.
450- attrs := envConfig.AllAttrs()
451- attrs["type"] = "blah"
452- newConfig, err := config.New(config.NoDefaults, attrs)
453- c.Assert(err, gc.IsNil)
454- err = s.State.SetEnvironConfig(newConfig, envConfig)
455+ newAttrs := map[string]interface{}{"logging-config": "juju=ERROR"}
456+ err = s.State.UpdateEnvironConfig(newAttrs, nil, nil)
457 c.Assert(err, gc.IsNil)
458 wc.AssertOneChange()
459
460 // Change it back to the original config.
461- err = s.State.SetEnvironConfig(envConfig, newConfig)
462+ oldAttrs := map[string]interface{}{"logging-config": envConfig.AllAttrs()["logging-config"]}
463+ err = s.State.UpdateEnvironConfig(oldAttrs, nil, nil)
464 c.Assert(err, gc.IsNil)
465 wc.AssertOneChange()
466
467
468=== modified file 'state/api/keymanager/client_test.go'
469--- state/api/keymanager/client_test.go 2014-03-10 13:13:43 +0000
470+++ state/api/keymanager/client_test.go 2014-03-18 02:38:23 +0000
471@@ -14,7 +14,6 @@
472 "launchpad.net/juju-core/state/api/params"
473 keymanagerserver "launchpad.net/juju-core/state/apiserver/keymanager"
474 keymanagertesting "launchpad.net/juju-core/state/apiserver/keymanager/testing"
475- "launchpad.net/juju-core/state/testing"
476 "launchpad.net/juju-core/utils/ssh"
477 sshtesting "launchpad.net/juju-core/utils/ssh/testing"
478 )
479@@ -35,7 +34,7 @@
480 }
481
482 func (s *keymanagerSuite) setAuthorisedKeys(c *gc.C, keys string) {
483- err := testing.UpdateConfig(s.BackingState, map[string]interface{}{"authorized-keys": keys})
484+ err := s.BackingState.UpdateEnvironConfig(map[string]interface{}{"authorized-keys": keys}, nil, nil)
485 c.Assert(err, gc.IsNil)
486 }
487
488
489=== modified file 'state/api/keyupdater/authorisedkeys_test.go'
490--- state/api/keyupdater/authorisedkeys_test.go 2014-01-24 12:39:48 +0000
491+++ state/api/keyupdater/authorisedkeys_test.go 2014-03-18 02:38:23 +0000
492@@ -54,7 +54,7 @@
493 }
494
495 func (s *keyupdaterSuite) setAuthorisedKeys(c *gc.C, keys string) {
496- err := testing.UpdateConfig(s.BackingState, map[string]interface{}{"authorized-keys": keys})
497+ err := s.BackingState.UpdateEnvironConfig(map[string]interface{}{"authorized-keys": keys}, nil, nil)
498 c.Assert(err, gc.IsNil)
499 }
500
501
502=== modified file 'state/api/logger/logger_test.go'
503--- state/api/logger/logger_test.go 2013-09-17 03:12:12 +0000
504+++ state/api/logger/logger_test.go 2014-03-18 02:38:23 +0000
505@@ -50,7 +50,7 @@
506 }
507
508 func (s *loggerSuite) setLoggingConfig(c *gc.C, loggingConfig string) {
509- err := testing.UpdateConfig(s.BackingState, map[string]interface{}{"logging-config": loggingConfig})
510+ err := s.BackingState.UpdateEnvironConfig(map[string]interface{}{"logging-config": loggingConfig}, nil, nil)
511 c.Assert(err, gc.IsNil)
512 }
513
514
515=== modified file 'state/apiserver/client/client.go'
516--- state/apiserver/client/client.go 2014-03-14 10:28:40 +0000
517+++ state/apiserver/client/client.go 2014-03-18 02:38:23 +0000
518@@ -761,38 +761,23 @@
519 // EnvironmentSet implements the server-side part of the
520 // set-environment CLI command.
521 func (c *Client) EnvironmentSet(args params.EnvironmentSet) error {
522- // TODO(dimitern,thumper): 2013-11-06 bug #1167616
523- // SetEnvironConfig should take both new and old configs.
524-
525- // Get the existing environment config from the state.
526- oldConfig, err := c.api.state.EnvironConfig()
527- if err != nil {
528- return err
529- }
530 // Make sure we don't allow changing agent-version.
531- if v, found := args.Config["agent-version"]; found {
532- oldVersion, _ := oldConfig.AgentVersion()
533- if v != oldVersion.String() {
534- return fmt.Errorf("agent-version cannot be changed")
535+ checkAgentVersion := func(updateAttrs map[string]interface{}, removeAttrs []string, oldConfig *config.Config) error {
536+ if v, found := updateAttrs["agent-version"]; found {
537+ oldVersion, _ := oldConfig.AgentVersion()
538+ if v != oldVersion.String() {
539+ return fmt.Errorf("agent-version cannot be changed")
540+ }
541 }
542- }
543- // Apply the attributes specified for the command to the state config.
544- newConfig, err := oldConfig.Apply(args.Config)
545- if err != nil {
546- return err
547- }
548- env, err := environs.New(oldConfig)
549- if err != nil {
550- return err
551- }
552- // Now validate this new config against the existing config via the provider.
553- provider := env.Provider()
554- newProviderConfig, err := provider.Validate(newConfig, oldConfig)
555- if err != nil {
556- return err
557- }
558- // Now try to apply the new validated config.
559- return c.api.state.SetEnvironConfig(newProviderConfig, oldConfig)
560+ return nil
561+ }
562+
563+ // TODO(waigani) 2014-3-11 #1167616
564+ // Add a txn retry loop to ensure that the settings on disk have not
565+ // changed underneath us.
566+
567+ return c.api.state.UpdateEnvironConfig(args.Config, nil, checkAgentVersion)
568+
569 }
570
571 // SetEnvironAgentVersion sets the environment agent version.
572
573=== modified file 'state/apiserver/client/client_test.go'
574--- state/apiserver/client/client_test.go 2014-03-14 10:28:40 +0000
575+++ state/apiserver/client/client_test.go 2014-03-18 02:38:23 +0000
576@@ -1741,18 +1741,9 @@
577 store, restore := makeMockCharmStore()
578 defer restore()
579
580- oldConfig, err := s.State.EnvironConfig()
581- c.Assert(err, gc.IsNil)
582-
583- attrs := coretesting.Attrs(oldConfig.AllAttrs())
584- attrs = attrs.Merge(coretesting.Attrs{
585- "charm-store-auth": "token=value",
586- "test-mode": true})
587-
588- cfg, err := config.New(config.NoDefaults, attrs)
589- c.Assert(err, gc.IsNil)
590-
591- err = s.State.SetEnvironConfig(cfg, oldConfig)
592+ attrs := map[string]interface{}{"charm-store-auth": "token=value",
593+ "test-mode": true}
594+ err := s.State.UpdateEnvironConfig(attrs, nil, nil)
595 c.Assert(err, gc.IsNil)
596
597 curl, _ := addCharm(c, store, "dummy")
598
599=== modified file 'state/apiserver/keymanager/keymanager.go'
600--- state/apiserver/keymanager/keymanager.go 2014-03-05 19:41:34 +0000
601+++ state/apiserver/keymanager/keymanager.go 2014-03-18 02:38:23 +0000
602@@ -144,14 +144,14 @@
603 return keyInfo
604 }
605
606-func (api *KeyManagerAPI) writeSSHKeys(currentConfig *config.Config, sshKeys []string) error {
607+func (api *KeyManagerAPI) writeSSHKeys(sshKeys []string) error {
608 // Write out the new keys.
609 keyStr := strings.Join(sshKeys, "\n")
610- newConfig, err := currentConfig.Apply(map[string]interface{}{config.AuthKeysConfig: keyStr})
611- if err != nil {
612- return fmt.Errorf("creating new environ config: %v", err)
613- }
614- err = api.state.SetEnvironConfig(newConfig, currentConfig)
615+ attrs := map[string]interface{}{config.AuthKeysConfig: keyStr}
616+ // TODO(waigani) 2014-03-17 bug #1293324
617+ // Pass in validation to ensure SSH keys
618+ // have not changed underfoot
619+ err := api.state.UpdateEnvironConfig(attrs, nil, nil)
620 if err != nil {
621 return fmt.Errorf("writing environ config: %v", err)
622 }
623@@ -159,12 +159,12 @@
624 }
625
626 // currentKeyDataForAdd gathers data used when adding ssh keys.
627-func (api *KeyManagerAPI) currentKeyDataForAdd() (keys []string, fingerprints *set.Strings, cfg *config.Config, err error) {
628+func (api *KeyManagerAPI) currentKeyDataForAdd() (keys []string, fingerprints *set.Strings, err error) {
629 fp := set.NewStrings()
630 fingerprints = &fp
631- cfg, err = api.state.EnvironConfig()
632+ cfg, err := api.state.EnvironConfig()
633 if err != nil {
634- return nil, nil, nil, err
635+ return nil, nil, fmt.Errorf("reading current key data: %v", err)
636 }
637 keys = ssh.SplitAuthorisedKeys(cfg.AuthorizedKeys())
638 for _, key := range keys {
639@@ -174,7 +174,7 @@
640 }
641 fingerprints.Add(fingerprint)
642 }
643- return keys, fingerprints, cfg, nil
644+ return keys, fingerprints, nil
645 }
646
647 // AddKeys adds new authorised ssh keys for the specified user.
648@@ -195,7 +195,7 @@
649 }
650
651 // For now, authorised keys are global, common to all users.
652- sshKeys, currentFingerprints, currentConfig, err := api.currentKeyDataForAdd()
653+ sshKeys, currentFingerprints, err := api.currentKeyDataForAdd()
654 if err != nil {
655 return params.ErrorResults{}, common.ServerError(fmt.Errorf("reading current key data: %v", err))
656 }
657@@ -214,7 +214,7 @@
658 }
659 sshKeys = append(sshKeys, key)
660 }
661- err = api.writeSSHKeys(currentConfig, sshKeys)
662+ err = api.writeSSHKeys(sshKeys)
663 if err != nil {
664 return params.ErrorResults{}, common.ServerError(err)
665 }
666@@ -278,7 +278,7 @@
667 }
668
669 // For now, authorised keys are global, common to all users.
670- sshKeys, currentFingerprints, currentConfig, err := api.currentKeyDataForAdd()
671+ sshKeys, currentFingerprints, err := api.currentKeyDataForAdd()
672 if err != nil {
673 return params.ErrorResults{}, common.ServerError(fmt.Errorf("reading current key data: %v", err))
674 }
675@@ -297,7 +297,7 @@
676 }
677 sshKeys = append(sshKeys, keyInfo.key)
678 }
679- err = api.writeSSHKeys(currentConfig, sshKeys)
680+ err = api.writeSSHKeys(sshKeys)
681 if err != nil {
682 return params.ErrorResults{}, common.ServerError(err)
683 }
684@@ -306,13 +306,13 @@
685
686 // currentKeyDataForAdd gathers data used when deleting ssh keys.
687 func (api *KeyManagerAPI) currentKeyDataForDelete() (
688- keys map[string]string, invalidKeys []string, comments map[string]string, cfg *config.Config, err error) {
689+ keys map[string]string, invalidKeys []string, comments map[string]string, err error) {
690
691- // For now, authorised keys are global, common to all users.
692- cfg, err = api.state.EnvironConfig()
693+ cfg, err := api.state.EnvironConfig()
694 if err != nil {
695- return nil, nil, nil, nil, fmt.Errorf("reading current key data: %v", err)
696+ return nil, nil, nil, fmt.Errorf("reading current key data: %v", err)
697 }
698+ // For now, authorised keys are global, common to all users.
699 existingSSHKeys := ssh.SplitAuthorisedKeys(cfg.AuthorizedKeys())
700
701 // Build up a map of keys indexed by fingerprint, and fingerprints indexed by comment
702@@ -332,7 +332,7 @@
703 comments[comment] = fingerprint
704 }
705 }
706- return keys, invalidKeys, comments, cfg, nil
707+ return keys, invalidKeys, comments, nil
708 }
709
710 // DeleteKeys deletes the authorised ssh keys for the specified user.
711@@ -352,7 +352,7 @@
712 return params.ErrorResults{}, common.ServerError(common.ErrPerm)
713 }
714
715- sshKeys, invalidKeys, keyComments, currentConfig, err := api.currentKeyDataForDelete()
716+ sshKeys, invalidKeys, keyComments, err := api.currentKeyDataForDelete()
717 if err != nil {
718 return params.ErrorResults{}, common.ServerError(fmt.Errorf("reading current key data: %v", err))
719 }
720@@ -382,7 +382,7 @@
721 return params.ErrorResults{}, common.ServerError(stderrors.New("cannot delete all keys"))
722 }
723
724- err = api.writeSSHKeys(currentConfig, keysToWrite)
725+ err = api.writeSSHKeys(keysToWrite)
726 if err != nil {
727 return params.ErrorResults{}, common.ServerError(err)
728 }
729
730=== modified file 'state/apiserver/keymanager/keymanager_test.go'
731--- state/apiserver/keymanager/keymanager_test.go 2014-03-10 13:13:43 +0000
732+++ state/apiserver/keymanager/keymanager_test.go 2014-03-18 02:38:23 +0000
733@@ -16,7 +16,6 @@
734 "launchpad.net/juju-core/state/apiserver/keymanager"
735 keymanagertesting "launchpad.net/juju-core/state/apiserver/keymanager/testing"
736 apiservertesting "launchpad.net/juju-core/state/apiserver/testing"
737- statetesting "launchpad.net/juju-core/state/testing"
738 "launchpad.net/juju-core/utils/ssh"
739 sshtesting "launchpad.net/juju-core/utils/ssh/testing"
740 )
741@@ -78,7 +77,7 @@
742 }
743
744 func (s *keyManagerSuite) setAuthorisedKeys(c *gc.C, keys string) {
745- err := statetesting.UpdateConfig(s.State, map[string]interface{}{"authorized-keys": keys})
746+ err := s.State.UpdateEnvironConfig(map[string]interface{}{"authorized-keys": keys}, nil, nil)
747 c.Assert(err, gc.IsNil)
748 envConfig, err := s.State.EnvironConfig()
749 c.Assert(err, gc.IsNil)
750
751=== modified file 'state/apiserver/keyupdater/authorisedkeys_test.go'
752--- state/apiserver/keyupdater/authorisedkeys_test.go 2013-12-12 05:12:49 +0000
753+++ state/apiserver/keyupdater/authorisedkeys_test.go 2014-03-18 02:38:23 +0000
754@@ -73,7 +73,7 @@
755 }
756
757 func (s *authorisedKeysSuite) setAuthorizedKeys(c *gc.C, keys string) {
758- err := statetesting.UpdateConfig(s.State, map[string]interface{}{"authorized-keys": keys})
759+ err := s.State.UpdateEnvironConfig(map[string]interface{}{"authorized-keys": keys}, nil, nil)
760 c.Assert(err, gc.IsNil)
761 envConfig, err := s.State.EnvironConfig()
762 c.Assert(err, gc.IsNil)
763
764=== modified file 'state/apiserver/logger/logger_test.go'
765--- state/apiserver/logger/logger_test.go 2014-01-22 02:08:33 +0000
766+++ state/apiserver/logger/logger_test.go 2014-03-18 02:38:23 +0000
767@@ -74,7 +74,7 @@
768 }
769
770 func (s *loggerSuite) setLoggingConfig(c *gc.C, loggingConfig string) {
771- err := statetesting.UpdateConfig(s.State, map[string]interface{}{"logging-config": loggingConfig})
772+ err := s.State.UpdateEnvironConfig(map[string]interface{}{"logging-config": loggingConfig}, nil, nil)
773 c.Assert(err, gc.IsNil)
774 envConfig, err := s.State.EnvironConfig()
775 c.Assert(err, gc.IsNil)
776
777=== modified file 'state/apiserver/provisioner/provisioner_test.go'
778--- state/apiserver/provisioner/provisioner_test.go 2014-03-17 13:22:55 +0000
779+++ state/apiserver/provisioner/provisioner_test.go 2014-03-18 02:38:23 +0000
780@@ -683,13 +683,10 @@
781 }
782
783 func (s *withoutStateServerSuite) TestContainerConfig(c *gc.C) {
784- cfg, err := s.State.EnvironConfig()
785- c.Assert(err, gc.IsNil)
786- newCfg, err := cfg.Apply(map[string]interface{}{
787+ attrs := map[string]interface{}{
788 "http-proxy": "http://proxy.example.com:9000",
789- })
790- c.Assert(err, gc.IsNil)
791- err = s.State.SetEnvironConfig(newCfg, cfg)
792+ }
793+ err := s.State.UpdateEnvironConfig(attrs, nil, nil)
794 c.Assert(err, gc.IsNil)
795 expectedProxy := osenv.ProxySettings{
796 Http: "http://proxy.example.com:9000",
797
798=== modified file 'state/apiserver/rsyslog/rsyslog.go'
799--- state/apiserver/rsyslog/rsyslog.go 2014-02-27 03:46:35 +0000
800+++ state/apiserver/rsyslog/rsyslog.go 2014-03-18 02:38:23 +0000
801@@ -40,17 +40,9 @@
802 result.Error = common.ServerError(err)
803 return result, nil
804 }
805- old, err := api.st.EnvironConfig()
806- if err != nil {
807- return params.ErrorResult{}, err
808- }
809- cfg, err := old.Apply(map[string]interface{}{"rsyslog-ca-cert": string(args.CACert)})
810- if err != nil {
811+ attrs := map[string]interface{}{"rsyslog-ca-cert": string(args.CACert)}
812+ if err := api.st.UpdateEnvironConfig(attrs, nil, nil); err != nil {
813 result.Error = common.ServerError(err)
814- } else {
815- if err := api.st.SetEnvironConfig(cfg, old); err != nil {
816- result.Error = common.ServerError(err)
817- }
818 }
819 return result, nil
820 }
821
822=== added file 'state/configvalidator_test.go'
823--- state/configvalidator_test.go 1970-01-01 00:00:00 +0000
824+++ state/configvalidator_test.go 2014-03-18 02:38:23 +0000
825@@ -0,0 +1,114 @@
826+// Copyright 2014 Canonical Ltd.
827+// Licensed under the AGPLv3, see LICENCE file for details.
828+
829+package state_test
830+
831+import (
832+ gc "launchpad.net/gocheck"
833+ "launchpad.net/juju-core/environs/config"
834+ "launchpad.net/juju-core/errors"
835+ "launchpad.net/juju-core/state"
836+ coretesting "launchpad.net/juju-core/testing"
837+)
838+
839+type ConfigValidatorSuite struct {
840+ ConnSuite
841+ configValidator mockConfigValidator
842+}
843+
844+var _ = gc.Suite(&ConfigValidatorSuite{})
845+
846+type mockConfigValidator struct {
847+ validateError error
848+ validateCfg *config.Config
849+ validateOld *config.Config
850+ validateValid *config.Config
851+}
852+
853+// To test UpdateEnvironConfig updates state, Validate returns a config
854+// different to both input configs
855+func mockValidCfg() (valid *config.Config, err error) {
856+ cfg, err := config.New(config.UseDefaults, coretesting.FakeConfig())
857+ if err != nil {
858+ return nil, err
859+ }
860+ valid, err = cfg.Apply(map[string]interface{}{
861+ "arbitrary-key": "cptn-marvel",
862+ })
863+ if err != nil {
864+ return nil, err
865+ }
866+ return valid, nil
867+}
868+
869+func (p *mockConfigValidator) Validate(cfg, old *config.Config) (valid *config.Config, err error) {
870+ p.validateCfg = cfg
871+ p.validateOld = old
872+ p.validateValid, p.validateError = mockValidCfg()
873+ return p.validateValid, p.validateError
874+}
875+
876+func (s *ConfigValidatorSuite) SetUpTest(c *gc.C) {
877+ s.ConnSuite.SetUpTest(c)
878+ s.configValidator = mockConfigValidator{}
879+ s.policy.getConfigValidator = func(string) (state.ConfigValidator, error) {
880+ return &s.configValidator, nil
881+ }
882+}
883+
884+func (s *ConfigValidatorSuite) updateEnvironConfig(c *gc.C) error {
885+ updateAttrs := map[string]interface{}{
886+ "authorized-keys": "different-keys",
887+ "arbitrary-key": "shazam!",
888+ }
889+ return s.State.UpdateEnvironConfig(updateAttrs, nil, nil)
890+}
891+
892+func (s *ConfigValidatorSuite) TestConfigValidate(c *gc.C) {
893+ err := s.updateEnvironConfig(c)
894+ c.Assert(err, gc.IsNil)
895+}
896+
897+func (s *ConfigValidatorSuite) TestUpdateEnvironConfigFailsOnConfigValidateError(c *gc.C) {
898+ var configValidatorErr error
899+ s.policy.getConfigValidator = func(string) (state.ConfigValidator, error) {
900+ configValidatorErr = errors.NotFoundf("")
901+ return &s.configValidator, configValidatorErr
902+ }
903+
904+ err := s.updateEnvironConfig(c)
905+ c.Assert(err, gc.ErrorMatches, " not found")
906+}
907+
908+func (s *ConfigValidatorSuite) TestUpdateEnvironConfigUpdatesState(c *gc.C) {
909+ s.updateEnvironConfig(c)
910+ stateCfg, err := s.State.EnvironConfig()
911+ c.Assert(err, gc.IsNil)
912+ newValidCfg, err := mockValidCfg()
913+ c.Assert(err, gc.IsNil)
914+ c.Assert(stateCfg.AllAttrs()["arbitrary-key"], gc.Equals, newValidCfg.AllAttrs()["arbitrary-key"])
915+}
916+
917+func (s *ConfigValidatorSuite) TestConfigValidateUnimplemented(c *gc.C) {
918+ var configValidatorErr error
919+ s.policy.getConfigValidator = func(string) (state.ConfigValidator, error) {
920+ return nil, configValidatorErr
921+ }
922+
923+ err := s.updateEnvironConfig(c)
924+ c.Assert(err, gc.ErrorMatches, "policy returned nil configValidator without an error")
925+ configValidatorErr = errors.NewNotImplementedError("Validator")
926+ err = s.updateEnvironConfig(c)
927+ c.Assert(err, gc.IsNil)
928+}
929+
930+func (s *ConfigValidatorSuite) TestConfigValidateNoPolicy(c *gc.C) {
931+ s.policy.getConfigValidator = func(providerType string) (state.ConfigValidator, error) {
932+ c.Errorf("should not have been invoked")
933+ return nil, nil
934+ }
935+
936+ state.SetPolicy(s.State, nil)
937+ err := s.updateEnvironConfig(c)
938+ c.Assert(err, gc.IsNil)
939+}
940
941=== modified file 'state/conn_test.go'
942--- state/conn_test.go 2014-03-13 13:42:50 +0000
943+++ state/conn_test.go 2014-03-18 02:38:23 +0000
944@@ -97,7 +97,8 @@
945 }
946
947 type mockPolicy struct {
948- getPrechecker func(*config.Config) (state.Prechecker, error)
949+ getPrechecker func(*config.Config) (state.Prechecker, error)
950+ getConfigValidator func(string) (state.ConfigValidator, error)
951 }
952
953 func (p *mockPolicy) Prechecker(cfg *config.Config) (state.Prechecker, error) {
954@@ -106,3 +107,10 @@
955 }
956 return nil, errors.NewNotImplementedError("Prechecker")
957 }
958+
959+func (p *mockPolicy) ConfigValidator(providerType string) (state.ConfigValidator, error) {
960+ if p.getConfigValidator != nil {
961+ return p.getConfigValidator(providerType)
962+ }
963+ return nil, errors.NewNotImplementedError("ConfigValidator")
964+}
965
966=== modified file 'state/initialize_test.go'
967--- state/initialize_test.go 2014-03-17 16:12:00 +0000
968+++ state/initialize_test.go 2014-03-18 02:38:23 +0000
969@@ -110,17 +110,18 @@
970 func (s *InitializeSuite) TestEnvironConfigWithAdminSecret(c *gc.C) {
971 // admin-secret blocks Initialize.
972 good := testing.EnvironConfig(c)
973- bad, err := good.Apply(map[string]interface{}{"admin-secret": "foo"})
974+ badUpdateAttrs := map[string]interface{}{"admin-secret": "foo"}
975+ bad, err := good.Apply(badUpdateAttrs)
976
977 _, err = state.Initialize(state.TestingStateInfo(), bad, state.TestingDialOpts(), state.Policy(nil))
978 c.Assert(err, gc.ErrorMatches, "admin-secret should never be written to the state")
979
980- // admin-secret blocks SetEnvironConfig.
981+ // admin-secret blocks UpdateEnvironConfig.
982 st := state.TestingInitialize(c, good, state.Policy(nil))
983 st.Close()
984
985 s.openState(c)
986- err = s.State.SetEnvironConfig(bad, good)
987+ err = s.State.UpdateEnvironConfig(badUpdateAttrs, nil, nil)
988 c.Assert(err, gc.ErrorMatches, "admin-secret should never be written to the state")
989
990 // EnvironConfig remains inviolate.
991@@ -140,12 +141,11 @@
992 _, err = state.Initialize(state.TestingStateInfo(), bad, state.TestingDialOpts(), state.Policy(nil))
993 c.Assert(err, gc.ErrorMatches, "agent-version must always be set in state")
994
995- // Bad agent-version blocks SetEnvironConfig.
996 st := state.TestingInitialize(c, good, state.Policy(nil))
997 st.Close()
998
999 s.openState(c)
1000- err = s.State.SetEnvironConfig(bad, good)
1001+ err = s.State.UpdateEnvironConfig(map[string]interface{}{}, []string{"agent-version"}, nil)
1002 c.Assert(err, gc.ErrorMatches, "agent-version must always be set in state")
1003
1004 // EnvironConfig remains inviolate.
1005
1006=== modified file 'state/machine_test.go'
1007--- state/machine_test.go 2014-03-13 07:54:56 +0000
1008+++ state/machine_test.go 2014-03-18 02:38:23 +0000
1009@@ -79,9 +79,8 @@
1010 manual, err := s.machine0.IsManual()
1011 c.Assert(err, gc.IsNil)
1012 c.Assert(manual, jc.IsFalse)
1013- newcfg, err := cfg.Apply(map[string]interface{}{"type": "null"})
1014- c.Assert(err, gc.IsNil)
1015- err = s.State.SetEnvironConfig(newcfg, cfg)
1016+ attrs := map[string]interface{}{"type": "null"}
1017+ err = s.State.UpdateEnvironConfig(attrs, nil, nil)
1018 c.Assert(err, gc.IsNil)
1019 manual, err = s.machine0.IsManual()
1020 c.Assert(err, gc.IsNil)
1021
1022=== modified file 'state/policy.go'
1023--- state/policy.go 2014-02-20 08:11:19 +0000
1024+++ state/policy.go 2014-03-18 02:38:23 +0000
1025@@ -24,6 +24,10 @@
1026 // Prechecker takes a *config.Config and returns
1027 // a (possibly nil) Prechecker or an error.
1028 Prechecker(*config.Config) (Prechecker, error)
1029+
1030+ // ConfigValidator takes a string (environ type) and returns
1031+ // a (possibly nil) ConfigValidator or an error.
1032+ ConfigValidator(string) (ConfigValidator, error)
1033 }
1034
1035 // Prechecker is a policy interface that is provided to State
1036@@ -40,6 +44,12 @@
1037 PrecheckInstance(series string, cons constraints.Value) error
1038 }
1039
1040+// ConfigValidator is a policy interface that is provided to State
1041+// to check validity of new configuration attributes before applying them to state.
1042+type ConfigValidator interface {
1043+ Validate(cfg, old *config.Config) (valid *config.Config, err error)
1044+}
1045+
1046 // precheckInstance calls the state's assigned policy, if non-nil, to obtain
1047 // a Prechecker, and calls PrecheckInstance if a non-nil Prechecker is returned.
1048 func (st *State) precheckInstance(series string, cons constraints.Value) error {
1049@@ -61,3 +71,21 @@
1050 }
1051 return prechecker.PrecheckInstance(series, cons)
1052 }
1053+
1054+// validate calls the state's assigned policy, if non-nil, to obtain
1055+// a Validator, and calls validate if a non-nil Validator is returned.
1056+func (st *State) validate(cfg, old *config.Config) (valid *config.Config, err error) {
1057+ if st.policy == nil {
1058+ return cfg, nil
1059+ }
1060+ configValidator, err := st.policy.ConfigValidator(cfg.Type())
1061+ if errors.IsNotImplementedError(err) {
1062+ return cfg, nil
1063+ } else if err != nil {
1064+ return nil, err
1065+ }
1066+ if configValidator == nil {
1067+ return nil, fmt.Errorf("policy returned nil configValidator without an error")
1068+ }
1069+ return configValidator.Validate(cfg, old)
1070+}
1071
1072=== modified file 'state/state.go'
1073--- state/state.go 2014-03-13 13:42:50 +0000
1074+++ state/state.go 2014-03-18 02:38:23 +0000
1075@@ -259,12 +259,33 @@
1076 return ErrExcessiveContention
1077 }
1078
1079-// SetEnvironConfig replaces the current configuration of the
1080-// environment with the provided configuration.
1081-func (st *State) SetEnvironConfig(cfg, old *config.Config) error {
1082- if err := checkEnvironConfig(cfg); err != nil {
1083- return err
1084- }
1085+func (st *State) buildAndValidateEnvironConfig(updateAttrs map[string]interface{}, removeAttrs []string, oldConfig *config.Config) (validCfg *config.Config, err error) {
1086+ newConfig, err := oldConfig.Apply(updateAttrs)
1087+ if err != nil {
1088+ return nil, err
1089+ }
1090+ if len(removeAttrs) != 0 {
1091+ newConfig, err = newConfig.Remove(removeAttrs)
1092+ if err != nil {
1093+ return nil, err
1094+ }
1095+ }
1096+ if err := checkEnvironConfig(newConfig); err != nil {
1097+ return nil, err
1098+ }
1099+ return st.validate(newConfig, oldConfig)
1100+}
1101+
1102+type ValidateConfigFunc func(updateAttrs map[string]interface{}, removeAttrs []string, oldConfig *config.Config) error
1103+
1104+// UpateEnvironConfig adds, updates or removes attributes in the current
1105+// configuration of the environment with the provided updateAttrs and
1106+// removeAttrs.
1107+func (st *State) UpdateEnvironConfig(updateAttrs map[string]interface{}, removeAttrs []string, additionalValidation ValidateConfigFunc) error {
1108+ if len(updateAttrs)+len(removeAttrs) == 0 {
1109+ return nil
1110+ }
1111+
1112 // TODO(axw) 2013-12-6 #1167616
1113 // Ensure that the settings on disk have not changed
1114 // underneath us. The settings changes are actually
1115@@ -275,13 +296,30 @@
1116 if err != nil {
1117 return err
1118 }
1119- newattrs := cfg.AllAttrs()
1120- for k, _ := range old.AllAttrs() {
1121- if _, ok := newattrs[k]; !ok {
1122+
1123+ // Get the existing environment config from state.
1124+ oldConfig, err := config.New(config.NoDefaults, settings.Map())
1125+ if err != nil {
1126+ return err
1127+ }
1128+ if additionalValidation != nil {
1129+ err = additionalValidation(updateAttrs, removeAttrs, oldConfig)
1130+ if err != nil {
1131+ return err
1132+ }
1133+ }
1134+ validCfg, err := st.buildAndValidateEnvironConfig(updateAttrs, removeAttrs, oldConfig)
1135+ if err != nil {
1136+ return err
1137+ }
1138+
1139+ validAttrs := validCfg.AllAttrs()
1140+ for k, _ := range oldConfig.AllAttrs() {
1141+ if _, ok := validAttrs[k]; !ok {
1142 settings.Delete(k)
1143 }
1144 }
1145- settings.Update(newattrs)
1146+ settings.Update(validAttrs)
1147 _, err = settings.Write()
1148 return err
1149 }
1150
1151=== modified file 'state/state_test.go'
1152--- state/state_test.go 2014-03-17 16:39:43 +0000
1153+++ state/state_test.go 2014-03-18 02:38:23 +0000
1154@@ -1250,18 +1250,20 @@
1155 }
1156
1157 func (s *StateSuite) TestEnvironConfig(c *gc.C) {
1158- cfg, err := s.State.EnvironConfig()
1159- c.Assert(err, gc.IsNil)
1160- change, err := cfg.Apply(map[string]interface{}{
1161+ attrs := map[string]interface{}{
1162 "authorized-keys": "different-keys",
1163 "arbitrary-key": "shazam!",
1164- })
1165- c.Assert(err, gc.IsNil)
1166- err = s.State.SetEnvironConfig(change, cfg)
1167- c.Assert(err, gc.IsNil)
1168- cfg, err = s.State.EnvironConfig()
1169- c.Assert(err, gc.IsNil)
1170- c.Assert(cfg.AllAttrs(), gc.DeepEquals, change.AllAttrs())
1171+ }
1172+ cfg, err := s.State.EnvironConfig()
1173+ c.Assert(err, gc.IsNil)
1174+ err = s.State.UpdateEnvironConfig(attrs, nil, nil)
1175+ c.Assert(err, gc.IsNil)
1176+ cfg, err = cfg.Apply(attrs)
1177+ c.Assert(err, gc.IsNil)
1178+ oldCfg, err := s.State.EnvironConfig()
1179+ c.Assert(err, gc.IsNil)
1180+
1181+ c.Assert(oldCfg, gc.DeepEquals, cfg)
1182 }
1183
1184 func (s *StateSuite) TestEnvironConstraints(c *gc.C) {
1185@@ -1681,6 +1683,37 @@
1186 })
1187 }
1188
1189+func (s *StateSuite) TestAdditionalValidation(c *gc.C) {
1190+ updateAttrs := map[string]interface{}{"logging-config": "juju=ERROR"}
1191+ configValidator1 := func(updateAttrs map[string]interface{}, removeAttrs []string, oldConfig *config.Config) error {
1192+ c.Assert(updateAttrs, gc.DeepEquals, map[string]interface{}{"logging-config": "juju=ERROR"})
1193+ if _, found := updateAttrs["logging-config"]; found {
1194+ return fmt.Errorf("cannot change logging-config")
1195+ }
1196+ return nil
1197+ }
1198+ removeAttrs := []string{"logging-config"}
1199+ configValidator2 := func(updateAttrs map[string]interface{}, removeAttrs []string, oldConfig *config.Config) error {
1200+ c.Assert(removeAttrs, gc.DeepEquals, []string{"logging-config"})
1201+ for _, i := range removeAttrs {
1202+ if i == "logging-config" {
1203+ return fmt.Errorf("cannot remove logging-config")
1204+ }
1205+ }
1206+ return nil
1207+ }
1208+ configValidator3 := func(updateAttrs map[string]interface{}, removeAttrs []string, oldConfig *config.Config) error {
1209+ return nil
1210+ }
1211+
1212+ err := s.State.UpdateEnvironConfig(updateAttrs, nil, configValidator1)
1213+ c.Assert(err, gc.ErrorMatches, "cannot change logging-config")
1214+ err = s.State.UpdateEnvironConfig(nil, removeAttrs, configValidator2)
1215+ c.Assert(err, gc.ErrorMatches, "cannot remove logging-config")
1216+ err = s.State.UpdateEnvironConfig(updateAttrs, nil, configValidator3)
1217+ c.Assert(err, gc.IsNil)
1218+}
1219+
1220 type attrs map[string]interface{}
1221
1222 func (s *StateSuite) TestWatchEnvironConfig(c *gc.C) {
1223@@ -1699,11 +1732,10 @@
1224 assertChange := func(change attrs) {
1225 cfg, err := s.State.EnvironConfig()
1226 c.Assert(err, gc.IsNil)
1227+ cfg, err = cfg.Apply(change)
1228+ c.Assert(err, gc.IsNil)
1229 if change != nil {
1230- oldcfg := cfg
1231- cfg, err = cfg.Apply(change)
1232- c.Assert(err, gc.IsNil)
1233- err = s.State.SetEnvironConfig(cfg, oldcfg)
1234+ err = s.State.UpdateEnvironConfig(change, nil, nil)
1235 c.Assert(err, gc.IsNil)
1236 }
1237 s.State.StartSync()
1238@@ -1761,7 +1793,6 @@
1239 func (s *StateSuite) TestWatchEnvironConfigCorruptConfig(c *gc.C) {
1240 cfg, err := s.State.EnvironConfig()
1241 c.Assert(err, gc.IsNil)
1242- oldcfg := cfg
1243
1244 // Corrupt the environment configuration.
1245 settings := s.Session.DB("juju").C("settings")
1246@@ -1797,9 +1828,11 @@
1247 }
1248
1249 // Fix the configuration.
1250- err = s.State.SetEnvironConfig(cfg, oldcfg)
1251+ err = settings.UpdateId("e", bson.D{{"$set", bson.D{{"name", "foo"}}}})
1252 c.Assert(err, gc.IsNil)
1253 fixed := cfg.AllAttrs()
1254+ err = s.State.UpdateEnvironConfig(fixed, nil, nil)
1255+ c.Assert(err, gc.IsNil)
1256
1257 s.State.StartSync()
1258 select {
1259@@ -2559,9 +2592,7 @@
1260 func (s *StateSuite) changeEnviron(c *gc.C, envConfig *config.Config, name string, value interface{}) {
1261 attrs := envConfig.AllAttrs()
1262 attrs[name] = value
1263- newConfig, err := config.New(config.NoDefaults, attrs)
1264- c.Assert(err, gc.IsNil)
1265- c.Assert(s.State.SetEnvironConfig(newConfig, envConfig), gc.IsNil)
1266+ c.Assert(s.State.UpdateEnvironConfig(attrs, nil, nil), gc.IsNil)
1267 }
1268
1269 func (s *StateSuite) assertAgentVersion(c *gc.C, envConfig *config.Config, vers string) {
1270
1271=== modified file 'state/testing/agent.go'
1272--- state/testing/agent.go 2013-09-11 03:48:05 +0000
1273+++ state/testing/agent.go 2014-03-18 02:38:23 +0000
1274@@ -11,5 +11,5 @@
1275 // SetAgentVersion sets the current agent version in the state's
1276 // environment configuration.
1277 func SetAgentVersion(st *state.State, vers version.Number) error {
1278- return UpdateConfig(st, map[string]interface{}{"agent-version": vers.String()})
1279+ return st.UpdateEnvironConfig(map[string]interface{}{"agent-version": vers.String()}, nil, nil)
1280 }
1281
1282=== removed file 'state/testing/config.go'
1283--- state/testing/config.go 2013-12-06 04:47:45 +0000
1284+++ state/testing/config.go 1970-01-01 00:00:00 +0000
1285@@ -1,22 +0,0 @@
1286-// Copyright 2013 Canonical Ltd.
1287-// Licensed under the AGPLv3, see LICENCE file for details.
1288-
1289-package testing
1290-
1291-import (
1292- "launchpad.net/juju-core/state"
1293-)
1294-
1295-// UpdateConfig sets the current agent version in the state's
1296-// environment configuration.
1297-func UpdateConfig(st *state.State, newValues map[string]interface{}) error {
1298- cfg, err := st.EnvironConfig()
1299- if err != nil {
1300- return err
1301- }
1302- newcfg, err := cfg.Apply(newValues)
1303- if err != nil {
1304- return err
1305- }
1306- return st.SetEnvironConfig(newcfg, cfg)
1307-}
1308
1309=== modified file 'upgrades/deprecatedattributes.go'
1310--- upgrades/deprecatedattributes.go 2014-03-07 02:48:20 +0000
1311+++ upgrades/deprecatedattributes.go 2014-03-18 02:38:23 +0000
1312@@ -3,29 +3,16 @@
1313
1314 package upgrades
1315
1316-import (
1317- "fmt"
1318-
1319- "launchpad.net/juju-core/environs/config"
1320-)
1321-
1322 func processDeprecatedAttributes(context Context) error {
1323 st := context.State()
1324- cfg, err := st.EnvironConfig()
1325- if err != nil {
1326- return fmt.Errorf("failed to read current config: %v", err)
1327+ removeAttrs := []string{
1328+ "public-bucket",
1329+ "public-bucket-region",
1330+ "public-bucket-url",
1331+ "default-image-id",
1332+ "default-instance-type",
1333+ "shared-storage-port",
1334 }
1335- newAttrs := cfg.AllAttrs()
1336- delete(newAttrs, "public-bucket")
1337- delete(newAttrs, "public-bucket-region")
1338- delete(newAttrs, "public-bucket-url")
1339- delete(newAttrs, "default-image-id")
1340- delete(newAttrs, "default-instance-type")
1341- delete(newAttrs, "shared-storage-port")
1342 // TODO (wallyworld) - delete tools-url in 1.20
1343- newCfg, err := config.New(config.NoDefaults, newAttrs)
1344- if err != nil {
1345- return fmt.Errorf("failed to create new config: %v", err)
1346- }
1347- return st.SetEnvironConfig(newCfg, cfg)
1348+ return st.UpdateEnvironConfig(map[string]interface{}{}, removeAttrs, nil)
1349 }
1350
1351=== modified file 'upgrades/deprecatedattributes_test.go'
1352--- upgrades/deprecatedattributes_test.go 2014-03-13 07:54:56 +0000
1353+++ upgrades/deprecatedattributes_test.go 2014-03-18 02:38:23 +0000
1354@@ -27,19 +27,16 @@
1355 apiState: apiState,
1356 state: s.State,
1357 }
1358- cfg, err := s.State.EnvironConfig()
1359- c.Assert(err, gc.IsNil)
1360 // Add in old attributes.
1361- newCfg, err := cfg.Apply(map[string]interface{}{
1362+ newCfg := map[string]interface{}{
1363 "public-bucket": "foo",
1364 "public-bucket-region": "bar",
1365 "public-bucket-url": "shazbot",
1366 "default-instance-type": "vulch",
1367 "default-image-id": "1234",
1368 "shared-storage-port": 1234,
1369- })
1370- c.Assert(err, gc.IsNil)
1371- err = s.State.SetEnvironConfig(newCfg, cfg)
1372+ }
1373+ err := s.State.UpdateEnvironConfig(newCfg, nil, nil)
1374 c.Assert(err, gc.IsNil)
1375 }
1376
1377
1378=== modified file 'upgrades/rsyslogport.go'
1379--- upgrades/rsyslogport.go 2014-02-28 05:44:03 +0000
1380+++ upgrades/rsyslogport.go 2014-03-18 02:38:23 +0000
1381@@ -9,15 +9,8 @@
1382
1383 func updateRsyslogPort(context Context) error {
1384 st := context.State()
1385- old, err := st.EnvironConfig()
1386- if err != nil {
1387- return err
1388- }
1389- cfg, err := old.Apply(map[string]interface{}{
1390+ attrs := map[string]interface{}{
1391 "syslog-port": config.DefaultSyslogPort,
1392- })
1393- if err != nil {
1394- return err
1395 }
1396- return st.SetEnvironConfig(cfg, old)
1397+ return st.UpdateEnvironConfig(attrs, nil, nil)
1398 }
1399
1400=== modified file 'worker/authenticationworker/worker_test.go'
1401--- worker/authenticationworker/worker_test.go 2013-12-23 05:18:58 +0000
1402+++ worker/authenticationworker/worker_test.go 2014-03-18 02:38:23 +0000
1403@@ -15,7 +15,6 @@
1404 "launchpad.net/juju-core/state"
1405 "launchpad.net/juju-core/state/api"
1406 "launchpad.net/juju-core/state/api/keyupdater"
1407- "launchpad.net/juju-core/state/testing"
1408 coretesting "launchpad.net/juju-core/testing"
1409 "launchpad.net/juju-core/utils/ssh"
1410 sshtesting "launchpad.net/juju-core/utils/ssh/testing"
1411@@ -90,7 +89,7 @@
1412
1413 func (s *workerSuite) setAuthorisedKeys(c *gc.C, keys ...string) {
1414 keyStr := strings.Join(keys, "\n")
1415- err := testing.UpdateConfig(s.BackingState, map[string]interface{}{"authorized-keys": keyStr})
1416+ err := s.BackingState.UpdateEnvironConfig(map[string]interface{}{"authorized-keys": keyStr}, nil, nil)
1417 c.Assert(err, gc.IsNil)
1418 s.BackingState.StartSync()
1419 }
1420
1421=== modified file 'worker/environ_test.go'
1422--- worker/environ_test.go 2013-12-05 03:42:25 +0000
1423+++ worker/environ_test.go 2014-03-18 02:38:23 +0000
1424@@ -46,30 +46,34 @@
1425 }
1426
1427 func (s *waitForEnvironSuite) TestInvalidConfig(c *gc.C) {
1428+ var oldType string
1429+ oldType = s.Conn.Environ.Config().AllAttrs()["type"].(string)
1430+
1431 // Create an invalid config by taking the current config and
1432 // tweaking the provider type.
1433- var oldType string
1434- testing.ChangeEnvironConfig(c, s.State, func(attrs coretesting.Attrs) coretesting.Attrs {
1435- oldType = attrs["type"].(string)
1436- return attrs.Merge(coretesting.Attrs{"type": "unknown"})
1437- })
1438- w := s.State.WatchForEnvironConfigChanges()
1439+ info := s.StateInfo(c)
1440+ opts := state.DefaultDialOpts()
1441+ st2, err := state.Open(info, opts, state.Policy(nil))
1442+ c.Assert(err, gc.IsNil)
1443+ defer st2.Close()
1444+ err = st2.UpdateEnvironConfig(map[string]interface{}{"type": "unknown"}, nil, nil)
1445+ c.Assert(err, gc.IsNil)
1446+
1447+ w := st2.WatchForEnvironConfigChanges()
1448 defer stopWatcher(c, w)
1449 done := make(chan environs.Environ)
1450 go func() {
1451- env, err := worker.WaitForEnviron(w, s.State, nil)
1452+ env, err := worker.WaitForEnviron(w, st2, nil)
1453 c.Check(err, gc.IsNil)
1454 done <- env
1455 }()
1456 // Wait for the loop to process the invalid configuratrion
1457 <-worker.LoadedInvalid
1458
1459- testing.ChangeEnvironConfig(c, s.State, func(attrs coretesting.Attrs) coretesting.Attrs {
1460- return attrs.Merge(coretesting.Attrs{
1461- "type": oldType,
1462- "secret": "environ_test",
1463- })
1464- })
1465+ st2.UpdateEnvironConfig(map[string]interface{}{
1466+ "type": oldType,
1467+ "secret": "environ_test",
1468+ }, nil, nil)
1469
1470 env := <-done
1471 c.Assert(env, gc.NotNil)
1472
1473=== modified file 'worker/firewaller/firewaller_test.go'
1474--- worker/firewaller/firewaller_test.go 2014-01-22 15:40:18 +0000
1475+++ worker/firewaller/firewaller_test.go 2014-03-18 02:38:23 +0000
1476@@ -4,13 +4,13 @@
1477 package firewaller_test
1478
1479 import (
1480+ "launchpad.net/juju-core/environs/config"
1481 "reflect"
1482 stdtesting "testing"
1483 "time"
1484
1485 gc "launchpad.net/gocheck"
1486
1487- "launchpad.net/juju-core/environs/config"
1488 "launchpad.net/juju-core/instance"
1489 "launchpad.net/juju-core/juju"
1490 "launchpad.net/juju-core/juju/testing"
1491@@ -37,6 +37,10 @@
1492 firewaller *apifirewaller.State
1493 }
1494
1495+type FirewallerGlobalModeSuite struct {
1496+ FirewallerSuite
1497+}
1498+
1499 var _ worker.Worker = (*firewaller.Firewaller)(nil)
1500
1501 // assertPorts retrieves the open ports of the instance and compares them
1502@@ -91,6 +95,13 @@
1503
1504 var _ = gc.Suite(&FirewallerSuite{})
1505
1506+func (s FirewallerGlobalModeSuite) SetUpTest(c *gc.C) {
1507+ add := map[string]interface{}{"firewall-mode": config.FwGlobal}
1508+ s.DummyConfig = dummy.SampleConfig().Merge(add).Delete("admin-secret", "ca-private-key")
1509+
1510+ s.FirewallerSuite.SetUpTest(c)
1511+}
1512+
1513 func (s *FirewallerSuite) SetUpTest(c *gc.C) {
1514 s.JujuConnSuite.SetUpTest(c)
1515 s.charm = s.AddTestingCharm(c, "dummy")
1516@@ -129,23 +140,6 @@
1517 return u, m
1518 }
1519
1520-func (s *FirewallerSuite) setGlobalMode(c *gc.C) {
1521- // TODO(rog) This should not be possible - you shouldn't
1522- // be able to set the firewalling mode after an environment
1523- // has bootstrapped.
1524- oldConfig := s.Conn.Environ.Config()
1525- attrs := oldConfig.AllAttrs()
1526- delete(attrs, "admin-secret")
1527- delete(attrs, "ca-private-key")
1528- attrs["firewall-mode"] = config.FwGlobal
1529- newConfig, err := config.New(config.NoDefaults, attrs)
1530- c.Assert(err, gc.IsNil)
1531- err = s.State.SetEnvironConfig(newConfig, oldConfig)
1532- c.Assert(err, gc.IsNil)
1533- err = s.Conn.Environ.SetConfig(newConfig)
1534- c.Assert(err, gc.IsNil)
1535-}
1536-
1537 // startInstance starts a new instance for the given machine.
1538 func (s *FirewallerSuite) startInstance(c *gc.C, m *state.Machine) instance.Instance {
1539 inst, hc := testing.AssertStartInstance(c, s.Conn.Environ, m.Id())
1540@@ -573,10 +567,7 @@
1541 c.Assert(err, gc.IsNil)
1542 }
1543
1544-func (s *FirewallerSuite) TestGlobalMode(c *gc.C) {
1545- // Change configuration.
1546- s.setGlobalMode(c)
1547-
1548+func (s *FirewallerGlobalModeSuite) TestGlobalMode(c *gc.C) {
1549 // Start firewaller and open ports.
1550 fw, err := firewaller.NewFirewaller(s.firewaller)
1551 c.Assert(err, gc.IsNil)
1552@@ -621,10 +612,7 @@
1553 s.assertEnvironPorts(c, nil)
1554 }
1555
1556-func (s *FirewallerSuite) TestGlobalModeStartWithUnexposedService(c *gc.C) {
1557- // Change configuration.
1558- s.setGlobalMode(c)
1559-
1560+func (s *FirewallerGlobalModeSuite) TestGlobalModeStartWithUnexposedService(c *gc.C) {
1561 m, err := s.State.AddMachine("quantal", state.JobHostUnits)
1562 c.Assert(err, gc.IsNil)
1563 s.startInstance(c, m)
1564@@ -650,10 +638,7 @@
1565 s.assertEnvironPorts(c, []instance.Port{{"tcp", 80}})
1566 }
1567
1568-func (s *FirewallerSuite) TestGlobalModeRestart(c *gc.C) {
1569- // Change configuration.
1570- s.setGlobalMode(c)
1571-
1572+func (s *FirewallerGlobalModeSuite) TestGlobalModeRestart(c *gc.C) {
1573 // Start firewaller and open ports.
1574 fw, err := firewaller.NewFirewaller(s.firewaller)
1575 c.Assert(err, gc.IsNil)
1576@@ -688,10 +673,7 @@
1577 s.assertEnvironPorts(c, []instance.Port{{"tcp", 80}, {"tcp", 8888}})
1578 }
1579
1580-func (s *FirewallerSuite) TestGlobalModeRestartUnexposedService(c *gc.C) {
1581- // Change configuration.
1582- s.setGlobalMode(c)
1583-
1584+func (s *FirewallerGlobalModeSuite) TestGlobalModeRestartUnexposedService(c *gc.C) {
1585 // Start firewaller and open ports.
1586 fw, err := firewaller.NewFirewaller(s.firewaller)
1587 c.Assert(err, gc.IsNil)
1588@@ -724,10 +706,7 @@
1589 s.assertEnvironPorts(c, nil)
1590 }
1591
1592-func (s *FirewallerSuite) TestGlobalModeRestartPortCount(c *gc.C) {
1593- // Change configuration.
1594- s.setGlobalMode(c)
1595-
1596+func (s *FirewallerGlobalModeSuite) TestGlobalModeRestartPortCount(c *gc.C) {
1597 // Start firewaller and open ports.
1598 fw, err := firewaller.NewFirewaller(s.firewaller)
1599 c.Assert(err, gc.IsNil)
1600
1601=== modified file 'worker/instancepoller/observer_test.go'
1602--- worker/instancepoller/observer_test.go 2014-03-05 19:41:34 +0000
1603+++ worker/instancepoller/observer_test.go 2014-03-18 02:38:23 +0000
1604@@ -12,6 +12,7 @@
1605 gc "launchpad.net/gocheck"
1606
1607 "launchpad.net/juju-core/juju/testing"
1608+ "launchpad.net/juju-core/state"
1609 coretesting "launchpad.net/juju-core/testing"
1610 )
1611
1612@@ -43,17 +44,18 @@
1613
1614 env := obs.Environ()
1615 c.Assert(env.Config().AllAttrs(), gc.DeepEquals, originalConfig.AllAttrs())
1616-
1617 var oldType string
1618+ oldType = env.Config().AllAttrs()["type"].(string)
1619+
1620+ info := s.StateInfo(c)
1621+ opts := state.DefaultDialOpts()
1622+ st2, err := state.Open(info, opts, state.Policy(nil))
1623+ defer st2.Close()
1624+
1625 // Change to an invalid configuration and check
1626 // that the observer's environment remains the same.
1627- testing.ChangeEnvironConfig(c, s.State, func(attrs coretesting.Attrs) coretesting.Attrs {
1628- oldType = attrs["type"].(string)
1629- return attrs.Merge(coretesting.Attrs{
1630- "type": "invalid",
1631- })
1632- })
1633- s.State.StartSync()
1634+ st2.UpdateEnvironConfig(map[string]interface{}{"type": "invalid"}, nil, nil)
1635+ st2.StartSync()
1636
1637 // Wait for the observer to register the invalid environment
1638 timeout := time.After(coretesting.LongWait)
1639@@ -74,13 +76,8 @@
1640
1641 // Change the environment back to a valid configuration
1642 // with a different name and check that we see it.
1643- testing.ChangeEnvironConfig(c, s.State, func(attrs coretesting.Attrs) coretesting.Attrs {
1644- return attrs.Merge(coretesting.Attrs{
1645- "type": oldType,
1646- "name": "a-new-name",
1647- })
1648- })
1649- s.State.StartSync()
1650+ st2.UpdateEnvironConfig(map[string]interface{}{"type": oldType, "name": "a-new-name"}, nil, nil)
1651+ st2.StartSync()
1652
1653 for a := coretesting.LongAttempt.Start(); a.Next(); {
1654 env := obs.Environ()
1655
1656=== modified file 'worker/machineenvironmentworker/machineenvironmentworker_test.go'
1657--- worker/machineenvironmentworker/machineenvironmentworker_test.go 2014-03-13 07:54:56 +0000
1658+++ worker/machineenvironmentworker/machineenvironmentworker_test.go 2014-03-18 02:38:23 +0000
1659@@ -119,8 +119,6 @@
1660 }
1661
1662 func (s *MachineEnvironmentWatcherSuite) updateConfig(c *gc.C) (osenv.ProxySettings, osenv.ProxySettings) {
1663- oldConfig, err := s.State.EnvironConfig()
1664- c.Assert(err, gc.IsNil)
1665
1666 proxySettings := osenv.ProxySettings{
1667 Http: "http proxy",
1668@@ -128,9 +126,10 @@
1669 Ftp: "ftp proxy",
1670 NoProxy: "no proxy",
1671 }
1672-
1673- envConfig, err := oldConfig.Apply(config.ProxyConfigMap(proxySettings))
1674- c.Assert(err, gc.IsNil)
1675+ attrs := map[string]interface{}{}
1676+ for k, v := range config.ProxyConfigMap(proxySettings) {
1677+ attrs[k] = v
1678+ }
1679
1680 // We explicitly set apt proxy settings as well to show that it is the apt
1681 // settings that are used for the apt config, and not just the normal
1682@@ -141,10 +140,11 @@
1683 Https: "apt https proxy",
1684 Ftp: "apt ftp proxy",
1685 }
1686- envConfig, err = envConfig.Apply(config.AptProxyConfigMap(aptProxySettings))
1687- c.Assert(err, gc.IsNil)
1688+ for k, v := range config.AptProxyConfigMap(aptProxySettings) {
1689+ attrs[k] = v
1690+ }
1691
1692- err = s.State.SetEnvironConfig(envConfig, oldConfig)
1693+ err := s.State.UpdateEnvironConfig(attrs, nil, nil)
1694 c.Assert(err, gc.IsNil)
1695
1696 return proxySettings, aptProxySettings
1697
1698=== modified file 'worker/provisioner/provisioner_test.go'
1699--- worker/provisioner/provisioner_test.go 2014-03-13 07:54:56 +0000
1700+++ worker/provisioner/provisioner_test.go 2014-03-18 02:38:23 +0000
1701@@ -96,11 +96,8 @@
1702 // that causes the given environMethod of the dummy provider to return
1703 // an error, which is also returned as a message to be checked.
1704 func breakDummyProvider(c *gc.C, st *state.State, environMethod string) string {
1705- oldCfg, err := st.EnvironConfig()
1706- c.Assert(err, gc.IsNil)
1707- cfg, err := oldCfg.Apply(map[string]interface{}{"broken": environMethod})
1708- c.Assert(err, gc.IsNil)
1709- err = st.SetEnvironConfig(cfg, oldCfg)
1710+ attrs := map[string]interface{}{"broken": environMethod}
1711+ err := st.UpdateEnvironConfig(attrs, nil, nil)
1712 c.Assert(err, gc.IsNil)
1713 return fmt.Sprintf("dummy.%s is broken", environMethod)
1714 }
1715@@ -121,21 +118,21 @@
1716 // so the Settings returned from the watcher will not pass
1717 // validation.
1718 func (s *CommonProvisionerSuite) invalidateEnvironment(c *gc.C) {
1719- attrs := s.cfg.AllAttrs()
1720- attrs["type"] = "unknown"
1721- invalidCfg, err := config.New(config.NoDefaults, attrs)
1722+ st, err := state.Open(s.StateInfo(c), state.DefaultDialOpts(), state.Policy(nil))
1723 c.Assert(err, gc.IsNil)
1724- err = s.State.SetEnvironConfig(invalidCfg, s.cfg)
1725+ defer st.Close()
1726+ attrs := map[string]interface{}{"type": "unknown"}
1727+ err = st.UpdateEnvironConfig(attrs, nil, nil)
1728 c.Assert(err, gc.IsNil)
1729 }
1730
1731 // fixEnvironment undoes the work of invalidateEnvironment.
1732-func (s *CommonProvisionerSuite) fixEnvironment() error {
1733- cfg, err := s.State.EnvironConfig()
1734- if err != nil {
1735- return err
1736- }
1737- return s.State.SetEnvironConfig(s.cfg, cfg)
1738+func (s *CommonProvisionerSuite) fixEnvironment(c *gc.C) error {
1739+ st, err := state.Open(s.StateInfo(c), state.DefaultDialOpts(), state.Policy(nil))
1740+ c.Assert(err, gc.IsNil)
1741+ defer st.Close()
1742+ attrs := map[string]interface{}{"type": s.cfg.AllAttrs()["type"]}
1743+ return st.UpdateEnvironConfig(attrs, nil, nil)
1744 }
1745
1746 // stopper is stoppable.
1747@@ -415,7 +412,7 @@
1748 }
1749
1750 // Unbreak the environ config.
1751- err = s.fixEnvironment()
1752+ err = s.fixEnvironment(c)
1753 c.Assert(err, gc.IsNil)
1754
1755 // Restart the PA to make sure the machine is skipped again.
1756@@ -480,7 +477,7 @@
1757 // the PA should not create it
1758 s.checkNoOperations(c)
1759
1760- err = s.fixEnvironment()
1761+ err = s.fixEnvironment(c)
1762 c.Assert(err, gc.IsNil)
1763
1764 s.checkStartInstance(c, m)
1765@@ -611,20 +608,15 @@
1766 // the PA should create it using the old environment
1767 s.checkStartInstance(c, m)
1768
1769- err = s.fixEnvironment()
1770+ err = s.fixEnvironment(c)
1771 c.Assert(err, gc.IsNil)
1772
1773 // insert our observer
1774 cfgObserver := make(chan *config.Config, 1)
1775 provisioner.SetObserver(p, cfgObserver)
1776
1777- oldcfg, err := s.State.EnvironConfig()
1778- c.Assert(err, gc.IsNil)
1779- attrs := oldcfg.AllAttrs()
1780- attrs["secret"] = "beef"
1781- cfg, err := config.New(config.NoDefaults, attrs)
1782- c.Assert(err, gc.IsNil)
1783- err = s.State.SetEnvironConfig(cfg, oldcfg)
1784+ err = s.State.UpdateEnvironConfig(map[string]interface{}{"secret": "beef"}, nil, nil)
1785+ c.Assert(err, gc.IsNil)
1786
1787 s.BackingState.StartSync()
1788
1789@@ -666,13 +658,9 @@
1790 c.Assert(m1.Remove(), gc.IsNil)
1791
1792 // turn on safe mode
1793- oldcfg, err := s.State.EnvironConfig()
1794- c.Assert(err, gc.IsNil)
1795- attrs := oldcfg.AllAttrs()
1796- attrs["provisioner-safe-mode"] = true
1797- cfg, err := config.New(config.NoDefaults, attrs)
1798- c.Assert(err, gc.IsNil)
1799- err = s.State.SetEnvironConfig(cfg, oldcfg)
1800+ attrs := map[string]interface{}{"provisioner-safe-mode": true}
1801+ err = s.State.UpdateEnvironConfig(attrs, nil, nil)
1802+ c.Assert(err, gc.IsNil)
1803
1804 // start a new provisioner to shut down only the machine still in state.
1805 p = s.newEnvironProvisioner(c)
1806@@ -712,13 +700,9 @@
1807 provisioner.SetObserver(p, cfgObserver)
1808
1809 // turn on safe mode
1810- oldcfg, err := s.State.EnvironConfig()
1811- c.Assert(err, gc.IsNil)
1812- attrs := oldcfg.AllAttrs()
1813- attrs["provisioner-safe-mode"] = true
1814- cfg, err := config.New(config.NoDefaults, attrs)
1815- c.Assert(err, gc.IsNil)
1816- err = s.State.SetEnvironConfig(cfg, oldcfg)
1817+ attrs := map[string]interface{}{"provisioner-safe-mode": true}
1818+ err = s.State.UpdateEnvironConfig(attrs, nil, nil)
1819+ c.Assert(err, gc.IsNil)
1820
1821 s.BackingState.StartSync()
1822
1823
1824=== modified file 'worker/uniter/uniter_test.go'
1825--- worker/uniter/uniter_test.go 2014-03-17 13:45:26 +0000
1826+++ worker/uniter/uniter_test.go 2014-03-18 02:38:23 +0000
1827@@ -1959,16 +1959,13 @@
1828 type setProxySettings osenv.ProxySettings
1829
1830 func (s setProxySettings) step(c *gc.C, ctx *context) {
1831- old, err := ctx.st.EnvironConfig()
1832- c.Assert(err, gc.IsNil)
1833- cfg, err := old.Apply(map[string]interface{}{
1834+ attrs := map[string]interface{}{
1835 "http-proxy": s.Http,
1836 "https-proxy": s.Https,
1837 "ftp-proxy": s.Ftp,
1838 "no-proxy": s.NoProxy,
1839- })
1840- c.Assert(err, gc.IsNil)
1841- err = ctx.st.SetEnvironConfig(cfg, old)
1842+ }
1843+ err := ctx.st.UpdateEnvironConfig(attrs, nil, nil)
1844 c.Assert(err, gc.IsNil)
1845 // wait for the new values...
1846 expected := (osenv.ProxySettings)(s)

Subscribers

People subscribed via source and target branches

to status/vote changes: