ec2,openstack: control-bucket set in Prepare

ec2,openstack: control-bucket set in Prepare



https://code.launchpad.net/~fwereade/juju-core/prepare-control-bucket/+merge/188955

(do not edit description out of merge proposal)

[fwereade, 2013-10-02 22:44:16]

Please take a look.

[gz, 2013-10-02 23:13:12]

LGTM, but the two-versions is crying out for some ec2-openstack unification of
tests and code.

[rog, 2013-10-02 23:38:05]

LGTM with one suggestion (it could use some better names though)

https://codereview.appspot.com/14307043/diff/1/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/14307043/diff/1/provider/ec2/ec2.go#newcode205
provider/ec2/ec2.go:205: if _, ok := attrs["control-bucket"]; !ok {
Thinking about mgz's remark, how about a little helper in provider/common, which
should be useful for other prepare cases too?

type DefaultAttrs map[string] func() (interface{}, error)

// AddDefaults returns the given configuration with any
// defaults in newAttr added.
func AddDefaults(cfg *config.Config, newAttr DefaultAttrs) (*config.Config,
error) {
    attrs := cfg.AllAttrs()
    for attr, makeAttr := range newAttr {
        if _, ok := attrs[attr]; ok {
             continue
        }
        new, err := makeAttr()
        if err != nil {
            return nil, err
        }
        attrs[attr] = new
    }
    return cfg.Apply(attrs)
}

Then the code here becomes:

cfg, err := common.AddDefaults(cfg, common.DefaultAttrs{
    "control-bucket": func() (interface{}, error) { return utils.NewUUID() },
})

[fwereade, 2013-10-02 23:48:15]

With some shame, I'm calling this two-strikes-not-three. It *will* be three when
I get to landing a skeleton provider, though, and that's on its way.

[rog, 2013-10-02 23:59:30]

You could put my suggested function in environs/config and then
it could be used in environs.ensureAdminSecret, making it three, if you like
:-)


On 3 October 2013 00:48,  <fwereade@gmail.com> wrote:
> With some shame, I'm calling this two-strikes-not-three. It *will* be
> three when I get to landing a skeleton provider, though, and that's on
> its way.
>
> https://codereview.appspot.com/14307043/

[mue, 2013-10-07 08:15:38]

LGTM, only missing prefix "juju-" for "control-bucket".

https://codereview.appspot.com/14307043/diff/1/provider/ec2/ec2.go
File provider/ec2/ec2.go (right):

https://codereview.appspot.com/14307043/diff/1/provider/ec2/ec2.go#newcode210
provider/ec2/ec2.go:210: attrs["control-bucket"] = fmt.Sprintf("%x", uuid.Raw())
How about the prefix "juju-" here?

https://codereview.appspot.com/14307043/diff/1/provider/openstack/provider.go
File provider/openstack/provider.go (right):

https://codereview.appspot.com/14307043/diff/1/provider/openstack/provider.go...
provider/openstack/provider.go:133: attrs["control-bucket"] = fmt.Sprintf("%x",
uuid.Raw())
Same with prefix here. In BoilerplateConfig() we generate it with a prefix.