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: FTL_LOG_COLOR env to override tty detection #1508

Merged
merged 1 commit into from
May 17, 2024

Conversation

gak
Copy link
Contributor

@gak gak commented May 16, 2024

LOG_COLOR env to override tty detection

eg LOG_COLOR=1 LOG_LEVEL=trace go test -v ./buildengine -run TestEngine

@gak gak requested a review from alecthomas as a code owner May 16, 2024 05:29
@gak gak requested review from a team and worstell and removed request for a team May 16, 2024 05:29
@ftl-robot ftl-robot mentioned this pull request May 16, 2024
@@ -21,7 +21,9 @@ var _ Sink = (*plainSink)(nil)

func newPlainSink(w io.Writer, logTime bool) *plainSink {
var isaTTY bool
if f, ok := w.(*os.File); ok {
if os.Getenv("FTL_LOG_COLOR") != "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make this a Kong flag in log.Config, and pass it down into this function to avoid global state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I implemented this, but my main reason for this feature is in running tests and it now doesn't work with tests because ContextWithNewDefaultLogger doesn't go through log.Config. I did make a env check in there which is only checked when running tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense 👍

@alecthomas
Copy link
Collaborator

Good idea BTW!

@alecthomas alecthomas force-pushed the main branch 8 times, most recently from be01cce to 2fef0b4 Compare May 16, 2024 11:36
@gak gak force-pushed the gak/override-tty-log-colors branch 2 times, most recently from 1f17bde to 238c08d Compare May 16, 2024 21:17
@gak gak requested a review from alecthomas May 16, 2024 21:23
@@ -66,5 +66,7 @@ func ContextWithLogger(ctx context.Context, logger *Logger) context.Context {
}

func ContextWithNewDefaultLogger(ctx context.Context) context.Context {
return ContextWithLogger(ctx, Configure(os.Stderr, Config{Level: Debug}))
// Matches LOG_COLOR in log.Config. This is a special case for the default logger in testing.
color := os.Getenv("LOG_COLOR") != ""
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

LOG_COLOR=1 LOG_LEVEL=trace go test -v ./buildengine -run TestEngine
Copy link
Collaborator

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

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

Testing

@gak gak merged commit 9d46ee1 into main May 17, 2024
23 checks passed
@gak gak deleted the gak/override-tty-log-colors branch May 17, 2024 00:03
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.

2 participants