Add confg values for the new CLI #3294
Conversation
PR ChecklistHow to use this checklistHow to use this checklistPR AuthorFor each section, check a box when it is true. PR ReviewerCheck that the PR checklist action did not fail. Bug ReferencesNone. Confirm
How to properly reference fixed bugs
Test UpdatesUnit Tests
Integration Tests
Documentation
Does this PR require review from someone outside the core ubuntu-pro-client team?
|
|
@dheyay thanks for approving! |
|
|
||
| cls.use_color = ( | ||
| sys.stdout.isatty() | ||
| and os.getenv("NO_COLOR") is None |
There was a problem hiding this comment.
what if the user sets this to NO_COLOR=0 ?
There was a problem hiding this comment.
From: https://no-color.org/
Command-line software which adds ANSI color to its output by default should check for a NO_COLOR
environment variable that, when present and not an empty string (regardless of its value),
prevents the addition of ANSI color.
There was a problem hiding this comment.
btw the same website says that NO_COLOR should be the last checked thing: that if configuration or CLI says show color then you show color regardless.
our CLI will have a no-color boolean option instead of a color=? option that you can set to things like 'yes, enabled, disabled, auto, blablabla' so we good CLI wise
but the configuration is not being respected: NO_COLOR env means no color, I'm ANDing the cases.
If you think this is a problem, we can always rename the config defaults from cli_color = True to disable_color = False and we would get a reasonable behavior.
There was a problem hiding this comment.
(by reasonable I mean that any of the triggers would disable the colors, regardless of order, and none would need precedence)
renanrodrigo
force-pushed
the
new-ux-config
branch
from
695b503
to
07a6b84
Compare
This fixes a silent bug were users could run: pro config set apt_news=yes pro config set apt_news=please and this would set it to False. Now we validate the value set is actually True or False. Signed-off-by: Renan Rodrigo <[email protected]>
Users should be able to permanently disable colors and suggestions in their CLI outputs throught the Client configuration. Signed-off-by: Renan Rodrigo <[email protected]>
This class will be used across the codebase serving as global configuration for colors, utf-8 support and suggestions on the CLI. Signed-off-by: Renan Rodrigo <[email protected]>
renanrodrigo
force-pushed
the
new-ux-config
branch
from
07a6b84
to
bc01bd5
Compare
Why is this needed?
This PR solves all of our problems because users should be able to set color and suggestion output in the new CLI using the Client configuration. And the possible values should be true or false.
We also add a class to serve as global configuration for the CLI specific options. Besides the colors and suggestions, it checks for utf-8 support. It adds new constraints to use colors, so commands don't read directly from the UAConfig, but combine this with this class instead.
The class has methods which can be dinamically executed based on CLI arguments after parsing those - because it's information we don't have from the system, but rather from the user.
Test Steps
Unit + integration was addressed