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

Detect CI environments for default no-color value #330

Merged
merged 7 commits into from Jun 16, 2023

Conversation

silverwind
Copy link
Contributor

@silverwind silverwind commented May 11, 2023

Use go-supportscolor to detect color support for CI environments and more, which among GitHub Actions will enable color support on number of other environments which previously could not display color at all with gotestsum.

@silverwind silverwind changed the title Enable color support by default on Drone CI Detect CI color support via go-supportscolor May 11, 2023
Drone CI supports 256 colors, so color can be enabled by default, even
when stdout is not a TTY.
@dnephin
Copy link
Member

dnephin commented May 20, 2023

Thank you for the PR! It would be great to enable color by default on any CI systems that support it. I believe color should already be enabled by default in any interactive terminal.

I'd like to avoiding adding another dependency for this. I believe all we need is a few of the lines around here: https://github.com/jwalton/go-supportscolor/blob/198795bbe247f127f76b501948f2f41fc88b8080/supportscolor.go#L213-L215

Some systems, like CircleCI already do use color, because they properly emulate a terminal.

It's also worth noting that it's always possible to enable color using --no-color=false. This change would enable color by default in some environments, but it should have already been possible to enable color in those environments with the command line flag.

@silverwind
Copy link
Contributor Author

I'd like to avoiding adding another dependency for this

Having it via a dependency ensures that future environments will only need to be supported in one repo instead of hundrets. I see a benefit in doing it via a dependency, even if it does "a bit more" like this one does.

I planned to PR support for a the new Gitea Actions environment into go-supportscolor, but currently it is struggling with 256 colors, that's why I held off from it.

In case you still don't want go-supportscolor, I will alter the PR as per the recommendations, remove the dependency and also include GITEA_ACTIONS=true detection for Gitea Actions which can do 16 color fine currently, and I think that is all this module uses.

--no-color=false: I searched for an CLI option to force-enable color, but it wouldn't have dawned me that this double negation actually would do the trick. Good to know, but I would suggest a --color or --force-color option as that is much more obvious.

@dnephin
Copy link
Member

dnephin commented May 21, 2023

I will alter the PR as per the recommendations, remove the dependency and also include GITEA_ACTIONS=true detection for Gitea Actions

That sounds great! Thank you for making this change!

I would suggest a --color option as that is much more obvious.

I agree it is not very obvious right now. Hiding the --no-color option and changing to --color would be a great improvement. We can continue to default that flag to true when a terminal or supported CI system is detected.

@silverwind silverwind changed the title Detect CI color support via go-supportscolor Detect CI environments for default no-color value May 23, 2023
@silverwind
Copy link
Contributor Author

silverwind commented May 23, 2023

I have reverted the dependency and have now effectively replicated this block.

@silverwind
Copy link
Contributor Author

silverwind commented May 24, 2023

I think the test failure is related to it detecting the CircleCI environment, will update the fixture.

Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Thank you! This looks great. Sorry it's taking me a while to review. Just one question

cmd/main.go Outdated Show resolved Hide resolved
Copy link
Member

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Thank you!

@dnephin dnephin merged commit 00a4f25 into gotestyourself:main Jun 16, 2023
7 checks passed
@silverwind silverwind deleted the drone branch June 16, 2023 05:38
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.

None yet

2 participants