From 16263d0157cc2a507f7df93bfa7827be4c70c979 Mon Sep 17 00:00:00 2001 From: a-dubs Date: Mon, 27 Nov 2023 09:50:23 -0500 Subject: [PATCH 1/4] cli: improved cli/log message for unexpected errors Message for Unexpected errors is now a formatted string that takes in an error message so that the original error message is no longer hidden from the end user. Fixes: #2600 --- uaclient/cli/__init__.py | 3 ++- uaclient/cli/tests/test_cli.py | 4 +++- uaclient/cli/tests/test_cli_attach.py | 2 +- uaclient/contract.py | 12 ++++++++---- uaclient/messages/__init__.py | 9 +++++---- 5 files changed, 19 insertions(+), 11 deletions(-) diff --git a/uaclient/cli/__init__.py b/uaclient/cli/__init__.py index 976f5a5cd1..597da2eec4 100644 --- a/uaclient/cli/__init__.py +++ b/uaclient/cli/__init__.py @@ -1766,7 +1766,8 @@ def wrapper(*args, **kwargs): LOG.exception("Unhandled exception, please file a bug") lock.clear_lock_file_if_present() event.info( - info_msg=messages.UNEXPECTED_ERROR.msg, file_type=sys.stderr + info_msg=messages.UNEXPECTED_ERROR.format(error_msg=str(e)), + file_type=sys.stderr, ) event.error( error_msg=getattr(e, "msg", str(e)), error_type="exception" diff --git a/uaclient/cli/tests/test_cli.py b/uaclient/cli/tests/test_cli.py index af311678c7..7f65aebe5f 100644 --- a/uaclient/cli/tests/test_cli.py +++ b/uaclient/cli/tests/test_cli.py @@ -491,7 +491,9 @@ class TestMain: ( ( TypeError("'NoneType' object is not subscriptable"), - messages.UNEXPECTED_ERROR.msg, + messages.UNEXPECTED_ERROR.format( + error_msg="'NoneType' object is not subscriptable" + ), "Unhandled exception, please file a bug", ), ), diff --git a/uaclient/cli/tests/test_cli_attach.py b/uaclient/cli/tests/test_cli_attach.py index f083cf3435..8e13bb8d8f 100644 --- a/uaclient/cli/tests/test_cli_attach.py +++ b/uaclient/cli/tests/test_cli_attach.py @@ -515,7 +515,7 @@ def test_attach_config_enable_services( ), ( Exception("error"), - messages.UNEXPECTED_ERROR, + messages.UNEXPECTED_ERROR.format(error_msg="error"), messages.E_ATTACH_FAILURE_UNEXPECTED, ), ), diff --git a/uaclient/contract.py b/uaclient/contract.py index 334387ab48..4506ebb944 100644 --- a/uaclient/contract.py +++ b/uaclient/contract.py @@ -492,7 +492,7 @@ def process_entitlements_delta( from uaclient.entitlements import entitlements_enable_order delta_error = False - unexpected_error = False + unexpected_errors = [] # We need to sort our entitlements because some of them # depend on other service to be enable first. @@ -523,7 +523,7 @@ def process_entitlements_delta( ) except Exception as e: LOG.exception(e) - unexpected_error = True + unexpected_errors.append(e) failed_services.append(name) LOG.exception( "Unexpected error processing contract delta for %s: %r", @@ -536,10 +536,14 @@ def process_entitlements_delta( if service_enabled and deltas: event.service_processed(name) event.services_failed(failed_services) - if unexpected_error: + if len(unexpected_errors) > 0: raise exceptions.AttachFailureUnknownError( failed_services=[ - (name, messages.UNEXPECTED_ERROR) for name in failed_services + ( + name, + messages.UNEXPECTED_ERROR.format(error_msg=str(exception)), + ) + for name, exception in zip(failed_services, unexpected_errors) ] ) elif delta_error: diff --git a/uaclient/messages/__init__.py b/uaclient/messages/__init__.py index cb2768a0e6..5a867891ec 100644 --- a/uaclient/messages/__init__.py +++ b/uaclient/messages/__init__.py @@ -1553,13 +1553,14 @@ def __repr__(self): the --access-only flag.""", ) -UNEXPECTED_ERROR = NamedMessage( +UNEXPECTED_ERROR = FormattedNamedMessage( "unexpected-error", t.gettext( """\ -Unexpected error(s) occurred. -For more details, see the log: /var/log/ubuntu-advantage.log -To file a bug run: ubuntu-bug ubuntu-advantage-tools""" +Unexpected error(s) occurred: {error_msg} +For more details, see the log: ~/.cache/ubuntu-pro/ubuntu-pro.log +First, try searching online for solutions to this error. +Otherwise, file a bug using the command: ubuntu-bug ubuntu-advantage-tools""" ), ) From d346ff270cc3e9b55e6defc26774e6107cfcc364 Mon Sep 17 00:00:00 2001 From: a-dubs Date: Wed, 6 Dec 2023 15:03:46 -0500 Subject: [PATCH 2/4] feat: unxpected error messages point to correct log path if not root added util function to get proper log path based on if currently root or not --- uaclient/cli/__init__.py | 7 +++-- uaclient/cli/tests/test_cli.py | 5 ++-- uaclient/cli/tests/test_cli_attach.py | 6 +++- uaclient/contract.py | 6 +++- uaclient/log.py | 12 ++++++++ uaclient/messages/__init__.py | 7 ++--- uaclient/tests/test_log.py | 40 +++++++++++++++++++++++++++ 7 files changed, 73 insertions(+), 10 deletions(-) diff --git a/uaclient/cli/__init__.py b/uaclient/cli/__init__.py index 597da2eec4..7995928425 100644 --- a/uaclient/cli/__init__.py +++ b/uaclient/cli/__init__.py @@ -66,7 +66,7 @@ ) from uaclient.files import notices, state_files from uaclient.files.notices import Notice -from uaclient.log import JsonArrayFormatter +from uaclient.log import JsonArrayFormatter, get_user_or_root_log_file_path from uaclient.timer.update_messaging import refresh_motd, update_motd_messages from uaclient.yaml import safe_dump, safe_load @@ -1766,7 +1766,10 @@ def wrapper(*args, **kwargs): LOG.exception("Unhandled exception, please file a bug") lock.clear_lock_file_if_present() event.info( - info_msg=messages.UNEXPECTED_ERROR.format(error_msg=str(e)), + info_msg=messages.UNEXPECTED_ERROR.format( + error_msg=str(e), + log_path=get_user_or_root_log_file_path(), + ), file_type=sys.stderr, ) event.error( diff --git a/uaclient/cli/tests/test_cli.py b/uaclient/cli/tests/test_cli.py index 7f65aebe5f..fef3a9be00 100644 --- a/uaclient/cli/tests/test_cli.py +++ b/uaclient/cli/tests/test_cli.py @@ -492,7 +492,8 @@ class TestMain: ( TypeError("'NoneType' object is not subscriptable"), messages.UNEXPECTED_ERROR.format( - error_msg="'NoneType' object is not subscriptable" + error_msg="'NoneType' object is not subscriptable", + log_path="/var/log/ubuntu-advantage.log", ), "Unhandled exception, please file a bug", ), @@ -510,6 +511,7 @@ def test_errors_handled_gracefully( m_log_exception, m_event_info, m_delete_cache_key, + # _m_get_user_or_root_log_path, event, exception, expected_error_msg, @@ -530,7 +532,6 @@ def test_errors_handled_gracefully( exc = excinfo.value assert 1 == exc.code - assert [ mock.call(info_msg=expected_error_msg, file_type=mock.ANY) ] == m_event_info.call_args_list diff --git a/uaclient/cli/tests/test_cli_attach.py b/uaclient/cli/tests/test_cli_attach.py index 8e13bb8d8f..c994fc5174 100644 --- a/uaclient/cli/tests/test_cli_attach.py +++ b/uaclient/cli/tests/test_cli_attach.py @@ -515,7 +515,10 @@ def test_attach_config_enable_services( ), ( Exception("error"), - messages.UNEXPECTED_ERROR.format(error_msg="error"), + messages.UNEXPECTED_ERROR.format( + error_msg="error", + log_path="/var/log/ubuntu-advantage.log", + ), messages.E_ATTACH_FAILURE_UNEXPECTED, ), ), @@ -534,6 +537,7 @@ def test_attach_when_one_service_fails_to_enable( m_process_entitlement_delta, m_enable_order, _m_attachment_data_file_write, + # _m_get_user_or_root_log_path, expected_exception, expected_msg, expected_outer_msg, diff --git a/uaclient/contract.py b/uaclient/contract.py index 4506ebb944..00a8bb570e 100644 --- a/uaclient/contract.py +++ b/uaclient/contract.py @@ -24,6 +24,7 @@ ) from uaclient.files.state_files import attachment_data_file from uaclient.http import serviceclient +from uaclient.log import get_user_or_root_log_file_path # Here we describe every endpoint from the ua-contracts # service that is used by this client implementation. @@ -541,7 +542,10 @@ def process_entitlements_delta( failed_services=[ ( name, - messages.UNEXPECTED_ERROR.format(error_msg=str(exception)), + messages.UNEXPECTED_ERROR.format( + error_msg=str(exception), + log_path=get_user_or_root_log_file_path(), + ), ) for name, exception in zip(failed_services, unexpected_errors) ] diff --git a/uaclient/log.py b/uaclient/log.py index 05e658af2b..a6810400bc 100644 --- a/uaclient/log.py +++ b/uaclient/log.py @@ -5,6 +5,7 @@ from typing import Any, Dict, List # noqa: F401 from uaclient import defaults, system, util +from uaclient.config import UAConfig class RedactionFilter(logging.Filter): @@ -61,6 +62,17 @@ def format(self, record: logging.LogRecord) -> str: return json.dumps(list(local_log_record.values())) +def get_user_or_root_log_file_path() -> str: + """ + Gets the correct log_file path, + adjusting for whether the user is root or not. + """ + if util.we_are_currently_root(): + return UAConfig().log_file + else: + return "~/.cache/" + defaults.USER_CACHE_SUBDIR + "/ubuntu-pro.log" + + def get_user_log_file() -> str: """Gets the correct user log_file storage location""" return system.get_user_cache_dir() + "/ubuntu-pro.log" diff --git a/uaclient/messages/__init__.py b/uaclient/messages/__init__.py index 5a867891ec..c58a31474d 100644 --- a/uaclient/messages/__init__.py +++ b/uaclient/messages/__init__.py @@ -1557,10 +1557,9 @@ def __repr__(self): "unexpected-error", t.gettext( """\ -Unexpected error(s) occurred: {error_msg} -For more details, see the log: ~/.cache/ubuntu-pro/ubuntu-pro.log -First, try searching online for solutions to this error. -Otherwise, file a bug using the command: ubuntu-bug ubuntu-advantage-tools""" +An unexpected error occurred: {error_msg} +For more details, see the log: {log_path} +If you think this is a bug, please run: ubuntu-bug ubuntu-advantage-tools""" ), ) diff --git a/uaclient/tests/test_log.py b/uaclient/tests/test_log.py index fbd6c481a8..f92a0fe092 100644 --- a/uaclient/tests/test_log.py +++ b/uaclient/tests/test_log.py @@ -2,6 +2,7 @@ import logging from io import StringIO +import mock import pytest from uaclient import log as pro_log @@ -158,3 +159,42 @@ def test_valid_json_output( assert val[6].get("key") == extra.get("key") else: assert 7 == len(val) + + +class TestLogHelpers: + def test_get_user_or_root_log_file_path(self): + """ + Tests that the correct default log_file storage location is retrieved + when the user is root. + """ + # test root log file path + with mock.patch( + "uaclient.util.we_are_currently_root", + return_value=True, + ): + assert ( + pro_log.get_user_or_root_log_file_path() + == "/var/log/ubuntu-advantage.log" + ) + # test default user log file path + with mock.patch( + "uaclient.util.we_are_currently_root", + return_value=False, + ): + assert ( + pro_log.get_user_or_root_log_file_path() + == "~/.cache/ubuntu-pro/ubuntu-pro.log" + ) + # test custom user log file path + with mock.patch( + "uaclient.config.UAConfig.log_file", new_callable=mock.PropertyMock + ) as mock_log_file: + expected_log_file = "/tmp/foo.log" + mock_log_file.return_value = expected_log_file + with mock.patch( + "uaclient.util.we_are_currently_root", return_value=True + ) as mock_root: + result = pro_log.get_user_or_root_log_file_path() + mock_root.assert_called() + mock_log_file.assert_called() + assert expected_log_file == result From 1a5230a476c42db74b404ce1714efb8890378f33 Mon Sep 17 00:00:00 2001 From: a-dubs Date: Fri, 12 Jan 2024 12:37:10 -0500 Subject: [PATCH 3/4] added orndorffgrant's suggestions from PR --- uaclient/cli/tests/test_cli.py | 1 - uaclient/cli/tests/test_cli_attach.py | 1 - uaclient/log.py | 2 +- uaclient/tests/test_log.py | 75 +++++++++++++++------------ 4 files changed, 42 insertions(+), 37 deletions(-) diff --git a/uaclient/cli/tests/test_cli.py b/uaclient/cli/tests/test_cli.py index fef3a9be00..6a237b9eb5 100644 --- a/uaclient/cli/tests/test_cli.py +++ b/uaclient/cli/tests/test_cli.py @@ -511,7 +511,6 @@ def test_errors_handled_gracefully( m_log_exception, m_event_info, m_delete_cache_key, - # _m_get_user_or_root_log_path, event, exception, expected_error_msg, diff --git a/uaclient/cli/tests/test_cli_attach.py b/uaclient/cli/tests/test_cli_attach.py index c994fc5174..8a55b7fdd9 100644 --- a/uaclient/cli/tests/test_cli_attach.py +++ b/uaclient/cli/tests/test_cli_attach.py @@ -537,7 +537,6 @@ def test_attach_when_one_service_fails_to_enable( m_process_entitlement_delta, m_enable_order, _m_attachment_data_file_write, - # _m_get_user_or_root_log_path, expected_exception, expected_msg, expected_outer_msg, diff --git a/uaclient/log.py b/uaclient/log.py index a6810400bc..2814a5263b 100644 --- a/uaclient/log.py +++ b/uaclient/log.py @@ -70,7 +70,7 @@ def get_user_or_root_log_file_path() -> str: if util.we_are_currently_root(): return UAConfig().log_file else: - return "~/.cache/" + defaults.USER_CACHE_SUBDIR + "/ubuntu-pro.log" + return get_user_log_file() def get_user_log_file() -> str: diff --git a/uaclient/tests/test_log.py b/uaclient/tests/test_log.py index f92a0fe092..dc7d0f3829 100644 --- a/uaclient/tests/test_log.py +++ b/uaclient/tests/test_log.py @@ -162,39 +162,46 @@ def test_valid_json_output( class TestLogHelpers: - def test_get_user_or_root_log_file_path(self): + @pytest.mark.parametrize( + [ + "we_are_currently_root", + "expected", + ], + [ + (True, "cfg_log_file"), + (False, "user_log_file"), + ], + ) + @mock.patch( + "uaclient.log.get_user_log_file", + return_value="user_log_file", + ) + @mock.patch( + "uaclient.config.UAConfig.log_file", + new_callable=mock.PropertyMock, + return_value="cfg_log_file", + ) + @mock.patch("uaclient.util.we_are_currently_root") + def test_get_user_or_root_log_file_path( + self, + m_we_are_currently_root, + m_cfg_log_file, + m_get_user_log_file, + we_are_currently_root, + expected, + ): """ - Tests that the correct default log_file storage location is retrieved - when the user is root. + Tests that the correct log_file path is retrieved + when the user is root and non-root """ - # test root log file path - with mock.patch( - "uaclient.util.we_are_currently_root", - return_value=True, - ): - assert ( - pro_log.get_user_or_root_log_file_path() - == "/var/log/ubuntu-advantage.log" - ) - # test default user log file path - with mock.patch( - "uaclient.util.we_are_currently_root", - return_value=False, - ): - assert ( - pro_log.get_user_or_root_log_file_path() - == "~/.cache/ubuntu-pro/ubuntu-pro.log" - ) - # test custom user log file path - with mock.patch( - "uaclient.config.UAConfig.log_file", new_callable=mock.PropertyMock - ) as mock_log_file: - expected_log_file = "/tmp/foo.log" - mock_log_file.return_value = expected_log_file - with mock.patch( - "uaclient.util.we_are_currently_root", return_value=True - ) as mock_root: - result = pro_log.get_user_or_root_log_file_path() - mock_root.assert_called() - mock_log_file.assert_called() - assert expected_log_file == result + m_we_are_currently_root.return_value = we_are_currently_root + result = pro_log.get_user_or_root_log_file_path() + # ensure mocks are used properly + assert m_we_are_currently_root.call_count == 1 + assert m_cfg_log_file.call_count + m_get_user_log_file.call_count == 1 + if we_are_currently_root: + assert m_cfg_log_file.call_count == 1 + else: + assert m_get_user_log_file.call_count == 1 + # ensure correct log_file path is returned + assert expected == result From bb3535f94f9e537bb3d47ff8c58604b44e121a6c Mon Sep 17 00:00:00 2001 From: a-dubs Date: Tue, 16 Jan 2024 08:57:01 -0500 Subject: [PATCH 4/4] po: ran update-pos script yet again --- debian/po/pt_BR.po | 9 +++++---- debian/po/ubuntu-pro.pot | 9 +++++---- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/debian/po/pt_BR.po b/debian/po/pt_BR.po index 5479d0cdc1..91e3e2e256 100644 --- a/debian/po/pt_BR.po +++ b/debian/po/pt_BR.po @@ -7,7 +7,7 @@ msgid "" msgstr "" "Project-Id-Version: PACKAGE VERSION\n" "Report-Msgid-Bugs-To: \n" -"POT-Creation-Date: 2024-01-05 17:38-0300\n" +"POT-Creation-Date: 2024-01-16 08:56-0500\n" "PO-Revision-Date: 2023-09-25 12:29-0400\n" "Last-Translator: Lucas Moura \n" "Language-Team: Brazilian Portuguese \n" "Language-Team: LANGUAGE \n" @@ -2258,10 +2258,11 @@ msgid "" msgstr "" #: ../../uaclient/messages/__init__.py:1559 +#, python-brace-format msgid "" -"Unexpected error(s) occurred.\n" -"For more details, see the log: /var/log/ubuntu-advantage.log\n" -"To file a bug run: ubuntu-bug ubuntu-advantage-tools" +"An unexpected error occurred: {error_msg}\n" +"For more details, see the log: {log_path}\n" +"If you think this is a bug, please run: ubuntu-bug ubuntu-advantage-tools" msgstr "" #: ../../uaclient/messages/__init__.py:1569