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

Change logging format #370

Merged
merged 4 commits into from
Aug 29, 2024
Merged

Change logging format #370

merged 4 commits into from
Aug 29, 2024

Conversation

nabuskey
Copy link
Collaborator

Currently, log outputs are hard to read. For example:

time=2024-08-26T20:22:19.468Z level=INFO msg="Creating kind cluster" logger=setup cluster=localdev
time=2024-08-26T20:22:36.409Z level=INFO msg="Done creating cluster" logger=setup cluster=localdev
time=2024-08-26T20:22:36.566Z level=INFO msg="Adding CRDs to the cluster" logger=setup
time=2024-08-26T20:22:37.377Z level=INFO msg="Setting up CoreDNS" logger=setup
time=2024-08-26T20:22:37.404Z level=INFO msg="Setting up TLS certificate" logger=setup
time=2024-08-26T20:22:37.425Z level=INFO msg="Creating localbuild resource" logger=setup
time=2024-08-26T20:22:37.425Z level=INFO msg="Starting EventSource" controller=localbuild controllerGroup=idpbuilder.cnoe.io controllerKind=Localbuild source="kind source: *v1alpha1.Localbuild"

There is a lot of repeated texts that adds no value.

With this PR log messages look like:

Aug 27 19:45:47 INFO Creating kind cluster logger=setup
Aug 27 19:45:47 INFO Runtime detected logger=setup provider=docker
Aug 27 19:45:47 INFO Cluster already exists logger=setup cluster=localdev
hi
Aug 27 19:45:47 INFO Adding CRDs to the cluster logger=setup
Aug 27 19:45:47 INFO Setting up CoreDNS logger=setup
Aug 27 19:45:47 INFO Setting up TLS certificate logger=setup
Aug 27 19:45:47 INFO Creating localbuild resource logger=setup
Aug 27 19:45:47 INFO Starting EventSource controller=localbuild controllerGroup=idpbuilder.cnoe.io controllerKind=Localbuild source=kind source: *v1alpha1.Localbuild

This also adds colored output option. Disabled by default.

image

Signed-off-by: Manabu McCloskey <[email protected]>
Signed-off-by: Manabu McCloskey <[email protected]>
Signed-off-by: Manabu McCloskey <[email protected]>
@cmoulliard
Copy link
Contributor

Don't forget to document how to enable/disable the colored output ;-)

@nabuskey
Copy link
Collaborator Author

Don't forget to document how to enable/disable the colored output ;-)

Yep. Plan is to make an issue to track it if this gets merged :)

@tapas4java
Copy link
Contributor

Any problem if we make colored output default to true ? Anyway it's not going to break anything

@nabuskey
Copy link
Collaborator Author

Coloring is very subjective and I didn't want to impose what I like to others by default. If others like it over time, we can make it enabled by default imo.

pkg/cmd/root.go Outdated Show resolved Hide resolved
Signed-off-by: Manabu McCloskey <[email protected]>
@nimakaviani nimakaviani self-requested a review August 29, 2024 18:41
Copy link
Contributor

@nimakaviani nimakaviani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nimakaviani nimakaviani merged commit fba981a into cnoe-io:main Aug 29, 2024
5 checks passed
@nabuskey nabuskey deleted the colored-logging branch August 29, 2024 18:47
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