Improve Openstack config deprecation warnings.

Improve Openstack config deprecation warnings.

The warning messages when using the deprecated Openstack config
items default-image-id and default-instance-type are improved.

https://code.launchpad.net/~wallyworld/juju-core/better-openstack-config-deprecation-messages/+merge/176110

(do not edit description out of merge proposal)

[wallyworld, 2013-07-22 04:23:27]

Please take a look.

[thumper, 2013-07-22 04:43:02]

Primary issue is should be using *testing.FakeHome to change HOME environment.

https://codereview.appspot.com/11659043/diff/1/environs/openstack/config_test.go
File environs/openstack/config_test.go (right):

https://codereview.appspot.com/11659043/diff/1/environs/openstack/config_test...
environs/openstack/config_test.go:13: "launchpad.net/loggo"
header grouping

https://codereview.appspot.com/11659043/diff/1/environs/openstack/config_test...
environs/openstack/config_test.go:460: s.oldJujuHome =
config.SetJujuHome(c.MkDir())
just use a testing.FakeHome.

[wallyworld, 2013-07-22 05:41:12]

Please take a look.

https://codereview.appspot.com/11659043/diff/1/environs/openstack/config_test.go
File environs/openstack/config_test.go (right):

https://codereview.appspot.com/11659043/diff/1/environs/openstack/config_test...
environs/openstack/config_test.go:13: "launchpad.net/loggo"
On 2013/07/22 04:43:02, thumper wrote:
> header grouping

Done.

https://codereview.appspot.com/11659043/diff/1/environs/openstack/config_test...
environs/openstack/config_test.go:460: s.oldJujuHome =
config.SetJujuHome(c.MkDir())
On 2013/07/22 04:43:02, thumper wrote:
> just use a testing.FakeHome.

I forgot about fake home. I just copied an existing test in this file. I fixed
the other place too.

[rog, 2013-07-22 12:45:24]

LGTM with a few thoughts regarding the testing.

https://codereview.appspot.com/11659043/diff/6001/environs/openstack/config.go
File environs/openstack/config.go (right):

https://codereview.appspot.com/11659043/diff/6001/environs/openstack/config.g...
environs/openstack/config.go:142: "The correct instance flavor is now determined
using constraints, globally specified\n"+
s/now //

https://codereview.appspot.com/11659043/diff/6001/environs/openstack/config_t...
File environs/openstack/config_test.go (right):

https://codereview.appspot.com/11659043/diff/6001/environs/openstack/config_t...
environs/openstack/config_test.go:463: s.oldJujuHome =
testing.MakeEmptyFakeHome(c)
given that there's only one test, i'd just
reuse ConfigSuite and do the logging setup
within that test.

also, no need to remove the default writer
AFAICS, which makes the setup and teardown easier.

https://codereview.appspot.com/11659043/diff/6001/environs/openstack/config_t...
environs/openstack/config_test.go:485: t.Buffer.WriteString(fmt.Sprintf("%s %s
%s", level, module, message))
you're not testing the level or the module - why not just
t.Buffer.WriteString(message) ?

or instead of using bytes.Buffer, you could just do:

type testWriter struct {
    msgs []string
}

func (t *testWriter) Write(level loggo.Level, module, filename string, line int,
timestamp time.Time, message string) {
   t.msgs = append(t.msgs, message)
}

and you could combine the two test cases inside
TestDeprecationWarnings and match the two messages
directly.

https://codereview.appspot.com/11659043/diff/6001/environs/openstack/config_t...
environs/openstack/config_test.go:488: func (s *ConfigDeprecationSuite)
setupEnv(c *gc.C, deprecatedKey, value string) *environ {
can't you do:
    attrs := map[string]interface{} {
        "type": "openstack", 
        "control-bucket": "x",
        "authorized-keys": "fakekey",
        deprecatedKey: value,
    }
    e, err := environs.NewFromAttrs(attrs)
 
which would be a bit simpler?

https://codereview.appspot.com/11659043/diff/6001/environs/openstack/config_t...
environs/openstack/config_test.go:506: e, err := es.Open("testenv")
c.Assert(err, IsNil)

https://codereview.appspot.com/11659043/diff/6001/environs/openstack/config_t...
environs/openstack/config_test.go:520: c.Assert(stripped, gc.Matches, expected)
i don't think this is testing what you think it's testing.
the log buffer is shared between the two test cases,
so the tests would, i think, pass even if each case
was logging the other's attribute and value.

[wallyworld, 2013-07-23 05:03:47]

Please take a look.

https://codereview.appspot.com/11659043/diff/6001/environs/openstack/config.go
File environs/openstack/config.go (right):

https://codereview.appspot.com/11659043/diff/6001/environs/openstack/config.g...
environs/openstack/config.go:142: "The correct instance flavor is now determined
using constraints, globally specified\n"+
On 2013/07/22 12:45:24, rog wrote:
> s/now //

Done.

https://codereview.appspot.com/11659043/diff/6001/environs/openstack/config_t...
File environs/openstack/config_test.go (right):

https://codereview.appspot.com/11659043/diff/6001/environs/openstack/config_t...
environs/openstack/config_test.go:463: s.oldJujuHome =
testing.MakeEmptyFakeHome(c)
On 2013/07/22 12:45:24, rog wrote:
> given that there's only one test, i'd just
> reuse ConfigSuite and do the logging setup
> within that test.
> 
> also, no need to remove the default writer
> AFAICS, which makes the setup and teardown easier.

The default writer is replaced so that stdout doesn't become polluted when
running the tests.

I'd prefer to use a separate suite for those tests which need to mess with the
logger and not confuse things for tests which don't need this functionality.

https://codereview.appspot.com/11659043/diff/6001/environs/openstack/config_t...
environs/openstack/config_test.go:485: t.Buffer.WriteString(fmt.Sprintf("%s %s
%s", level, module, message))
On 2013/07/22 12:45:24, rog wrote:
> you're not testing the level or the module - why not just
> t.Buffer.WriteString(message) ?
> 

level is tested since we check that the message contains WARNING
But maybe we don't care.

> or instead of using bytes.Buffer, you could just do:
> 
> type testWriter struct {
>     msgs []string
> }
> 
> func (t *testWriter) Write(level loggo.Level, module, filename string, line
int,
> timestamp time.Time, message string) {
>    t.msgs = append(t.msgs, message)
> }
> 
> and you could combine the two test cases inside
> TestDeprecationWarnings and match the two messages
> directly.

Ok

https://codereview.appspot.com/11659043/diff/6001/environs/openstack/config_t...
environs/openstack/config_test.go:488: func (s *ConfigDeprecationSuite)
setupEnv(c *gc.C, deprecatedKey, value string) *environ {
On 2013/07/22 12:45:24, rog wrote:
> can't you do:
>     attrs := map[string]interface{} {
>         "type": "openstack", 
>         "control-bucket": "x",
>         "authorized-keys": "fakekey",
>         deprecatedKey: value,
>     }
>     e, err := environs.NewFromAttrs(attrs)
>  
> which would be a bit simpler?

Yes. Thanks. I just copied the code from somewhere else.

https://codereview.appspot.com/11659043/diff/6001/environs/openstack/config_t...
environs/openstack/config_test.go:506: e, err := es.Open("testenv")
On 2013/07/22 12:45:24, rog wrote:
> c.Assert(err, IsNil)

Done.

https://codereview.appspot.com/11659043/diff/6001/environs/openstack/config_t...
environs/openstack/config_test.go:520: c.Assert(stripped, gc.Matches, expected)
On 2013/07/22 12:45:24, rog wrote:
> i don't think this is testing what you think it's testing.
> the log buffer is shared between the two test cases,
> so the tests would, i think, pass even if each case
> was logging the other's attribute and value.

You are right. Mentally I was thinking "separate tests". I've fixed it all.
Thanks.

[thumper, 2013-07-23 05:08:57]

LGTM

[rog, 2013-07-23 07:35:39]

that's better, thanks. LGTM