Handle patronictl edit-config diff pager in a more user friendly way (#2605)

`patronictl edit-config` requires a pager to show the diff output back to the user. It used to be hard-coded to use either `less` or `more`.

When these tools were not available in the host that would cause `patronictl` to face an exception in `ydiff` module and to show the stack trace in the console.

This PR changes `patronictl edit-config` command to behave like this:

- If `PAGER` environment variable is set, attempt to find the corresponding executable.
- If `PAGER` is not set or is set with an invalid executable, then attempt to use either `less` or `more` as it used to do.
- If no executable is find at all then throw a `PatroniCtlException` to show an user friendly message

Unit tests in `tests/test_ctl.py` were modified accordingly.

References: PAT-21
Close #2604

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:

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'))