cloudfoundry / eirini

Pluggable container orchestration for Cloud Foundry, and a Kubernetes backend
Apache License 2.0
115 stars 30 forks source link

Stop the route emitter logging directly to stdout #47

Closed williammartin closed 5 years ago

williammartin commented 5 years ago

The route emitter structure has a few fmt.Printlns sprinkled around. This is not ideal for a couple of reasons:

  1. When unit testing, expected error messages get printed out to the test stdout, which is confusing when all the tests are actually passing.
  2. Having some library depths print to stdio streams is pretty annoying, there's no way to turn it off.

This PR introduces a log to the Emitter struct that can be injected. In tests we then multiwriter to a buffer we can assert on (turns out, these error logs were not actually being tested), and to the GinkgoWriter, so if a test fails we still get the output.

A few further notes on this:

This problem is also in the metrics emitter, but I didn't fix it there in case you reject this.

cfdreddbot commented 5 years ago

:white_check_mark: Hey williammartin! The commit authors and yourself have already signed the CLA.

cf-gitbot commented 5 years ago

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/165100989

The labels on this github issue will be updated when the story is started.

gdankov commented 5 years ago

Thank you for yet another great PR!

We discussed the change and we agree with the points you made. We would also prefer to have an interface for the logger, instead of passing a plain io.Writer. One option (which we already use in some places) is lager.

We'll merge this and throw a chore in our tracker to eventually use lager everywhere.

Feel free to PR the metrics collector code if you wish, preferably with lager.

@gdankov & @JulzDiverse