added interface for logger

[dhuckins]

using an interface instead of the std lib struct type for the logger allows for more flexibility with users setting custom logger

[prskr]

Just out of curiosity: why not using github.com/go-logr/logr?
That is already a rather well-known abstraction e.g. used in Kubernetes that has pre-built integrations for some popular logging libraries out there?

[dhuckins]

Just out of curiosity: why not using github.com/go-logr/logr? That is already a rather well-known abstraction e.g. used in Kubernetes that has pre-built integrations for some popular logging libraries out there?

was looking for a simpler PR (adding another dependency/refactoring the existing logging in this app would need some communication/additional steps/etc)

this allows anything with Printf(format string, v ...interface{}) to be used as a logger eg:

• zerolog
• logrus

or whatever custom logger someone has cooked up

[prskr]

As I said, just an idea! Although there are only 4 places where the logger is used anyway 😄
Just stumbled upon this PR and thought after I made this multiple times previously without actually improving anything by introducing a custom logging interface I'd throw in my 2 cents in 😄

[dhuckins]

As I said, just an idea! Although there are only 4 places where the logger is used anyway 😄 Just stumbled upon this PR and thought after I made this multiple times previously without actually improving anything by introducing a custom logging interface I'd throw in my 2 cents in 😄

fair enough! my goal with the PR was to have a custom logger (basically an interface instead of a struct) with the smallest change possible
if the owners/contributors want something different, I'm definitely open to changing

[mdelapenya]

there are only 4 places where the logger is used anyway 😄

TBH I do not have a strong opinion on this. I'd be more than happy with a community effort to decide where to go here :)

[prskr]

What about:

type TestLogger interface {
    Logf(format string, args ...interface{})
    // optionally:
    Errorf(format string, args ...interface{})
}

This is already compatible with testing.TB and therefore could be used in most testing scenarios without any adapter.

I'd propose to get rid of the global Logger variable completely but move it to the ContainerRequest to allow users passing their own logger per container to avoid issues in parallel tests and use a DiscardLogger or something similar in case no logger is set?

[gianarb]

Ciao! This is a highly opinionated and kind of complex topic. It is hard to get an agreement on what logging interface we should use. But we need to take a decision.

I like what is proposed right now because it is simple. And looking at the logs we span today, we do not need anything complex. I understand why log level are important, but I think we can build up a sort of convention when needed passing a k/v with key=level for example. I think Printf(frmat string, v ...interface{}) supports that via v parameter.

Having a level is important because if needed you can decide what gets forwarded to stdout and what to sderr.

I would like to do with the proposed one. @baez90 wdyt?

[prskr]

😄 I see, tbh I did not really think of the whole 'level' debate - I rather had in mind what consequences an error should have in a testcase when I'm starting a testcontainer and how to implement the interface as easy as possible to get the logs in my test summary.

Having a distinguished case for logging errors makes it easier e.g. to build an adapter that propagates them to testing.T.Errorf(...) or testing.T.Fatalf(...) while the proposed Printf(format string, v ...interface{}) would at least require to if err, ok := v[n].(error); ok { ... } for all vs (or at least len(v)%2) to get the error if present and handle it so that my test case fails eventually.
So my original proposal would probably make more sense if it would look like this:

type Logger interface {
    Info(msg string, keysAndValues ...interface{})
    Error(err error, msg string, keysAndValues ...interface{})
}

(which is btw. exactly the same signature as in go-logr 😄 )

But in the end I think @gianarb is totally right, the original proposal is totally fine and is open enough for everyone to implement their specific behavior based on their personal preference 😊

Edit: the longer I think about it the more I come to the conclusion that the original proposal might be a better fit. Is there a chance to include a func TestLogger(tb testing.TB) Logger { ... } to easily include the logs in the test output? 🙈

[gianarb]

Edit: the longer I think about it the more I come to the conclusion that the original proposal might be a better fit. Is there a chance to include a func TestLogger(tb testing.TB) Logger { ... } to easily include the logs in the test output?

Nice idea, @dhuckins do you mind to add this bit so we can proceed and merge?
Thanks a lot

[prskr]

I can also provide this as part of a follow up PR.

I think it could make sense to be able to optionally set the logger as part of the ContainerRequest because otherwise setting a global logger based on a testing.T in a scenario of parallel tests could get really confusing 😅

But independent of if this makes sense or not you could merge this PR already and further discussions could take place in a follow up so @dhuckins doesn't have to wait any longer?

[gianarb]

Yes we can do it as a follow up PR you are right. @baez90 yes it will be great to be able to override a logger for a specific container, probably with a function like WithLogger(logger) somewhere

[dhuckins]

sorry, have been busy last few days
thank you @baez90 and @gianarb!