Code review comment for lp:~wallyworld/juju-core/default-debug-logging

LGTM. Ship it

On Thu, Oct 10, 2013 at 11:57 AM, Ian Booth <email address hidden> wrote:
> Reviewers: mp+190282_code.launchpad.net,
>
> Message:
> Please take a look.
>
> Description:
> Agents log using debug by default
>
> This branch changes the default logging level for agents to use debug.
> The level is set to debug only if the user has not specified something
> different via logging-config or JUJU_LOGGING_CONFIG
>
> https://code.launchpad.net/~wallyworld/juju-core/default-debug-logging/+merge/190282
>
> (do not edit description out of merge proposal)
>
>
> Please review this at https://codereview.appspot.com/14592044/
>
> Affected files (+31, -1 lines):
> A [revision details]
> M environs/config/config.go
> M environs/config/config_test.go
>
>
> Index: [revision details]
> === added file '[revision details]'
> --- [revision details] 2012-01-01 00:00:00 +0000
> +++ [revision details] 2012-01-01 00:00:00 +0000
> @@ -0,0 +1,2 @@
> +Old revision: tarmac-20131009221722-1lb0kalirzqc6s0b
> +New revision: <email address hidden>
>
> Index: environs/config/config.go
> === modified file 'environs/config/config.go'
> --- environs/config/config.go 2013-10-04 12:18:17 +0000
> +++ environs/config/config.go 2013-10-10 00:49:51 +0000
> @@ -104,7 +104,15 @@
> if environmentValue := os.Getenv(osenv.JujuLoggingConfig);
> environmentValue != "" {
> c.m["logging-config"] = environmentValue
> } else {
> - c.m["logging-config"] = loggo.LoggerInfo()
> + //TODO(wallyworld) - 2013-10-10 bug=1237731
> + // We need better way to ensure default logging is set to debug.
> + // This is a *quick* fix to get 1.16 out the door.
> + loggoConfig := loggo.LoggerInfo()
> + if loggoConfig != "<root>=WARNING" {
> + c.m["logging-config"] = loggoConfig
> + } else {
> + c.m["logging-config"] = "<root>=DEBUG"
> + }
> }
> }
>
>
>
> Index: environs/config/config_test.go
> === modified file 'environs/config/config_test.go'
> --- environs/config/config_test.go 2013-09-27 02:00:34 +0000
> +++ environs/config/config_test.go 2013-10-10 00:49:51 +0000
> @@ -32,6 +32,11 @@
>
> var _ = gc.Suite(&ConfigSuite{})
>
> +func (s *ConfigSuite) SetUpTest(c *gc.C) {
> + s.LoggingSuite.SetUpTest(c)
> + loggo.ResetLoggers()
> +}
> +
> // sampleConfig holds a configuration with all required
> // attributes set.
> var sampleConfig = testing.Attrs{
> @@ -90,6 +95,14 @@
> "default-series": "",
> },
> }, {
> + about: "Explicit logging",
> + useDefaults: config.UseDefaults,
> + attrs: testing.Attrs{
> + "type": "my-type",
> + "name": "my-name",
> + "logging-config": "juju=INFO",
> + },
> + }, {
> about: "Explicit authorized-keys",
> useDefaults: config.UseDefaults,
> attrs: testing.Attrs{
> @@ -721,6 +734,12 @@
> c.Assert(cfg.SSLHostnameVerification(), gc.Equals, v)
> }
>
> + if v, ok := test.attrs["logging-config"]; ok {
> + c.Assert(cfg.LoggingConfig(), gc.Equals, v)
> + } else {
> + c.Assert(cfg.LoggingConfig(), gc.Equals, "<root>=DEBUG")
> + }
> +
> url, urlPresent := cfg.ImageMetadataURL()
> if v, _ := test.attrs["image-metadata-url"].(string); v != "" {
> c.Assert(url, gc.Equals, v)
> @@ -764,6 +783,7 @@
> attrs["ca-private-key"] = ""
> attrs["image-metadata-url"] = ""
> attrs["tools-url"] = ""
> + attrs["logging-config"] = "<root>=DEBUG"
> // Default firewall mode is instance
> attrs["firewall-mode"] = string(config.FwInstance)
> c.Assert(cfg.AllAttrs(), gc.DeepEquals, attrs)
>
>
>
>
>
> --
> https://code.launchpad.net/~wallyworld/juju-core/default-debug-logging/+merge/190282
> You are subscribed to branch lp:juju-core.