Added support for -force-color to the CLI.

[apollo13]

This should fix #10973

[apollo13]

@jrasell Would appreciate a review. I have verified manually and it works, not to sure about tests. On the other hand I couldn't really find any for -no-color either :D

[apollo13]

I am pretty sure that the CSI snapshot failures in the test are not my fault! ;)

[apollo13]

I does not yet seem to work in gitlab CI, mhm :/ That will be fun to figure out.

[apollo13]

Okay, so -force-color does work, but the env variable does not. This seems to be caused by the fact that plan calls Meta.Colorize() which only takes flags into account but not the matching env variables. I am not 100% sure how nomad maps env vars to flags, any pointers?

[apollo13]

Okay, so -force-color does work, but the env variable does not. This seems to be caused by the fact that plan calls Meta.Colorize() which only takes flags into account but not the matching env variables. I am not 100% sure how nomad maps env vars to flags, any pointers?

Fixed for this PR, should be ready for review now

[jrasell]

Thanks a lot for this @apollo13 and sorry for the delay in reviewing. I have an line comment which I would appreciate your thoughts on.

Another comment which appears a couple of times would be to remove the (CI systems mainly) statement in the help outputs. I'd prefer the user to understand why they need to use the flag, rather than read the help and maybe assume they can override because of the CI detail. Again I am open to opinions here.

[apollo13]

I have fixed up the flag defaults and it should work as expected now. If there are performance issues we have to reevaluate.

[apollo13]

@lgfa29 I have rebased this PR onto the latest no-color changes in main. While on it I have simplified the code because Colorize doesn't need to check for a terminal since meta.Ui is only set when a terminal exists: https://github.com/hashicorp/nomad/blob/main/main.go#L118 (this is checked in main.go). Given that main.go already parses -no-color itself and puts it into the env there is also no reason to check the no-color flag (which now makes the test fail due to the way they are written :/).

I like how my PR looks now because it only bothers about the flags and env variables in one place. Sadly this "one place" is main.go which probably makes this somewhat annoying to test. I do not really have an idea on how to fix this -- I am open for ideas :) I guess we could somewhat "fix" this by moving the creation of meta.Ui into an utility function that could be tested more easily.

[apollo13]

I have pushed a new commit with a setupUi utility function, cleaned up the code and fixed the tests. Let me know what you think.

[apollo13]

@lgfa29 Any chance you could review this PR? :)

[lgfa29]

@lgfa29 Any chance you could review this PR? :)

Will do it in the coming days 🙂
Sorry of the delay here.

[lgfa29]

Thank you for this great PR @apollo13!

It's great to see this logic being pulled out of main.go since it's now easier to test it 🙂

I pushed a commit to change the logic slightly so that -no-color takes precedence of -force-color. These two options are meant to be used together, but in case they are it would be better to avoid colors since it's a safer option.

What do you think?

Code has beed refatored.

[apollo13]

Personally I think that force is stronger than no so I would have expected force to win, but no strong feelings…

[lgfa29]

Personally I think that force is stronger than no so I would have expected force to win, but no strong feelings…

So now we need a -force-no-color to beat them all 😅

Just kidding, or course. The reasoning behind -no-color to win is that using the two flags at the same time is most likely a mistake, and -no-color is the safest option since it won't pollute the output in case colors are not supported.

I will merge this now then. Thank you for the contribution!

[github-actions]

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.