-
Notifications
You must be signed in to change notification settings - Fork 18
Changes from all commits
8324265
7097159
9fd4fa8
035a86b
077af52
fb41e5f
26f2b09
7faa8ae
efdc2d8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package extpoints | ||
|
||
import ( | ||
"github.com/Sirupsen/logrus" | ||
"github.com/taskcluster/taskcluster-worker/engines" | ||
"github.com/taskcluster/taskcluster-worker/plugins" | ||
"github.com/taskcluster/taskcluster-worker/runtime" | ||
|
@@ -11,13 +12,14 @@ import ( | |
// | ||
// We wrap all arguments so that we can add additional properties without | ||
// breaking source compatibility with older plugins. | ||
// Note: This is passed by-value for efficiency (and to prohibit nil), if | ||
// adding any large fields please consider adding them as pointers. | ||
// Note: This is intended to be a simple argument wrapper, do not add methods | ||
// to this struct. | ||
type PluginOptions struct { | ||
environment *runtime.Environment | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, but this wasn't really intended as a doc comment... more as a source comment for people to read when they were editing the struct... |
||
engine *engines.Engine | ||
// Note: This is passed by-value for efficiency (and to prohibit nil), if | ||
// adding any large fields please consider adding them as pointers. | ||
// Note: This is intended to be a simple argument wrapper, do not add methods | ||
// to this struct. | ||
log *logrus.Entry | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So if we did the interface thing... We could to just hide the |
||
} | ||
|
||
// The PluginProvider interface must be implemented and registered by anyone | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
package runtime | ||
|
||
import ( | ||
"fmt" | ||
|
||
"github.com/Sirupsen/logrus" | ||
) | ||
|
||
// Create a logger that can be passed around through the environment. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Start docs comments like this with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is there something I should enable for it to warn me about things like this? 'make test' nor running 'go vet' directly has indicating any problems. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good call on changing this documentation by the way, still getting used to how things should be documented. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My bad, I think it is https://github.com/golang/lint |
||
// Loggers can be created based on the one returned from this method by calling | ||
// WithField or WithFields and specifying additional fields that the package | ||
// would like. | ||
func CreateLogger(level string) (*logrus.Logger, error) { | ||
if level == "" { | ||
level = "warn" | ||
} | ||
|
||
lvl, err := logrus.ParseLevel(level) | ||
if err != nil { | ||
return nil, fmt.Errorf("Unable to parse logging level: %s\n", level) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice, didn't know about Note: not sure if it's always wise to wrap errors... I guess it can't really hurt here... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For all errors that we wrap, we should include the underlying error in the message, so we don't lose the root cause - i.e. include an extra There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currrently the only error that method returns is "not a valid logrus Level: ". My attempt here was to remove the use of the 'logrus' word in the error message as the user doesn't care about what that package is or what it means. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Unless we're explicitly handling the error... because we know what it means but yeah, generally I agree... |
||
} | ||
|
||
logger := logrus.New() | ||
logger.Level = lvl | ||
return logger, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
package runtime | ||
|
||
import ( | ||
"fmt" | ||
"reflect" | ||
"testing" | ||
|
||
"github.com/Sirupsen/logrus" | ||
"github.com/stretchr/testify/assert" | ||
) | ||
|
||
func TestCreateLogger(t *testing.T) { | ||
logger, err := CreateLogger("debug") | ||
assert.Equal(t, err, nil, fmt.Sprintf("Error should not have been returned. %s", err)) | ||
assert.Equal(t, reflect.TypeOf(logger).String(), "*logrus.Logger") | ||
assert.Equal(t, logger.Level, logrus.DebugLevel) | ||
} | ||
|
||
func TestLoggerNotCreatedWithInvalidLevel(t *testing.T) { | ||
_, err := CreateLogger("debug1234") | ||
assert.NotEqual(t, err, nil, fmt.Sprintf("Error should not have been returned. %s", err)) | ||
} | ||
|
||
func TestDefaultWarnLevel(t *testing.T) { | ||
logger, err := CreateLogger("") | ||
assert.Equal(t, err, nil, fmt.Sprintf("Error should not have been returned. %s", err)) | ||
assert.Equal(t, reflect.TypeOf(logger).String(), "*logrus.Logger") | ||
assert.Equal(t, logger.Level, logrus.WarnLevel) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit, but
panic(err)
is just more elegant...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think
panic
is appropriate for crashes, but not appropriate for handling bad input - i.e. a user should never see a stack trace because he/she passed in an unrecognised logging level. In any case, I think we should process the underlying cause and crash if really an internal error, or report the bad value, if it is just a bad value. We could also consider creating and documenting different exit codes for different failures, although I believe exit code 1 is appropriate for invalid arguments.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do not need a stack trace, nor do we need to run deferred functions, so panic does not seem like the best suited option here. This is an error we can't recover from, and should exit immediately. It also allows us to define exit codes for things we know went wrong.
Panic seems more appropriate when it happens lower down in the execution of the worker (like if we had a plugin hit some unrecoverable state and we need to unwind the stack all the way up) and complete any deferred functions for cleanup.
For my understanding of what is idiomatic in Go, and in the case of using panic here, what is elegant about using panic vs this method of exiting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree... sorry I'm wrong... panic is bad here ! :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for raising the question about it though, it really gets me thinking about why something is better than the alternative rather than just copying from other places :)