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.

Sign up for GitHub

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

Jump to bottom

Add confg values for the new CLI #3294

Merged
merged 3 commits into from
Sep 18, 2024
Merged

Add confg values for the new CLI #3294

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3fcd1fc
config: validate boolean values when setting them
renanrodrigo Sep 5, 2024
5e98df6
config: add configuration options for the CLI
renanrodrigo Sep 5, 2024
bc01bd5
cli: add formatter config
renanrodrigo Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions features/api/config.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ Feature: Config status api
"attributes": {
"apt_news": true,
"apt_news_url": "https://motd.ubuntu.com/aptnews.json",
"cli_color": true,
"cli_suggestions": true,
"global_apt_http_proxy": null,
"global_apt_https_proxy": null,
"http_proxy": null,
Expand All @@ -32,6 +34,8 @@ Feature: Config status api
"attributes": {
"apt_news": false,
"apt_news_url": "https://motd.ubuntu.com/aptnews.json",
"cli_color": true,
"cli_suggestions": true,
"global_apt_http_proxy": null,
"global_apt_https_proxy": null,
"http_proxy": null,
Expand Down Expand Up @@ -73,6 +77,8 @@ Feature: Config status api
"attributes": {
"apt_news": true,
"apt_news_url": "https://motd.ubuntu.com/aptnews.json",
"cli_color": true,
"cli_suggestions": true,
"global_apt_http_proxy": null,
"global_apt_https_proxy": null,
"http_proxy": "http://someuser:somepassword@$behave_var{machine-ip proxy}:3128",
Expand All @@ -95,6 +101,8 @@ Feature: Config status api
"attributes": {
"apt_news": true,
"apt_news_url": "https://motd.ubuntu.com/aptnews.json",
"cli_color": true,
"cli_suggestions": true,
"global_apt_http_proxy": null,
"global_apt_https_proxy": null,
"http_proxy": "<REDACTED>",
Expand Down
4 changes: 4 additions & 0 deletions features/cli/config.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ Feature: CLI config command
metering_timer 14400
apt_news True
apt_news_url https://motd.ubuntu.com/aptnews.json
cli_color True
cli_suggestions True
"""
Then I will see the following on stderr:
"""
Expand All @@ -42,6 +44,8 @@ Feature: CLI config command
metering_timer 14400
apt_news False
apt_news_url https://motd.ubuntu.com/aptnews.json
cli_color True
cli_suggestions True
"""
Then I will see the following on stderr:
"""
Expand Down
4 changes: 2 additions & 2 deletions features/cli/help.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ Feature: Pro Client help text
apt_https_proxy, ua_apt_http_proxy, ua_apt_https_proxy,
global_apt_http_proxy, global_apt_https_proxy,
update_messaging_timer, metering_timer, apt_news,
apt_news_url
apt_news_url, cli_color, cli_suggestions

<options_string>:
-h, --help show this help message and exit
Expand All @@ -407,7 +407,7 @@ Feature: Pro Client help text
http_proxy, https_proxy, apt_http_proxy, apt_https_proxy,
ua_apt_http_proxy, ua_apt_https_proxy, global_apt_http_proxy,
global_apt_https_proxy, update_messaging_timer, metering_timer,
apt_news, apt_news_url
apt_news, apt_news_url, cli_color, cli_suggestions

<options_string>:
-h, --help show this help message and exit
Expand Down
18 changes: 17 additions & 1 deletion uaclient/api/u/pro/config/v1.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,18 @@ class ConfigInfo(DataObject, AdditionalInfo):
required=False,
doc="Update messaging timer",
),
Field(
"cli_color",
BoolDataValue,
required=False,
doc="Show colors in the CLI",
),
Field(
"cli_suggestions",
BoolDataValue,
required=False,
doc="Show suggestions in the CLI",
),
]

def __init__(
Expand All @@ -76,7 +88,9 @@ def __init__(
update_messaging_timer: Optional[int] = None,
metering_timer: Optional[int] = None,
apt_news: Optional[bool] = None,
apt_news_url: Optional[str] = None
apt_news_url: Optional[str] = None,
cli_color: Optional[bool] = None,
cli_suggestions: Optional[bool] = None
):
self.http_proxy = http_proxy
self.https_proxy = https_proxy
Expand All @@ -88,6 +102,8 @@ def __init__(
self.metering_timer = metering_timer
self.apt_news = apt_news
self.apt_news_url = apt_news_url
self.cli_color = cli_color
self.cli_suggestions = cli_suggestions


def config() -> ConfigInfo:
Expand Down
9 changes: 8 additions & 1 deletion uaclient/cli/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,14 @@ def action_config_set(args, *, cfg, **kwargs):
if not set_value.strip():
parser.print_help_for_command("config set")
raise exceptions.EmptyConfigValue(arg=set_key)

if type(getattr(cfg, set_key, None)) == bool:
if set_value.lower() not in ("true", "false"):
raise exceptions.InvalidArgChoice(
arg="<value>", choices="true, false"
)
set_value = set_value.lower() == "true"

if set_key in ("http_proxy", "https_proxy"):
protocol_type = set_key.split("_")[0]
if protocol_type == "http":
Expand Down Expand Up @@ -177,7 +185,6 @@ def action_config_set(args, *, cfg, **kwargs):
key=set_key, value=set_value
)
elif set_key == "apt_news":
set_value = set_value.lower() == "true"
if set_value:
apt_news.update_apt_news(cfg)
else:
Expand Down
38 changes: 38 additions & 0 deletions uaclient/cli/formatter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import os
import sys

from uaclient.config import UAConfig


# Class attributes and methods so we don't need singletons or globals for this
class ProOutputFormatterConfig:
use_utf8 = True
use_color = True
show_suggestions = True

# Initializing the class after the import is useful for unit testing
@classmethod
def init(cls, cfg: UAConfig):
cls.use_utf8 = (
sys.stdout.encoding is not None
and "UTF-8" in sys.stdout.encoding.upper()
)

cls.use_color = (
sys.stdout.isatty()
and os.getenv("NO_COLOR") is None
Copy link
Contributor

Choose a reason for hiding this comment

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

what if the user sets this to NO_COLOR=0 ?

Copy link
Member Author

@renanrodrigo renanrodrigo Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

(by reasonable I mean that any of the triggers would disable the colors, regardless of order, and none would need precedence)

and cfg.cli_color
)

cls.show_suggestions = cfg.cli_suggestions

@classmethod
def disable_color(cls) -> None:
cls.use_color = False

@classmethod
def disable_suggestions(cls) -> None:
cls.show_suggestions = False


ProOutputFormatterConfig.init(cfg=UAConfig())
4 changes: 4 additions & 0 deletions uaclient/cli/tests/test_cli_config_set.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,10 @@ class TestMainConfigSet:
"https_proxy= ",
"Empty value provided for https_proxy.",
),
(
"apt_news=please",
"<value> must be one of: true, false",
),
),
)
@mock.patch("uaclient.contract.get_available_resources")
Expand Down
2 changes: 2 additions & 0 deletions uaclient/cli/tests/test_cli_config_show.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ def test_show_values_and_limit_when_optional_key_provided(
metering_timer 14400
apt_news True
apt_news_url https://motd.ubuntu.com/aptnews.json
cli_color True
cli_suggestions True
"""
== out
)
Expand Down
65 changes: 65 additions & 0 deletions uaclient/cli/tests/test_cli_formatter.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import mock
import pytest

from uaclient.cli.formatter import ProOutputFormatterConfig as POFC

M_PATH = "uaclient.cli.formatter."


class TestProFormatterConfig:
@pytest.mark.parametrize(
"system_encoding,expected_use_utf8",
((None, False), ("iso-8859-13", False), ("utf-8", True)),
)
def test_use_utf8(self, system_encoding, expected_use_utf8, FakeConfig):
cfg = FakeConfig()

with mock.patch(M_PATH + "sys.stdout") as m_stdout:
m_stdout.encoding = system_encoding
POFC.init(cfg)

assert POFC.use_utf8 is expected_use_utf8

@pytest.mark.parametrize("config_value", (True, False))
@pytest.mark.parametrize("is_tty", (True, False))
@pytest.mark.parametrize("env_no_color", ("True", None))
@mock.patch(M_PATH + "sys.stdout.isatty")
@mock.patch(M_PATH + "os.getenv")
def test_color_config(
self,
m_getenv,
m_is_tty,
config_value,
is_tty,
env_no_color,
FakeConfig,
):
cfg = FakeConfig()
cfg.user_config.cli_color = config_value

m_is_tty.return_value = is_tty

m_getenv.return_value = env_no_color

POFC.init(cfg)

expected_result = True
if any((config_value is False, not is_tty, env_no_color)):
expected_result = False

assert POFC.use_color is expected_result

POFC.disable_color()
assert POFC.use_color is False

@pytest.mark.parametrize("config_value", (True, False))
def test_suggestions_config(self, config_value, FakeConfig):
cfg = FakeConfig()
cfg.user_config.cli_suggestions = config_value

POFC.init(cfg)

assert POFC.show_suggestions is config_value

POFC.disable_suggestions()
assert POFC.show_suggestions is False
26 changes: 26 additions & 0 deletions uaclient/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@
"metering_timer",
"apt_news",
"apt_news_url",
"cli_color",
"cli_suggestions",
)

# Basic schema validation top-level keys for parse_config handling
Expand Down Expand Up @@ -287,6 +289,30 @@ def apt_news_url(self, value: str):
self.user_config.apt_news_url = value
user_config_file.user_config.write(self.user_config)

@property
def cli_color(self) -> bool:
val = self.user_config.cli_color
if val is None:
return True
return val

@cli_color.setter
def cli_color(self, value: bool):
self.user_config.cli_color = value
user_config_file.user_config.write(self.user_config)

@property
def cli_suggestions(self) -> bool:
val = self.user_config.cli_suggestions
if val is None:
return True
return val

@cli_suggestions.setter
def cli_suggestions(self, value: bool):
self.user_config.cli_suggestions = value
user_config_file.user_config.write(self.user_config)

@property
def data_dir(self):
return self.cfg.get("data_dir", DEFAULT_DATA_DIR)
Expand Down
6 changes: 6 additions & 0 deletions uaclient/files/user_config_file.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ class UserConfigData(DataObject):
Field("polling_error_retry_delay", IntDataValue, required=False),
Field("metering_timer", IntDataValue, required=False),
Field("update_messaging_timer", IntDataValue, required=False),
Field("cli_color", BoolDataValue, required=False),
Field("cli_suggestions", BoolDataValue, required=False),
]

def __init__(
Expand All @@ -62,6 +64,8 @@ def __init__(
polling_error_retry_delay: Optional[int] = None,
metering_timer: Optional[int] = None,
update_messaging_timer: Optional[int] = None,
cli_color: Optional[bool] = None,
cli_suggestions: Optional[bool] = None,
):
self.apt_http_proxy = apt_http_proxy
self.apt_https_proxy = apt_https_proxy
Expand All @@ -77,6 +81,8 @@ def __init__(
self.polling_error_retry_delay = polling_error_retry_delay
self.metering_timer = metering_timer
self.update_messaging_timer = update_messaging_timer
self.cli_color = cli_color
self.cli_suggestions = cli_suggestions


event = event_logger.get_event_logger()
Expand Down
2 changes: 2 additions & 0 deletions uaclient/tests/test_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,8 @@ def test_remove_notice_removes_matching(
"https_proxy": None,
"update_messaging_timer": 21600,
"metering_timer": 14400,
"cli_color": True,
"cli_suggestions": True,
}


Expand Down
Loading