User configurable tools/image URLs

User configurable tools/image URLs

A new framework for processing simplestreams URLs is introduced,
as well as user configurable URLs. Config keys for user override
of image and tools metdata URLs are: "image-metadata-url" and
"tools-metadata-url". The framework sets the order of precedence
to be: 1. user, 2. provider specific, 3. default

By moving to image and tools urls, the user can separately 
control how image ids are found and where tools come from.

https://code.launchpad.net/~wallyworld/juju-core/tools-metadata-config/+merge/180493

Requires: https://code.launchpad.net/~wallyworld/juju-core/simplestreams-mirrors/+merge/180487

(do not edit description out of merge proposal)

[wallyworld, 2013-08-16 08:18:08]

Please take a look.

[wallyworld, 2013-08-20 04:05:30]

Please take a look.

[fwereade, 2013-08-21 10:03:29]

Thank you, nice clean refactoring; would appreciate a quick discussion about
invalid config defaults before we land this though.

https://codereview.appspot.com/13047043/diff/4001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/13047043/diff/4001/environs/config/config.go#n...
environs/config/config.go:335: func (c *Config) ToolsMetadataURL() string {
I'd really like us to habitually avoid in-band invalidity signalling; please use
a `, ok` style in these cases.

https://codereview.appspot.com/13047043/diff/4001/environs/config/config.go#n...
environs/config/config.go:410: "image-metadata-url":        "",
I'm not really sure what it does for us to insert invalid values. It does match
existing practice, though, so I'm open to consistency arguments.

https://codereview.appspot.com/13047043/diff/4001/environs/imagemetadata/urls...
File environs/imagemetadata/urls_test.go (right):

https://codereview.appspot.com/13047043/diff/4001/environs/imagemetadata/urls...
environs/imagemetadata/urls_test.go:15: home *testing.FakeHome
Should this maybe be a LoggingSuite too? IIRC config can log...

[wallyworld, 2013-08-21 11:10:45]

https://codereview.appspot.com/13047043/diff/4001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/13047043/diff/4001/environs/config/config.go#n...
environs/config/config.go:335: func (c *Config) ToolsMetadataURL() string {
On 2013/08/21 10:03:29, fwereade wrote:
> I'd really like us to habitually avoid in-band invalidity signalling; please
use
> a `, ok` style in these cases.

Huh? This just matches what is done for name and other params.

https://codereview.appspot.com/13047043/diff/4001/environs/config/config.go#n...
environs/config/config.go:410: "image-metadata-url":        "",
On 2013/08/21 10:03:29, fwereade wrote:
> I'm not really sure what it does for us to insert invalid values. It does
match
> existing practice, though, so I'm open to consistency arguments.

There are no invalid values here. It is ok for these to not be configured by the
user and the default "" matches what is done for other such params. If "", it
just means the user is not overriding the system values.

https://codereview.appspot.com/13047043/diff/4001/environs/imagemetadata/urls...
File environs/imagemetadata/urls_test.go (right):

https://codereview.appspot.com/13047043/diff/4001/environs/imagemetadata/urls...
environs/imagemetadata/urls_test.go:15: home *testing.FakeHome
On 2013/08/21 10:03:29, fwereade wrote:
> Should this maybe be a LoggingSuite too? IIRC config can log...

I can make it a LoggingSuite.

[wallyworld, 2013-08-22 08:36:42]

Please take a look.

https://codereview.appspot.com/13047043/diff/4001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/13047043/diff/4001/environs/config/config.go#n...
environs/config/config.go:335: func (c *Config) ToolsMetadataURL() string {
On 2013/08/21 10:03:29, fwereade wrote:
> I'd really like us to habitually avoid in-band invalidity signalling; please
use
> a `, ok` style in these cases.

Now fix, ignore previous comment

[fwereade, 2013-08-22 10:44:53]

LGTM modulo minor comments (I *would* really prefer tools *metadata* over just
tools, though...)

https://codereview.appspot.com/13047043/diff/4001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/13047043/diff/4001/environs/config/config.go#n...
environs/config/config.go:410: "image-metadata-url":        "",
On 2013/08/21 11:10:45, wallyworld wrote:
> On 2013/08/21 10:03:29, fwereade wrote:
> > I'm not really sure what it does for us to insert invalid values. It does
> match
> > existing practice, though, so I'm open to consistency arguments.
> 
> There are no invalid values here. It is ok for these to not be configured by
the
> user and the default "" matches what is done for other such params. If "", it
> just means the user is not overriding the system values.

I'm just questioning "" vs schema.Omit. The code probably comes out simpler this
way though, so no big deal.

https://codereview.appspot.com/13047043/diff/4001/environs/imagemetadata/urls...
File environs/imagemetadata/urls_test.go (right):

https://codereview.appspot.com/13047043/diff/4001/environs/imagemetadata/urls...
environs/imagemetadata/urls_test.go:15: home *testing.FakeHome
On 2013/08/21 11:10:45, wallyworld wrote:
> On 2013/08/21 10:03:29, fwereade wrote:
> > Should this maybe be a LoggingSuite too? IIRC config can log...
> 
> I can make it a LoggingSuite.

but you haven't yet AFAICS :)

https://codereview.appspot.com/13047043/diff/12001/environs/config/config.go
File environs/config/config.go (right):

https://codereview.appspot.com/13047043/diff/12001/environs/config/config.go#...
environs/config/config.go:343: // and wether it has been set.
s/wether/whether/

https://codereview.appspot.com/13047043/diff/12001/environs/config/config.go#...
environs/config/config.go:415: "tools-url":                 "",
tools-metadata-url feels superior to me. Why the change?

https://codereview.appspot.com/13047043/diff/12001/environs/tools/urls_test.go
File environs/tools/urls_test.go (right):

https://codereview.appspot.com/13047043/diff/12001/environs/tools/urls_test.g...
environs/tools/urls_test.go:45: func (s *URLsSuite) TestToolsURLsNoConfigURL(c
*gc.C) {
similarly, I thought the "Metadata" was nice

[jameinel, 2013-08-22 12:22:14]

https://codereview.appspot.com/13047043/diff/12001/environs/tools/urls.go
File environs/tools/urls.go (right):

https://codereview.appspot.com/13047043/diff/12001/environs/tools/urls.go#new...
environs/tools/urls.go:8: type SupportsCustomURLs interface {
docstrings on exported types