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

@CyrusNajmabadi CyrusNajmabadi commented Jul 3, 2018

No description provided.


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

@CyrusNajmabadi CyrusNajmabadi Jul 3, 2018

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.

Loading

@@ -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

@CyrusNajmabadi CyrusNajmabadi Jul 3, 2018

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.

Loading

Copy link
Contributor

@ellismg ellismg Jul 4, 2018

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.

Loading

Copy link
Contributor Author

@CyrusNajmabadi CyrusNajmabadi Jul 6, 2018

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.

Loading

Color: cmdutil.GetGlobalColorization(),
}

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

@CyrusNajmabadi CyrusNajmabadi Jul 3, 2018

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.

Loading

@CyrusNajmabadi CyrusNajmabadi requested a review from ellismg Jul 3, 2018
Cyrus Najmabadi added 2 commits Jul 3, 2018
@tamsky
Copy link

@tamsky tamsky commented Jul 5, 2018

Related issues this may close

Loading

@ellismg
Copy link
Contributor

@ellismg ellismg commented Jul 6, 2018

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

Loading

@CyrusNajmabadi
Copy link
Contributor Author

@CyrusNajmabadi CyrusNajmabadi commented Jul 6, 2018

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

Yes. I can look into that.

Loading

@CyrusNajmabadi
Copy link
Contributor Author

@CyrusNajmabadi CyrusNajmabadi commented Jul 6, 2018

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

Loading

@CyrusNajmabadi
Copy link
Contributor Author

@CyrusNajmabadi CyrusNajmabadi commented Jul 6, 2018

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...

Loading


// 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

@ellismg ellismg Jul 7, 2018

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?

Loading

Copy link
Contributor Author

@CyrusNajmabadi CyrusNajmabadi Jul 7, 2018

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.

Loading

@CyrusNajmabadi CyrusNajmabadi merged commit 3ca56d1 into master Jul 7, 2018
3 checks passed
Loading
@CyrusNajmabadi CyrusNajmabadi deleted the noColor branch Jul 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants