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

Add --no-color option to disable color output #1889

Merged
merged 9 commits into from
Aug 10, 2023

Conversation

caesarshift
Copy link
Contributor

@caesarshift caesarshift commented Jul 7, 2023

Description

Add --no-color option to disable color output

Related Issue

Fixes #890

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Other (security config, docs update, etc)

Checklist before merging

@netlify
Copy link

netlify bot commented Jul 7, 2023

Deploy Preview for zarf-docs canceled.

Name Link
🔨 Latest commit 6c3a5ed
🔍 Latest deploy log https://app.netlify.com/sites/zarf-docs/deploys/64d4f743c131c7000805b756

@caesarshift
Copy link
Contributor Author

I created this as a draft MR in order to get feed back on this note

@YrrepNoj
Copy link
Contributor

YrrepNoj commented Jul 7, 2023

Replying to this comment @caesarshift left on the issue: #890 (comment)

The colored log message that says where we're loading the config file from is happening in the initViper(...) function.

That initViper(...) is called by the init function for root.go. Since it's called by an init function, we can not rely on any configuration to be set yet.

If we want to make it so the log message for which config file is being used follows the same configuration as the rest of the logs, we could just move this line to after we have called cliSetup(...)

@caesarshift
Copy link
Contributor Author

Replying to this comment @caesarshift left on the issue: #890 (comment)

The colored log message that says where we're loading the config file from is happening in the initViper(...) function.

That initViper(...) is called by the init function for root.go. Since it's called by an init function, we can not rely on any configuration to be set yet.

If we want to make it so the log message for which config file is being used follows the same configuration as the rest of the logs, we could just move this line to after we have called cliSetup(...)

Based on your comment, I've added a follow-on commit that moves the print statements into the cli setup (moving it into the function preserves the order of the output to precede the current log level output)

@caesarshift caesarshift changed the title Draft: Add --no-color option to disable color output Add --no-color option to disable color output Jul 7, 2023
@caesarshift caesarshift marked this pull request as ready for review July 7, 2023 19:26
@caesarshift caesarshift force-pushed the 890-no-color branch 4 times, most recently from b06ddd8 to 8bf0fc8 Compare July 10, 2023 17:51
@caesarshift caesarshift requested a review from cmwylie19 as a code owner August 8, 2023 13:23
@caesarshift
Copy link
Contributor Author

FYSA, due to a merge conflict in src/config/config.go, I also rebased the branch.

Copy link
Contributor

@Racer159 Racer159 left a comment

Choose a reason for hiding this comment

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

lgtm

cmwylie19
cmwylie19 previously approved these changes Aug 8, 2023
Copy link
Contributor

@cmwylie19 cmwylie19 left a comment

Choose a reason for hiding this comment

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

lgtm

src/cmd/viper.go Outdated Show resolved Hide resolved
Copy link
Contributor

@Racer159 Racer159 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@cmwylie19 cmwylie19 left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@cmwylie19 cmwylie19 left a comment

Choose a reason for hiding this comment

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

lgtm

@Racer159 Racer159 merged commit bd6b92c into zarf-dev:main Aug 10, 2023
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.

zarf command line execution error logging and log file readability
5 participants