Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revisiting logging in the scep package #62

Open
kenkouot opened this issue Aug 9, 2017 · 1 comment
Open

Revisiting logging in the scep package #62

kenkouot opened this issue Aug 9, 2017 · 1 comment

Comments

@kenkouot
Copy link

kenkouot commented Aug 9, 2017

Currently, the scep package has go-kit as dependency for the sole purpose of logging. This approach has some downsides for consumers of the scep package that aren't also using the server/client implementations:

  • consumers may already use an existing, incompatible logging impl. with specific output configurations for their logging setup
  • enabling logging and what to log should generally be a function of the application, and not the library. Most of the information logged in the scep package is accessible from the caller, and can therefore be logged from the invoking context

My ideal is that the scep package takes a more Go standard library approach and implement no logging (and ideally panics), and instead rely on returning errors to the caller. Alternatively, if this doesn't work for you, the package could expose it's own local logging interface that consumers can satisfy.

@groob
Copy link
Member

groob commented Aug 9, 2017

I'm going to go with the second option, re-declaring kit/log.Logger as a local interface and accepting that as a dependency instead (defaulting to a noop logger).

Initially this library had no logging (which is something I prefer in many cases) but SCEP is pretty bad with error handling. Additionally many of the requests I get the user has no direct access to either their client or server, making debugging even more of a pain.

So I opted to add a debug logger that can be enabled to provide context for what's going on in the PKIOperation steps.

But I agree with you about the value of reducing the dependency graph. Thanks for the suggestion!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants