Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

[WIP][PAN-2030] Add formating and color to logging in CLI #1879

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

pscott
Copy link

@pscott pscott commented Aug 23, 2019

PR description

Add formatting and color to logging in CLI

Fixed Issue(s)

fixes PAN-2030

@CLAassistant
Copy link

CLAassistant commented Aug 23, 2019

CLA assistant check
All committers have signed the CLA.

@NicolasMassart
Copy link
Contributor

Thanks @pscott, I like your way to start contributing and welcome to this community.
I assigned myself to your PR as I will follow it and merge it once you will finish the job (when you remove the WIP and tell me I can merge). I also assigned two reviewers for this PR, including @jakehaugen who wrote the Jira ticket.

@ajsutton
Copy link
Contributor

We'll need to think carefully about this - currently Pantheon only logs to stdout and then when run in a server environment that is piped to a file. We need to ensure the file doesn't get filled with junk characters making the logs unreadable. I suspect that will require detecting if output is actually going to a console and only using color in that case (possibly along with a CLI option to explicitly enable or disable color).

@NicolasMassart
Copy link
Contributor

I suspect that will require detecting if output is actually going to a console and only using color in that case (possibly along with a CLI option to explicitly enable or disable color).

This is indeed the goal of https://pegasys1.atlassian.net/browse/PAN-2021 which is linked in https://pegasys1.atlassian.net/browse/PAN-2030

@ajsutton
Copy link
Contributor

PAN-2021 doesn't include the auto-detect requirement and we need to land both at the same time - we can't break logging by filling it full of color codes. We could default to no colours with an option to enable before adding auto detection, but we ended o either auto detect or default to off.

@pscott It's great that you've picked this up, sorry that all the details aren't entirely sorted but we'd love to work through them with you.

@NicolasMassart
Copy link
Contributor

NicolasMassart commented Sep 5, 2019

@pscott we are currently discussing changing the linked issue https://pegasys1.atlassian.net/browse/PAN-2021 to include a default to no color. Then this PR may have to include this change to make sure we remain compatible with the way terminal output works today. I'll keep you informed when we confirm the decision. Thanks for being so patient.

@NicolasMassart
Copy link
Contributor

@pscott I updated PAN-2021 to reflect the off by default requirement. Would you like to work on it too as it's very closely related to this one? You could even make the changes for both in this PR IMO.

@pscott
Copy link
Author

pscott commented Sep 9, 2019

Yes I believe everything should fit in this PR. WIP!

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.

4 participants