Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1262)

Issue 8610043: Implement the set-environment command.

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 8 months ago by thumper
Modified:
9 years, 8 months ago
Reviewers:
mp+157986, dfc, fwereade
Visibility:
Public.

Description

Implement the set-environment command. This change introduces a new command called set-environment. It has an alias of set-env, and is the pair to get-environment. Key/value pairs are taken on the command line as positional parameters. The environment is retrieved from state, updated with the new values, and then validated using the provider Validate method. The resulting configuration is passed bach through to the state.SetEnvironConfig. There are some standard config values that we really don't want changing. These rules have been encoded in a new function config.Validate. This is now called from each of the provider validate methods. I added a non-exported helper method on Config to do the type coersion for getting values from the known map. This is now used in many internal places. Since the agent-version was always added in the config.New to be the default value, I instead add this to the schema default values. The existing tests make sure this still works as intended. Added an extra test for backslashes in names. Moved the firewall-mode default behaviour up into the base class, as it was identical in all three provider validate methods. https://code.launchpad.net/~thumper/juju-core/set-environment/+merge/157986 Requires: https://code.launchpad.net/~thumper/juju-core/get-environment/+merge/157968 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 18

Patch Set 2 : Implement the set-environment command. #

Total comments: 4

Patch Set 3 : Implement the set-environment command. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+353 lines, -65 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/juju/environment.go View 1 2 1 chunk +75 lines, -0 lines 0 comments Download
M cmd/juju/environment_test.go View 1 1 chunk +93 lines, -0 lines 0 comments Download
M cmd/juju/main.go View 1 chunk +1 line, -0 lines 0 comments Download
M cmd/juju/main_test.go View 1 chunk +2 lines, -0 lines 0 comments Download
M environs/config/config.go View 1 8 chunks +73 lines, -41 lines 0 comments Download
M environs/config/config_test.go View 3 chunks +92 lines, -1 line 0 comments Download
M environs/dummy/environs.go View 1 chunk +5 lines, -7 lines 0 comments Download
M environs/ec2/config.go View 2 chunks +5 lines, -8 lines 0 comments Download
M environs/openstack/config.go View 2 chunks +5 lines, -8 lines 0 comments Download

Messages

Total messages: 6
thumper
Please take a look.
9 years, 8 months ago (2013-04-10 04:04:39 UTC) #1
fwereade
LGTM modulo comments below https://codereview.appspot.com/8610043/diff/1/cmd/juju/environment.go File cmd/juju/environment.go (right): https://codereview.appspot.com/8610043/diff/1/cmd/juju/environment.go#newcode117 cmd/juju/environment.go:117: c.values[key] = bits[1] Hmm, do ...
9 years, 8 months ago (2013-04-10 12:45:20 UTC) #2
thumper
Will repropose too. https://codereview.appspot.com/8610043/diff/1/cmd/juju/environment.go File cmd/juju/environment.go (right): https://codereview.appspot.com/8610043/diff/1/cmd/juju/environment.go#newcode117 cmd/juju/environment.go:117: c.values[key] = bits[1] On 2013/04/10 12:45:20, ...
9 years, 8 months ago (2013-04-10 23:17:23 UTC) #3
thumper
Please take a look.
9 years, 8 months ago (2013-04-10 23:40:15 UTC) #4
dfc
LGTM with a few comments. Excellent work deduplicating the logic inside the providers. https://codereview.appspot.com/8610043/diff/8001/cmd/juju/environment.go File ...
9 years, 8 months ago (2013-04-10 23:56:27 UTC) #5
thumper
9 years, 8 months ago (2013-04-11 00:24:31 UTC) #6
*** Submitted:

Implement the set-environment command.

This change introduces a new command called set-environment. It has an alias
of set-env, and is the pair to get-environment.

Key/value pairs are taken on the command line as positional parameters.

The environment is retrieved from state, updated with the new values, and then
validated using the provider Validate method.  The resulting configuration is
passed bach through to the state.SetEnvironConfig.

There are some standard config values that we really don't want
changing. These rules have been encoded in a new function config.Validate.
This is now called from each of the provider validate methods.

I added a non-exported helper method on Config to do the type coersion for
getting values from the known map.  This is now used in many internal places.

Since the agent-version was always added in the config.New to be the default
value, I instead add this to the schema default values.  The existing tests
make sure this still works as intended.

Added an extra test for backslashes in names.  Moved the firewall-mode default
behaviour up into the base class, as it was identical in all three provider
validate methods.

R=fwereade, dfc
CC=
https://codereview.appspot.com/8610043

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

https://codereview.appspot.com/8610043/diff/8001/cmd/juju/environment.go#newc...
cmd/juju/environment.go:132: // Get the existing environment config from the
state.
On 2013/04/10 23:56:27, dfc wrote:
> This logic is moderately subtle, and I have a memory that we do the same thing
> elsewhere, when uploading/updating the secrets in the environment
configuration.
> Could you please add a TODO or card to rationalise this.

Done.

https://codereview.appspot.com/8610043/diff/8001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/8610043/diff/8001/environs/config/config.go#ne...
environs/config/config.go:336: "agent-version":            
version.CurrentNumber().String(),
On 2013/04/10 23:56:27, dfc wrote:
> hmm, why has this changed ?

Just rationalizing the default process, was set in config.New before.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b