Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
better error msgs #6782
Conversation
|
!!build!! |
| - logger.Infof(err.Error()) | ||
| + // Logged as debug here because it is logged as an error in | ||
| + // worker/provisioner. | ||
| + logger.Debugf(err.Error()) |
rogpeppe
Jan 9, 2017
Owner
There's no point in logging error messages that we return (especially when they're
less informative than the error that we're returning).
The appropriate place for logging errors is if we're discarding them.
reedobrien
Jan 9, 2017
Contributor
I agree. This was existing code. I just wanted to turn down the amount of logging of the same error.
Removed.
| @@ -203,7 +203,7 @@ func (commandWrapperSuite) TestAutostartMachineFails(c *gc.C) { | ||
| container := NewTestContainer("aname", stub.Run, nil) | ||
| err := AutostartMachine(container) | ||
| c.Assert(stub.Calls(), jc.DeepEquals, []string{"virsh autostart aname"}) | ||
| - c.Assert(err, gc.ErrorMatches, "Boom") | ||
| + c.Check(err, gc.ErrorMatches, "failed to autostart domain \"aname\": Boom") |
reedobrien
Jan 9, 2017
Contributor
I usually use backquotes. Not sure what I was thinking...well I do, but prolly not worth the story. Fixed.
| @@ -70,7 +70,7 @@ func (broker *kvmBroker) StartInstance(args environs.StartInstanceParams) (*envi | ||
| if err != nil { | ||
| // It's not fatal (yet) if we couldn't pre-allocate addresses for the | ||
| // container. | ||
| - logger.Warningf("failed to prepare container %q network config: %v", machineId, err) | ||
| + logger.Debugf("failed to prepare container %q network config: %v", machineId, err) |
reedobrien
Jan 9, 2017
Contributor
SEt back to info, though I'm hopeful that the new dynamic bridging stuff makes this go away.
| - logger.Infof(err.Error()) | ||
| + // Logged as debug here because it is logged as an error in | ||
| + // worker/provisioner. | ||
| + logger.Debugf(err.Error()) |
rogpeppe
Jan 9, 2017
Owner
There's no point in logging error messages that we return (especially when they're
less informative than the error that we're returning).
The appropriate place for logging errors is if we're discarding them.
reedobrien
Jan 9, 2017
Contributor
I agree. This was existing code. I just wanted to turn down the amount of logging of the same error.
Removed.
| @@ -203,7 +203,7 @@ func (commandWrapperSuite) TestAutostartMachineFails(c *gc.C) { | ||
| container := NewTestContainer("aname", stub.Run, nil) | ||
| err := AutostartMachine(container) | ||
| c.Assert(stub.Calls(), jc.DeepEquals, []string{"virsh autostart aname"}) | ||
| - c.Assert(err, gc.ErrorMatches, "Boom") | ||
| + c.Check(err, gc.ErrorMatches, "failed to autostart domain \"aname\": Boom") |
reedobrien
Jan 9, 2017
Contributor
I usually use backquotes. Not sure what I was thinking...well I do, but prolly not worth the story. Fixed.
| @@ -70,7 +70,7 @@ func (broker *kvmBroker) StartInstance(args environs.StartInstanceParams) (*envi | ||
| if err != nil { | ||
| // It's not fatal (yet) if we couldn't pre-allocate addresses for the | ||
| // container. | ||
| - logger.Warningf("failed to prepare container %q network config: %v", machineId, err) | ||
| + logger.Debugf("failed to prepare container %q network config: %v", machineId, err) |
reedobrien
Jan 9, 2017
Contributor
SEt back to info, though I'm hopeful that the new dynamic bridging stuff makes this go away.
|
!!blah!! |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
reedobrien commentedJan 8, 2017
•
Edited 1 time
-
reedobrien
Jan 8, 2017
Add annotations to error messages from shelling out so we get informative error
messages. It currently reports "exit 1". Also change a couple logging statments Debug as the
caller logs an error and they just duplicate the information noisily.
QA: (This currently fails on develop but when the network code @jameinel and @frobware are working on lands, it won't fail anymore.)
10 .juju add-relation wordpress mysql
Check the logs and ensure we don't have a bunch of duplicate failure messages and that there is reasonable information regarding the failure.
juju debug-log --level DEBUG --replay --include machine-1
juju debug-log --level DEBUG --replay --include machine-0
Cleanup: juju kill-controller kvm/purego -y