diff --git a/patroni/ctl.py b/patroni/ctl.py index ab035f95..e64db043 100644 --- a/patroni/ctl.py +++ b/patroni/ctl.py @@ -1092,12 +1092,28 @@ def show_diff(before_editing, after_editing): width = 80 tab_width = 8 wrap = True - if shutil.which('less'): - pager = None - else: - pager = os.path.basename(shutil.which('more') or 'more') + pager = next( + ( + os.path.basename(p) + for p in (os.environ.get('PAGER'), "less", "more") + if p is not None and shutil.which(p) + ), + None, + ) pager_options = None + if opts.pager is None: + raise PatroniCtlException( + 'No pager could be found. Either set PAGER environment variable with ' + 'your pager or install either "less" or "more" in the host.' + ) + + # if we end up selecting "less" as "pager" then we set "pager" attribute + # to "None". "less" is the default pager for "ydiff" module, and that + # module adds some command-line options to "less" when "pager" is "None" + if opts.pager == 'less': + opts.pager = None + markup_to_pager(PatchStream(buf), opts) else: for line in unified_diff: diff --git a/tests/test_ctl.py b/tests/test_ctl.py index 39ee2520..56aa3af1 100644 --- a/tests/test_ctl.py +++ b/tests/test_ctl.py @@ -4,7 +4,7 @@ import unittest from click.testing import CliRunner from datetime import datetime, timedelta -from mock import patch, Mock +from mock import patch, Mock, call from patroni.ctl import ctl, load_config, output_members, get_dcs, parse_dcs, \ get_all_members, get_any_member, get_cursor, query_member, PatroniCtlException, apply_config_changes, \ format_config_for_editing, show_diff, invoke_editor, format_pg_version, CONFIG_FILE_PATH, PatronictlPrettyTable @@ -558,19 +558,54 @@ class TestCtl(unittest.TestCase): @patch('sys.stdout.isatty', return_value=False) @patch('patroni.ctl.markup_to_pager') + @patch('os.environ.get', return_value=None) @patch('shutil.which', return_value=None) - def test_show_diff(self, mock_which, mock_markup_to_pager, mock_isatty): + def test_show_diff(self, mock_which, mock_env_get, mock_markup_to_pager, mock_isatty): + # no TTY show_diff("foo:\n bar: 1\n", "foo:\n bar: 2\n") mock_markup_to_pager.assert_not_called() + # TTY but no PAGER nor executable mock_isatty.return_value = True + with self.assertRaises(PatroniCtlException) as e: + show_diff("foo:\n bar: 1\n", "foo:\n bar: 2\n") + self.assertEqual( + str(e.exception), + 'No pager could be found. Either set PAGER environment variable with ' + 'your pager or install either "less" or "more" in the host.' + ) + mock_env_get.assert_called_once_with('PAGER') + mock_which.assert_has_calls([ + call('less'), + call('more'), + ]) + mock_markup_to_pager.assert_not_called() + + # TTY with PAGER set but invalid + mock_env_get.reset_mock() + mock_env_get.return_value = 'random' + mock_which.reset_mock() + with self.assertRaises(PatroniCtlException) as e: + show_diff("foo:\n bar: 1\n", "foo:\n bar: 2\n") + self.assertEqual( + str(e.exception), + 'No pager could be found. Either set PAGER environment variable with ' + 'your pager or install either "less" or "more" in the host.' + ) + mock_env_get.assert_called_once_with('PAGER') + mock_which.assert_has_calls([ + call('random'), + call('less'), + call('more'), + ]) + mock_markup_to_pager.assert_not_called() + + # TTY with valid executable + mock_which.return_value = '/usr/bin/less' show_diff("foo:\n bar: 1\n", "foo:\n bar: 2\n") mock_markup_to_pager.assert_called_once() - show_diff("foo:\n bar: 1\n", "foo:\n bar: 2\n") - # Test that unicode handling doesn't fail with an exception - mock_which.return_value = '/usr/bin/less' show_diff(b"foo:\n bar: \xc3\xb6\xc3\xb6\n".decode('utf-8'), b"foo:\n bar: \xc3\xbc\xc3\xbc\n".decode('utf-8'))