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

feat: add an AsLogr function to use flash as a logr.Logger #1

Merged
merged 2 commits into from
Oct 14, 2021

Conversation

clementnuss
Copy link
Contributor

Useful for e.g. Kubernetes controller-runtime, which expects:

a log set of interfaces called logr (https://godoc.org/github.com/go-logr/logr)

This adds 2 imports and probably makes flash a bit "heavier", it's debatable whether this utility function is useful or not :)

@coveralls
Copy link

coveralls commented Oct 4, 2021

Pull Request Test Coverage Report for Build 1340776183

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.0%) to 99.231%

Totals Coverage Status
Change from base Build 450325039: -0.0%
Covered Lines: 129
Relevant Lines: 130

💛 - Coveralls

@zbindenren
Copy link
Member

Useful for e.g. Kubernetes controller-runtime, which expects:

a log set of interfaces called logr (https://godoc.org/github.com/go-logr/logr)

This adds 2 imports and probably makes flash a bit "heavier", it's debatable whether this utility function is useful or not :)

A am not sure if we want to add those dependencies for the k8s use case. It is already possible to use the logger in k8s with one function call:

package main

import (
	"github.com/go-logr/zapr"
	"github.com/postfinance/flash"
)

func main() {
	l := flash.New()

	z := zapr.NewLogger(l.Desugar())

	z.Info("info")
	z.V(0).Info("info")
}

Maybe mention in the Readme how to use the logger for k8s?

@marcsauter What are your thouthts?

@marcsauter
Copy link

I agree, mention it in the README should be enough.

@clementnuss
Copy link
Contributor Author

I have added an example in the README.md

And while I was at it, I also upgrade the module versions :)

@zbindenren zbindenren merged commit e9ea52c into postfinance:main Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants