environs: add configuration attributes and access

environs: add configuration attributes and access

This is an alternative possibility to https://codereview.appspot.com/6353092/

FOR DISCUSSION ONLY - WORK IN PROGRESS

https://code.launchpad.net/~rogpeppe/juju-core/configuration-alternative/+merge/114870

(do not edit description out of merge proposal)

[fwereade, 2012-07-13 16:08:14]

Much nicer approach in general, more than happy to toss mine out; but definitely
needs more tests for config creation and changes involving irritating details
like empty strings (which AFAICT will be the only way to re-read default key
paths and AWS auth), along with the other stuff I mention below. But, really
nice all the same.

https://codereview.appspot.com/6343107/diff/1/cmd/juju/cmd_test.go
File cmd/juju/cmd_test.go (right):

https://codereview.appspot.com/6343107/diff/1/cmd/juju/cmd_test.go#newcode52
cmd/juju/cmd_test.go:52: authorized-keys: keys
I'd make the dummy provider insert default values for authorized-keys and
default-series, no sense dirtying up the tests except when we care about those
settings.

https://codereview.appspot.com/6343107/diff/1/cmd/juju/deploy_test.go
File cmd/juju/deploy_test.go (right):

https://codereview.appspot.com/6343107/diff/1/cmd/juju/deploy_test.go#newcode25
cmd/juju/deploy_test.go:25: authorized-keys: "keys"
similarly

https://codereview.appspot.com/6343107/diff/1/environs/config/auth_test.go
File environs/config/auth_test.go (right):

https://codereview.appspot.com/6343107/diff/1/environs/config/auth_test.go#ne...
environs/config/auth_test.go:1: package config_test
This test was broken, because it wasn't being run, and I don't see the necessary
fixes, so I suspect it's still not being run.

https://codereview.appspot.com/6343107/diff/1/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/6343107/diff/1/environs/config/config.go#newcode9
environs/config/config.go:9: authorizedKeys string
needs defaultSeries

https://codereview.appspot.com/6343107/diff/1/environs/config/config.go#newco...
environs/config/config.go:22: "broken",
don't think this should be here

https://codereview.appspot.com/6343107/diff/1/environs/config/export_test.go
File environs/config/export_test.go (right):

https://codereview.appspot.com/6343107/diff/1/environs/config/export_test.go#...
environs/config/export_test.go:3: func AuthorizedKeys(path string) (string,
error) {
please, ReadAuthorizedKeys

https://codereview.appspot.com/6343107/diff/1/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/6343107/diff/1/environs/dummy/environs.go#newc...
environs/dummy/environs.go:222: "default-series": schema.String(),
default-series is needed by everything; it's required by the deploy command.

https://codereview.appspot.com/6343107/diff/1/environs/dummy/environs.go#newc...
environs/dummy/environs.go:226: "default-series",
This feels kinda icky. We *do* require default-series, just not on the way in.
Maybe we could call this inputChecker or something?

https://codereview.appspot.com/6343107/diff/1/environs/ec2/config.go
File environs/ec2/config.go (right):

https://codereview.appspot.com/6343107/diff/1/environs/ec2/config.go#newcode20
environs/ec2/config.go:20: defaultSeries  string
should be common, again... getting default-series out, for use by deploy, is the
whole point of all of this :)

https://codereview.appspot.com/6343107/diff/1/environs/interface.go
File environs/interface.go (left):

https://codereview.appspot.com/6343107/diff/1/environs/interface.go#oldcode21
environs/interface.go:21: Open() (Environ, error)
Still not wild on Open, but not especially bothered really.

https://codereview.appspot.com/6343107/diff/1/environs/interface.go
File environs/interface.go (right):

https://codereview.appspot.com/6343107/diff/1/environs/interface.go#newcode145
environs/interface.go:145: Config() EnvironConfig
Please no. I can't see any good reason for this at all, and IMO it shouldn't
even be possible to look at a Conn's Environ's config, because it is likely to
be wrong.

https://codereview.appspot.com/6343107/diff/1/environs/jujutest/livetests.go
File environs/jujutest/livetests.go (right):

https://codereview.appspot.com/6343107/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:157: c.Check(err, NotNil)
ErrorMatches

https://codereview.appspot.com/6343107/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:161: c.Check(err, NotNil)
ditto

https://codereview.appspot.com/6343107/diff/1/environs/jujutest/livetests.go#...
environs/jujutest/livetests.go:165: c.Check(err, NotNil)
ditto

https://codereview.appspot.com/6343107/diff/5001/environs/ec2/config_test.go
File environs/ec2/config_test.go (left):

https://codereview.appspot.com/6343107/diff/5001/environs/ec2/config_test.go#...
environs/ec2/config_test.go:66: }
Needs tests for Change, and probably Clone; and there are a few subtleties
around changing AWS auth (from env, and from direct keys) and
authorized-keys[-path]. Those should really go in the base Config tests though,
and just have a spot of verification here.

https://codereview.appspot.com/6343107/diff/5001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6343107/diff/5001/environs/ec2/ec2.go#newcode115
environs/ec2/ec2.go:115: return nil, fmt.Errorf("no ec2 endpoint found for
region %q, opening %q", cfg.region, cfg.Name())
Isn't this something we should be checking in the config, when we pick a region?

[rog, 2012-07-13 17:00:54]

On 2012/07/13 16:08:14, fwereade wrote:
> Much nicer approach in general, more than happy to toss mine out; but
definitely

thanks for the positive review. i will add some tests and address your comments
on monday.