Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

Resubmit PR# 2332: Enable colorized logging on Windows #2482

Merged
merged 5 commits into from
Mar 20, 2018
Merged

Resubmit PR# 2332: Enable colorized logging on Windows #2482

merged 5 commits into from
Mar 20, 2018

Conversation

drdarshan
Copy link
Contributor

This is resubmitting my previous PR #2332. I tried out @jackfrancis's updates and they work really well. I had to delete my fork for another reason so I needed to recreate the PR with the cumulative changes.

What this PR does / why we need it: On Windows, logging prints escape codes that do not render properly. I used a workaround suggested in https://github.com/mattn/go-colorable.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

Copy link
Member

@jackfrancis jackfrancis left a comment

Choose a reason for hiding this comment

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

lgtm

@jackfrancis jackfrancis merged commit 4468670 into Azure:master Mar 20, 2018
@EPinci
Copy link
Contributor

EPinci commented Mar 22, 2018

Looks like not all string are colorized properly (v0.14.4).
During an upgrade, only the first three lines are colorized:

INFO[0002] validating...
INFO[0008] Name suffix: 93283987
INFO[0008] Gathering agent pool names...
?[36mINFO?[0m[0009] Master VM name: k8s-master-93283987-0, orchestrator: Kubernetes:1.9.3 (MasterVMs)
?[36mINFO?[0m[0009] Master VM name: k8s-master-93283987-1, orchestrator: Kubernetes:1.9.3 (MasterVMs)
?[36mINFO?[0m[0009] Master VM name: k8s-master-93283987-2, orchestrator: Kubernetes:1.9.3 (MasterVMs)

Thank you.

@jackfrancis
Copy link
Member

@EPinci I see the same thing (only those first 3 INFO lines are colorized), but I don't see the symptoms below it (lines starting with ?), I just see non-colorized log output.

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

Successfully merging this pull request may close these issues.

3 participants