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

Support the NO_COLOR env variable to suppres any colored output. #1594

Merged
merged 9 commits into from
Jul 7, 2018

Conversation

CyrusNajmabadi
Copy link
Contributor

No description provided.


opts := backend.DisplayOptions{
Color: cmdutil.GetGlobalColorization(),
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For command that do not support a --color flag, this is the pattern to use. The color picked here will be "Never" if NO_COLOR is set in the environment. Or 'Always' otherwise. In the future we can consider if these command shoudl support --color as well. But i didn't want to have to add that here.

@@ -69,7 +75,7 @@ func newCancelCmd() *cobra.Command {

msg := fmt.Sprintf("%sThe currently running update for '%s' has been canceled!%s", colors.SpecAttention, s.Name(),
colors.Reset)
fmt.Println(colors.ColorizeText(msg))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no one should call colors.ColorizeText now. Only the very lowest level (that is calling into 'lorely') now does this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to stop exporting it? A brief look at the code makes me think we may be able to do but I am not sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup. I've stopped exporting it.

Color: cmdutil.GetGlobalColorization(),
}

s, err := requireStack(stack, false, opts)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Many interactive functions now take in opts so they can decide if they should colorize or not.

@CyrusNajmabadi CyrusNajmabadi requested a review from ellismg July 3, 2018 22:56
Cyrus Najmabadi added 2 commits July 3, 2018 16:26
@tamsky
Copy link

tamsky commented Jul 5, 2018

@ellismg
Copy link
Contributor

ellismg commented Jul 6, 2018

Would it be possible as part of this to make the --color flag global across all commands?

@CyrusNajmabadi
Copy link
Contributor Author

Would it be possible as part of this to make the --color flag global across all commands?

Yes. I can look into that.

@CyrusNajmabadi
Copy link
Contributor Author

Making this a global flag worked, and def cleaned things up.

@CyrusNajmabadi
Copy link
Contributor Author

Note: at this point... passing the variable around serves almost no purpose. It's effectively just a process-wide global that is set right at startup...

I'm going to keep as is. but we could literally just remove all usages of passing things around...


// GetGlobalColorization gets the global setting for how things should be colored.
// This is helpful for the parts of our stack that do not take a DisplayOptions struct.
func GetGlobalColorization() colors.Colorization {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would have assumed this would have to check the global c value you in the root pulumi command? Is that done some other place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's somewhat the reverse. the pulumi command just goes and sets this, so that it's available for anyone to go find if they need.

That's what i meant in my comment about us not needing to really pass along any 'color' option anymore. There is always just a well known place it can be found.

@CyrusNajmabadi CyrusNajmabadi merged commit 3ca56d1 into master Jul 7, 2018
@CyrusNajmabadi CyrusNajmabadi deleted the noColor branch July 7, 2018 04:30
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.

3 participants