[RHELC-970] Update CLI to introduce new subcommands for pre conversio…

…n analysis (#820)

* [RHELC-970] Update CLI to introduce new subcommands for pre conversion analysis

* Fix unit tests for pre_assessment_set

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>

* Fix --version when it is the only option on the command line

* Update man/init with custom SYNOPSIS

* Apply suggestions from code review

Co-authored-by: Rodolfo Olivieri <rodolfo.olivieri3@gmail.com>

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* Integration tests

* modify the pre-conversion report tests and one of the sanity to utilize the analyze subcommand
* add a scenario where convert subcommand is used
* add a reliability hardening fixture backing up and restoring
  os-release and system-release files

Signed-off-by: Daniel Diblik <ddiblik@redhat.com>

---------

Signed-off-by: Rodolfo Olivieri <rolivier@redhat.com>
Signed-off-by: Daniel Diblik <ddiblik@redhat.com>
Co-authored-by: Rodolfo Olivieri <rolivier@redhat.com>
Co-authored-by: Rodolfo Olivieri <rodolfo.olivieri3@gmail.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Co-authored-by: Daniel Diblik <ddiblik@redhat.com>

convert2rhel/systeminfo.py

@@ -25,7 +25,7 @@
 from six.moves import configparser, urllib
 
 from convert2rhel import logger, utils
-from convert2rhel.toolopts import tool_opts
+from convert2rhel.toolopts import POST_RPM_VA_LOG_FILENAME, PRE_RPM_VA_LOG_FILENAME, tool_opts
 from convert2rhel.utils import run_subprocess
 
 
@@ -48,12 +48,6 @@
     "7.9": "7Server",
 }
 
-# For a list of modified rpm files before the conversion starts
-PRE_RPM_VA_LOG_FILENAME = "rpm_va.log"
-
-# For a list of modified rpm files after the conversion finishes for comparison purposes
-POST_RPM_VA_LOG_FILENAME = "rpm_va_after_conversion.log"
-
 # List of EUS minor versions supported
 EUS_MINOR_VERSIONS = ["8.6"]
 

convert2rhel/toolopts.py

@@ -41,6 +41,28 @@
     "analyse": "analysis",
 }
 
+ARGS_WITH_VALUES = [
+    "-u",
+    "--username",
+    "-p",
+    "--password",
+    "-f",
+    "--password-from-file",
+    "-k",
+    "--activationkey",
+    "-o",
+    "--org",
+    "--pool",
+    "--serverurl",
+]
+PARENT_ARGS = ["--debug", "--help", "-h", "--version"]
+
+# For a list of modified rpm files before the conversion starts
+PRE_RPM_VA_LOG_FILENAME = "rpm_va.log"
+
+# For a list of modified rpm files after the conversion finishes for comparison purposes
+POST_RPM_VA_LOG_FILENAME = "rpm_va_after_conversion.log"
+
 
 class ToolOpts(object):
     def __init__(self):
@@ -82,13 +104,21 @@ def set_opts(self, supported_opts):
 class CLI(object):
     def __init__(self):
         self._parser = self._get_argparser()
+        self._shared_options_parser = argparse.ArgumentParser(add_help=False)
+        # Duplicating parent options here as we want to make it
+        # available for any other basic operation that we run without a
+        # subcommand in mind, and, it is a shared option so we can share it
+        # between any subcommands we may create in the future.
+        self._register_parent_options(self._parser)
+        self._register_parent_options(self._shared_options_parser)
         self._register_options()
         self._process_cli_options()
 
-    @staticmethod
-    def _get_argparser():
+    @property
+    def usage(self):
         usage = (
             "\n"
+            "  convert2rhel\n"
             "  convert2rhel [-h]\n"
             "  convert2rhel [--version]\n"
             "  convert2rhel [-u username] [-p password | -c conf_file_path] [--pool pool_id | -a] [--disablerepo repoid]"
@@ -97,40 +127,58 @@ def _get_argparser():
             "  convert2rhel [--no-rhsm] [--disablerepo repoid]"
             " [--enablerepo repoid] [--no-rpm-va] [--debug] [--restart] [-y]\n"
             "  convert2rhel [-k activation_key | -c conf_file_path] [-o organization] [--pool pool_id | -a] [--disablerepo repoid] [--enablerepo"
-            " repoid] [--serverurl url] [--keep-rhsm] [--no-rpm-va] [--debug] [--restart] [-y]"
+            " repoid] [--serverurl url] [--keep-rhsm] [--no-rpm-va] [--debug] [--restart] [-y]\n"
+            r" convert2rhel {analyze}"
             "\n\n"
-            "*WARNING* The tool needs to be run under the root user"
+            "*WARNING* The tool needs to be run under the root user\n"
         )
-        return argparse.ArgumentParser(
-            prog="convert2rhel",
-            conflict_handler="resolve",
-            usage=usage,
-            add_help=False,
+        return usage
+
+    def _get_argparser(self):
+        return argparse.ArgumentParser(conflict_handler="resolve", usage=self.usage)
+
+    def _register_commands(self):
+        """Configures parsers specific to the analyze and convert subcommands"""
+        subparsers = self._parser.add_subparsers(title="Subcommands", dest="command")
+        self._analyze_parser = subparsers.add_parser(
+            "analyze",
+            help="Run all Convert2RHEL initial checks up until the "
+            " Point of no Return (PONR) and generate a report with the findings."
+            " A rollback is initiated after the checks to put the system back"
+            " in the original state.",
+            parents=[self._shared_options_parser],
+            usage=self.usage,
         )
-
-    def _register_options(self):
-        """Prescribe what command line options the tool accepts."""
-        self._parser.add_argument(
-            "-h",
-            "--help",
-            action="help",
-            help="Show help message and exit.",
+        self._convert_parser = subparsers.add_parser(
+            "convert",
+            help="Convert the system.",
+            parents=[self._shared_options_parser],
+            usage=self.usage,
         )
-        self._parser.add_argument(
+
+    def _register_parent_options(self, parser):
+        """Prescribe what parent command line options the tool accepts."""
+        parser.add_argument(
             "--version",
             action="version",
             version=__version__,
             help="Show convert2rhel version and exit.",
         )
-        self._parser.add_argument(
+        parser.add_argument(
             "--debug",
             action="store_true",
             help="Print traceback in case of an abnormal exit and messages that could help find an issue.",
         )
-        # Importing here instead of on top of the file to avoid cyclic dependency
-        from convert2rhel.systeminfo import POST_RPM_VA_LOG_FILENAME, PRE_RPM_VA_LOG_FILENAME
 
+    def _register_options(self):
+        """Prescribe what command line options the tool accepts."""
         self._parser.add_argument(
+            "-h",
+            "--help",
+            action="help",
+            help="Show help message and exit.",
+        )
+        self._shared_options_parser.add_argument(
             "--no-rpm-va",
             action="store_true",
             help="Skip gathering changed rpm files using"
@@ -139,7 +187,7 @@ def _register_options(self):
             " to show you what rpm files have been affected by the conversion."
             % (PRE_RPM_VA_LOG_FILENAME, POST_RPM_VA_LOG_FILENAME),
         )
-        self._parser.add_argument(
+        self._shared_options_parser.add_argument(
             "--enablerepo",
             metavar="repoidglob",
             action="append",
@@ -149,7 +197,7 @@ def _register_options(self):
             " to override the default RHEL repoids that convert2rhel enables through"
             " subscription-manager.",
         )
-        self._parser.add_argument(
+        self._shared_options_parser.add_argument(
             "--disablerepo",
             metavar="repoidglob",
             action="append",
@@ -160,29 +208,34 @@ def _register_options(self):
 
         self._add_subscription_manager_options()
         self._add_alternative_installation_options()
-        self._add_automation_options()
-
-    def _add_automation_options(self):
-        group = self._parser.add_argument_group(
-            "Automation Options",
-            "The following options are used to automate the installation",
-        )
-        group.add_argument(
-            "-r",
-            "--restart",
-            help="Restart the system when it is successfully converted to RHEL to boot the new RHEL kernel.",
-            action="store_true",
+        self._register_commands()
+        self._add_automation_options(self._convert_parser, restart=True)
+        self._add_automation_options(self._analyze_parser, restart=False)
+
+    def _add_automation_options(self, parser, restart):
+        """Prescribe what automation command line options the tool accepts."""
+        group = parser.add_argument_group(
+            title="Automation Options",
+            description="The following options are used to automate the installation",
         )
+        if restart:
+            group.add_argument(
+                "-r",
+                "--restart",
+                help="Restart the system when it is successfully converted to RHEL to boot the new RHEL kernel.",
+                action="store_true",
+            )
         group.add_argument(
             "-y",
             help="Answer yes to all yes/no questions the tool asks.",
             action="store_true",
         )
 
     def _add_alternative_installation_options(self):
-        group = self._parser.add_argument_group(
-            "Alternative Installation Options",
-            "The following options are required if you do not intend on using subscription-manager",
+        """Prescribe what alternative command line options the tool accepts."""
+        group = self._shared_options_parser.add_argument_group(
+            title="Alternative Installation Options",
+            description="The following options are required if you do not intend on using subscription-manager",
         )
         group.add_argument(
             "--disable-submgr",
@@ -192,15 +245,16 @@ def _add_alternative_installation_options(self):
         group.add_argument(
             "--no-rhsm",
             action="store_true",
-            help="Do not use subscription-manager. Use custom repositories instead. See --enablerepo/--disablerepo"
-            " options. Without this option, subscription-manager is used to access RHEL repositories by default."
-            " Using this option requires specifying --enablerepo as well.",
+            help="Do not use the subscription-manager, use custom repositories instead. See --enablerepo/--disablerepo"
+            " options. Without this option, the subscription-manager is used to access RHEL repositories by default."
+            " Using this option requires to have the --enablerepo specified.",
         )
 
     def _add_subscription_manager_options(self):
-        group = self._parser.add_argument_group(
-            "Subscription Manager Options",
-            "The following options are specific to using subscription-manager.",
+        """Prescribe what subscription manager command line options the tool accepts."""
+        group = self._shared_options_parser.add_argument_group(
+            title="Subscription Manager Options",
+            description="The following options are specific to using subscription-manager.",
         )
         group.add_argument(
             "-u",
@@ -303,9 +357,9 @@ def _process_cli_options(self):
 
         warn_on_unsupported_options()
 
-        parsed_opts = self._parser.parse_args()
-
-        global tool_opts  # pylint: disable=C0103
+        # algorithm function to properly organize all CLI args
+        argv = _add_default_command(sys.argv[1:])
+        parsed_opts = self._parser.parse_args(argv)
 
         if parsed_opts.debug:
             tool_opts.debug = True
@@ -420,7 +474,10 @@ def _process_cli_options(self):
 
         tool_opts.autoaccept = parsed_opts.y
         tool_opts.auto_attach = parsed_opts.auto_attach
-        tool_opts.restart = parsed_opts.restart
+
+        # conversion only options
+        if tool_opts.activity == "conversion":
+            tool_opts.restart = parsed_opts.restart
 
         if parsed_opts.activationkey:
             tool_opts.activation_key = parsed_opts.activationkey
@@ -593,5 +650,18 @@ def _validate_serverurl_parsing(url_parts):
     return url_parts
 
 
+def _add_default_command(argv):
+    """Add the default command when none is given"""
+    args = argv
+    for index, argument in enumerate(args):
+        if argument in ("convert", "analyze"):
+            return args
+        if not argument in PARENT_ARGS and argv[index - 1] in ARGS_WITH_VALUES:
+            break
+
+    args.insert(0, "convert")
+    return args
+
+
 # Code to be executed upon module import
 tool_opts = ToolOpts()  # pylint: disable=C0103

convert2rhel/unit_tests/toolopts_test.py

@@ -33,7 +33,10 @@
 
 
 def mock_cli_arguments(args):
-    """Return a list of cli arguments where the first one is always the name of the executable, followed by 'args'."""
+    """
+    Return a list of cli arguments where the first one is always the name of
+    the executable, followed by 'args'.
+    """
     return sys.argv[0:1] + args
 
 
@@ -557,7 +560,7 @@ def test_validate_serverurl_parsing(url_parts, message):
         convert2rhel.toolopts._validate_serverurl_parsing(url_parts)
 
 
-def test__log_command_used(caplog, monkeypatch):
+def test_log_command_used(caplog, monkeypatch):
     obfuscation_string = "*" * 5
     input_command = mock_cli_arguments(
         ["--username", "uname", "--password", "123", "--activationkey", "456", "--org", "789"]
@@ -607,6 +610,23 @@ def test_org_activation_key_specified(argv, message, monkeypatch, caplog):
     assert message in caplog.text
 
 
+@pytest.mark.parametrize(
+    ("argv", "expected"),
+    (
+        (mock_cli_arguments(["convert"]), "conversion"),
+        (mock_cli_arguments(["analyze"]), "analysis"),
+        (mock_cli_arguments([]), "conversion"),
+    ),
+)
+def test_pre_assessment_set(argv, expected, monkeypatch):
+    tool_opts.__init__()
+    monkeypatch.setattr(sys, "argv", argv)
+
+    convert2rhel.toolopts.CLI()
+
+    assert tool_opts.activity == expected
+
+
 @pytest.mark.parametrize(
     ("argv", "expected"),
     (
@@ -653,3 +673,17 @@ def test_disable_and_enable_repos_with_different_repos(argv, expected, monkeypat
     convert2rhel.toolopts.CLI()
 
     assert expected not in caplog.records[-1].message
+
+
+@pytest.mark.parametrize(
+    ("argv", "expected"),
+    (
+        ([], ["convert"]),
+        (["--debug"], ["convert", "--debug"]),
+        (["analyze", "--debug"], ["analyze", "--debug"]),
+        (["--password=convert", "--debug"], ["convert", "--password=convert", "--debug"]),
+    ),
+)
+def test_add_default_command(argv, expected, monkeypatch):
+    monkeypatch.setattr(sys, "argv", argv)
+    assert convert2rhel.toolopts._add_default_command(argv) == expected

man/__init__.py

@@ -33,5 +33,4 @@ def get_parser():
         "Oracle Linux 6/7/8, Scientific Linux 7, Alma Linux 8, and Rocky Linux 8 "
         "to the respective major version of RHEL.".strip()
     )
-    parser.usage = None
     return parser

tests/integration/conftest.py

@@ -2,6 +2,7 @@
 import logging
 import os
 import re
+import shutil
 import subprocess
 import sys
 
@@ -372,3 +373,32 @@ def missing_centos_release_workaround(system_release, shell):
         rpm_output = shell("rpm -q centos-linux-release").output
         if "not installed" in rpm_output:
             shell("yum install -y --releasever=8 centos-linux-release")
+
+
+@pytest.fixture(autouse=True)
+def os_release_hardening(shell):
+    """
+    Fixture backing up and restoring /etc/os-release and /etc/system-release.
+    Runs for non-destructive tests only.
+    Hardens test reliability.
+    """
+    # Run only if the tests are tagged with the INT_TESTS_NONDESTRUCTIVE envar
+    if os.environ.get("INT_TESTS_NONDESTRUCTIVE"):
+        # Backup the files
+        os_release = "/etc/os-release"
+        os_release_bak = "/tmp/int_tests_bak/os-release.bak"
+        system_release = "/etc/system-release"
+        system_release_bak = "/tmp/int_tests_bak/system_release.bak"
+        shell("mkdir /tmp/int_tests_bak")
+        shutil.copy(os_release, os_release_bak)
+        shutil.copy(system_release, system_release_bak)
+
+        yield
+
+        # Restore the files if missing
+        if not os.path.exists(os_release):
+            shutil.copy(os_release_bak, os_release)
+        elif not os.path.exists(system_release):
+            shutil.copy(system_release_bak, system_release)
+    else:
+        yield