From 3fcd1fc2ac4a5dc542e7e8aee6d4f0c8d7fd10a9 Mon Sep 17 00:00:00 2001 From: Renan Rodrigo Date: Thu, 5 Sep 2024 10:54:45 -0300 Subject: [PATCH 1/3] config: validate boolean values when setting them 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 --- uaclient/cli/config.py | 9 ++++++++- uaclient/cli/tests/test_cli_config_set.py | 4 ++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/uaclient/cli/config.py b/uaclient/cli/config.py index b8077f1720..b1313001cd 100644 --- a/uaclient/cli/config.py +++ b/uaclient/cli/config.py @@ -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="", 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": @@ -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: diff --git a/uaclient/cli/tests/test_cli_config_set.py b/uaclient/cli/tests/test_cli_config_set.py index 2026fc5e51..3f955ca998 100644 --- a/uaclient/cli/tests/test_cli_config_set.py +++ b/uaclient/cli/tests/test_cli_config_set.py @@ -49,6 +49,10 @@ class TestMainConfigSet: "https_proxy= ", "Empty value provided for https_proxy.", ), + ( + "apt_news=please", + " must be one of: true, false", + ), ), ) @mock.patch("uaclient.contract.get_available_resources") From 5e98df6b9a776c8857e878618b9a11ef3414feca Mon Sep 17 00:00:00 2001 From: Renan Rodrigo Date: Thu, 5 Sep 2024 10:57:53 -0300 Subject: [PATCH 2/3] config: add configuration options for the CLI Users should be able to permanently disable colors and suggestions in their CLI outputs throught the Client configuration. Signed-off-by: Renan Rodrigo --- features/api/config.feature | 8 +++++++ features/cli/config.feature | 4 ++++ features/cli/help.feature | 4 ++-- uaclient/api/u/pro/config/v1.py | 18 ++++++++++++++- uaclient/cli/tests/test_cli_config_show.py | 2 ++ uaclient/config.py | 26 ++++++++++++++++++++++ uaclient/files/user_config_file.py | 6 +++++ uaclient/tests/test_config.py | 2 ++ 8 files changed, 67 insertions(+), 3 deletions(-) diff --git a/features/api/config.feature b/features/api/config.feature index 0affae203c..ba0c50a567 100644 --- a/features/api/config.feature +++ b/features/api/config.feature @@ -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, @@ -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, @@ -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", @@ -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": "", diff --git a/features/cli/config.feature b/features/cli/config.feature index 371e344dcd..8518c37941 100644 --- a/features/cli/config.feature +++ b/features/cli/config.feature @@ -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: """ @@ -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: """ diff --git a/features/cli/help.feature b/features/cli/help.feature index 034bd4029e..c4e42e92dd 100644 --- a/features/cli/help.feature +++ b/features/cli/help.feature @@ -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 : -h, --help show this help message and exit @@ -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 : -h, --help show this help message and exit diff --git a/uaclient/api/u/pro/config/v1.py b/uaclient/api/u/pro/config/v1.py index 2b3b9c1dd1..38c5e25162 100644 --- a/uaclient/api/u/pro/config/v1.py +++ b/uaclient/api/u/pro/config/v1.py @@ -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__( @@ -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 @@ -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: diff --git a/uaclient/cli/tests/test_cli_config_show.py b/uaclient/cli/tests/test_cli_config_show.py index 9eb082eb22..fec66f2a98 100644 --- a/uaclient/cli/tests/test_cli_config_show.py +++ b/uaclient/cli/tests/test_cli_config_show.py @@ -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 ) diff --git a/uaclient/config.py b/uaclient/config.py index e3dec4ff57..11959f50d8 100644 --- a/uaclient/config.py +++ b/uaclient/config.py @@ -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 @@ -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) diff --git a/uaclient/files/user_config_file.py b/uaclient/files/user_config_file.py index cc6c281d4c..0812ad80e5 100644 --- a/uaclient/files/user_config_file.py +++ b/uaclient/files/user_config_file.py @@ -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__( @@ -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 @@ -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() diff --git a/uaclient/tests/test_config.py b/uaclient/tests/test_config.py index e4ebb38910..4a15cfb087 100644 --- a/uaclient/tests/test_config.py +++ b/uaclient/tests/test_config.py @@ -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, } From bc01bd53cd03da4a1cc2d714522b898da58c853a Mon Sep 17 00:00:00 2001 From: Renan Rodrigo Date: Fri, 6 Sep 2024 01:42:33 -0300 Subject: [PATCH 3/3] cli: add formatter config 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 --- uaclient/cli/formatter.py | 38 ++++++++++++++ uaclient/cli/tests/test_cli_formatter.py | 65 ++++++++++++++++++++++++ 2 files changed, 103 insertions(+) create mode 100644 uaclient/cli/formatter.py create mode 100644 uaclient/cli/tests/test_cli_formatter.py diff --git a/uaclient/cli/formatter.py b/uaclient/cli/formatter.py new file mode 100644 index 0000000000..25dd63bb74 --- /dev/null +++ b/uaclient/cli/formatter.py @@ -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 + 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()) diff --git a/uaclient/cli/tests/test_cli_formatter.py b/uaclient/cli/tests/test_cli_formatter.py new file mode 100644 index 0000000000..93c0b10a53 --- /dev/null +++ b/uaclient/cli/tests/test_cli_formatter.py @@ -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