From 424daee946f9189d7ecdd2e5e21202c2ec02f4f8 Mon Sep 17 00:00:00 2001

From: Rob Crittenden <rcritten@redhat.com>

Date: Wed, 6 Apr 2022 13:19:36 -0400

Subject: [PATCH] Unify command-line options and configuration

This makes it possible to add command-line options to the

configuration file.

The config file is loaded then the command-line options are

merged in. The first one option set takes precedence. So if

an option, say output_type, is in the configuration file then

passing output-type on the command-line will not override it.

The workaround is to pass --config= to ipa-healthcheck in order

to not load the configuration file.

This will allow for greater flexibility when running this automatically

without having to manually change test timer scripting directly.

https://bugzilla.redhat.com/show_bug.cgi?id=1872467

Signed-off-by: Rob Crittenden <rcritten@redhat.com>

---

 man/man5/ipahealthcheck.conf.5    | 10 ++++++-

 man/man8/ipa-healthcheck.8        |  3 +++

 src/ipahealthcheck/core/config.py |  2 +-

 src/ipahealthcheck/core/core.py   | 22 +++++++++++----

 src/ipahealthcheck/core/output.py |  4 +--

 tests/test_init.py                |  6 ++---

 tests/test_options.py             | 45 +++++++++++++++++++++++++++++++

 7 files changed, 80 insertions(+), 12 deletions(-)

 create mode 100644 tests/test_options.py

diff --git a/man/man5/ipahealthcheck.conf.5 b/man/man5/ipahealthcheck.conf.5

index 50d5506..8ff83c9 100644

--- a/man/man5/ipahealthcheck.conf.5

+++ b/man/man5/ipahealthcheck.conf.5

@@ -36,6 +36,14 @@ The following options are relevant for the server:

 .TP

 .B cert_expiration_days\fR

 The number of days left before a certificate expires to start displaying a warning. The default is 28.

+.TP

+All command\-line options may be included in the configuration file. Dashes must be converted to underscore for the configuration file, e.g. \-\-output\-type becomes output_type. All options, including those that don't make sense in a config file, like \-\-list\-sources, are allowed. Let the buyer beware.

+.TP

+The purpose of allowing command\-line options to be in the configuration file is for automation without having to tweak the automation script. For example, if you want the default output type to be human for the systemd timer automated runs, settting output_type=human in the configuration file will do this. When loading configuration the first option wins, so if any option is in the configuration file then it cannot be overridden by the command-line unless a different configuration file is specified (see \-\-config).

+.TP

+There may be conflicting exceptions. For example, if all=True is set in the configuration file, and the command\-line contains \-\-failures\-only, then only failures will be displayed because of the way the option evaluation is done.

+.TP

+Options that don't make sense for the configuration file include \-\-list\-sources and \-\-input\-file.

 .SH "FILES"

 .TP

 .I /etc/ipahealthcheck/ipahealthcheck.conf

@@ -49,4 +57,4 @@ configuration file

  cert_expiration_days=7

 .SH "SEE ALSO"

-.BR ipa-healthcheck (8)

+.BR ipa\-healthcheck (8)

diff --git a/man/man8/ipa-healthcheck.8 b/man/man8/ipa-healthcheck.8

index 9c97cfe..148a08b 100644

--- a/man/man8/ipa-healthcheck.8

+++ b/man/man8/ipa-healthcheck.8

@@ -30,6 +30,9 @@ Display a list of the available sources and the checks associated with those sou

 .SS "OPTIONAL ARGUMENTS"

 .TP

+\fB\-\-config\fR=\fIFILE\fR

+The configuration file to use. If an empty string is passed in then no configuration file is loaded. The default is /etc/ipahealthcheck/ipahealthcheck.conf.

+.TP

 \fB\-\-source\fR=\fISOURCE\fR

 Execute checks within the named source, or all sources in the given namespace.

 .TP

diff --git a/src/ipahealthcheck/core/config.py b/src/ipahealthcheck/core/config.py

index bf9ca1f..c4322a5 100644

--- a/src/ipahealthcheck/core/config.py

+++ b/src/ipahealthcheck/core/config.py

@@ -63,7 +63,7 @@ class Config:

         """

         Merge variables from dict ``d`` into the configuration

-        The last one wins.

+        The first one wins.

         :param d: dict containing configuration

         """

diff --git a/src/ipahealthcheck/core/core.py b/src/ipahealthcheck/core/core.py

index 6fd3640..aaadd3b 100644

--- a/src/ipahealthcheck/core/core.py

+++ b/src/ipahealthcheck/core/core.py

@@ -163,6 +163,8 @@ def list_sources(plugins):

 def add_default_options(parser, output_registry, default_output):

     output_names = [plugin.__name__.lower() for

                     plugin in output_registry.plugins]

+    parser.add_argument('--config', dest='config',

+                        default=None, help='Config file to load')

     parser.add_argument('--verbose', dest='verbose', action='store_true',

                         default=False, help='Run in verbose mode')

     parser.add_argument('--debug', dest='debug', action='store_true',

@@ -176,9 +178,10 @@ def add_default_options(parser, output_registry, default_output):

     parser.add_argument('--check', dest='check',

                         default=None,

                         help='Check to execute, e.g. BazCheck')

-    parser.add_argument('--output-type', dest='output', choices=output_names,

+    parser.add_argument('--output-type', dest='output_type',

+                        choices=output_names,

                         default=default_output, help='Output method')

-    parser.add_argument('--output-file', dest='outfile', default=None,

+    parser.add_argument('--output-file', dest='output_file', default=None,

                         help='File to store output')

     parser.add_argument('--version', dest='version', action='store_true',

                         help='Report the version number and exit')

@@ -266,7 +269,6 @@ class RunChecks:

         add_output_options(self.parser, self.output_registry)

         self.add_options()

         options = parse_options(self.parser)

-        self.options = options

         if options.version:

             for registry in self.entry_points:

@@ -290,10 +292,20 @@ class RunChecks:

         if options.debug:

             logger.setLevel(logging.DEBUG)

-        config = read_config(self.configfile)

+        if options.config is not None:

+            config = read_config(options.config)

+        else:

+            config = read_config(self.configfile)

         if config is None:

             return 1

+        # Unify config and options. One of these variables will be

+        # eventually deprecated in the future. This way all cli

+        # options can be set in config instead.

+        config.merge(vars(options))

+        self.options = config

+        options = config

+

         # pylint: disable=assignment-from-none

         rval = self.pre_check()

         # pylint: enable=assignment-from-none

@@ -327,7 +339,7 @@ class RunChecks:

                 plugins.append(plugin)

         for out in self.output_registry.plugins:

-            if out.__name__.lower() == options.output:

+            if out.__name__.lower() == options.output_type:

                 output = out(options)

                 break

diff --git a/src/ipahealthcheck/core/output.py b/src/ipahealthcheck/core/output.py

index 787ac6d..e44990e 100644

--- a/src/ipahealthcheck/core/output.py

+++ b/src/ipahealthcheck/core/output.py

@@ -36,7 +36,7 @@ class Output:

        which will render the results into a string for writing.

     """

     def __init__(self, options):

-        self.filename = options.outfile

+        self.filename = options.output_file

         # Non-required options in the framework, set logical defaults to

         # pre 0.6 behavior with everything reported.

@@ -110,7 +110,7 @@ class JSON(Output):

     def __init__(self, options):

         super().__init__(options)

-        self.indent = options.indent

+        self.indent = int(options.indent)

     def generate(self, data):

         output = json.dumps(data, indent=self.indent)

diff --git a/tests/test_init.py b/tests/test_init.py

index e18e03c..5f9e3e2 100644

--- a/tests/test_init.py

+++ b/tests/test_init.py

@@ -10,12 +10,12 @@ from ipahealthcheck.core.output import output_registry

 class RunChecks:

     def run_healthcheck(self):

         options = argparse.Namespace(check=None, debug=False, indent=2,

-                                     list_sources=False, outfile=None,

-                                     output='json', source=None,

+                                     list_sources=False, output_file=None,

+                                     output_type='json', source=None,

                                      verbose=False)

         for out in output_registry.plugins:

-            if out.__name__.lower() == options.output:

+            if out.__name__.lower() == options.output_type:

                 out(options)

                 break

diff --git a/tests/test_options.py b/tests/test_options.py

new file mode 100644

index 0000000..da1866f

--- /dev/null

+++ b/tests/test_options.py

@@ -0,0 +1,45 @@

+#

+# Copyright (C) 2022 FreeIPA Contributors see COPYING for license

+#

+

+import argparse

+import os

+import tempfile

+from unittest.mock import patch

+

+from ipahealthcheck.core.core import RunChecks

+from ipahealthcheck.core.plugin import Results

+

+options = argparse.Namespace(check=None, source=None, debug=False,

+                             indent=2, list_sources=False,

+                             output_type='json', output_file=None,

+                             verbose=False, version=False, config=None)

+

+

+@patch('ipahealthcheck.core.core.run_service_plugins')

+@patch('ipahealthcheck.core.core.run_plugins')

+@patch('ipahealthcheck.core.core.parse_options')

+def test_options_merge(mock_parse, mock_run, mock_service):

+    """

+    Test merging file-based and CLI options

+    """

+    mock_service.return_value = (Results(), [])

+    mock_run.return_value = Results()

+    mock_parse.return_value = options

+    fd, config_path = tempfile.mkstemp()

+    os.close(fd)

+    with open(config_path, "w") as fd:

+        fd.write('[default]\n')

+        fd.write('output_type=human\n')

+        fd.write('indent=5\n')

+

+    try:

+        run = RunChecks(['ipahealthcheck.registry'], config_path)

+

+        run.run_healthcheck()

+

+        # verify two valus that have defaults with our overriden values

+        assert run.options.output_type == 'human'

+        assert run.options.indent == '5'

+    finally:

+        os.remove(config_path)

-- 

2.31.1