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

feat: Auto-detect term for coloring helm-diff output #24

Merged
merged 2 commits into from
Apr 14, 2022

Conversation

mumoshu
Copy link
Contributor

@mumoshu mumoshu commented Apr 6, 2022

Since helm-diff has added an ability to auto-detect the term to decide if it should output with color or not, helmfile had been defaulted to no-color.
This resoloves that, by adding a term-detection logic that is same as helm-diff.

As a part of this work, I have also implemented a new global flag --color, which is used for forcing color without relying on the term-detection logic implemented in helmfile or explicitly setting the HELM_DIFF_COLOR envvar. I hope it is useful for folks.

Ref roboll/helmfile#2043

@mumoshu mumoshu requested a review from itscaro as a code owner April 6, 2022 02:38
Since helm-diff has added an ability to auto-detect the term to decide if it should output with color or not, helmfile had been defaulted to no-color.
This resoloves that, by adding a term-detection logic that is same as helm-diff.

As a part of this work, I have also implemented a new global flag `--color`, which is used for forcing color without relying on the term-detection logic implemented in helmfile or explicitly setting the HELM_DIFF_COLOR envvar. I hope it is useful for folks.

Ref roboll/helmfile#2043

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
@mumoshu mumoshu force-pushed the auto-detect-term-for-color-diff branch from cc385cd to 97e0ca7 Compare April 6, 2022 02:38
@itscaro
Copy link
Contributor

itscaro commented Apr 6, 2022

I would like to have an exclusive validation on --color and --no-color, what do you think?

@mumoshu
Copy link
Contributor Author

mumoshu commented Apr 8, 2022

@itscaro Sounds good!

@mumoshu
Copy link
Contributor Author

mumoshu commented Apr 8, 2022

@itscaro I just reread the codebase and it's a bit harder than I thought because there seems no right place to add an exclusiveness validation.

The functions like Color and NoColor in configImpl lacks an error return value so we need all those instances in the app and the test code.

Or perhaps there's maybe a better place to put such validation? Perhaps immediately before initializing App or Run structs? 🤔

WDYT?

@itscaro
Copy link
Contributor

itscaro commented Apr 8, 2022

Yes we can do some validation before initializing app, maybe around here https://github.com/helmfile/helmfile/blob/f1a250c/main.go#L996-L1011

Signed-off-by: Yusuke Kuoka <ykuoka@gmail.com>
@mumoshu mumoshu force-pushed the auto-detect-term-for-color-diff branch from 497e229 to f84ab66 Compare April 13, 2022 23:43
@mumoshu
Copy link
Contributor Author

mumoshu commented Apr 13, 2022

@itscaro Added f84ab66 and manually tested it. It seems to be working!

@mumoshu mumoshu merged commit 42ad7be into main Apr 14, 2022
@mumoshu mumoshu deleted the auto-detect-term-for-color-diff branch April 14, 2022 22:24
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2022
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.

None yet

2 participants