install: improve CLI positional argument handling

Instead of specifying which knobs should be positional arguments in
cli.install_tool(), do it using a flag in knob definition, where the rest
of CLI configuration is.

As a side effect, the usage string for CLI tools can now be generated
automatically.

https://fedorahosted.org/freeipa/ticket/6392

Reviewed-By: Martin Basti <mbasti@redhat.com>

install/tools/ipa-replica-install

@@ -26,8 +26,6 @@ from ipaserver.install.server import Replica
 ReplicaInstall = cli.install_tool(
     Replica,
     command_name='ipa-replica-install',
-    positional_arguments=['replica_file'],
-    usage='%prog [options] [REPLICA_FILE]',
     log_file_name=paths.IPAREPLICA_INSTALL_LOG,
     debug_option=True,
 )

ipapython/install/cli.py

@@ -23,20 +23,39 @@ if six.PY3:
     long = int
 
 
+def _get_usage(configurable_class):
+    usage = '%prog [options]'
+
+    for owner_cls, name in configurable_class.knobs():
+        knob_cls = getattr(owner_cls, name)
+        if knob_cls.cli_positional:
+            if knob_cls.cli_metavar is not None:
+                metavar = knob_cls.cli_metavar
+            elif knob_cls.cli_name is not None:
+                metavar = knob_cls.cli_name.upper()
+            else:
+                metavar = name.replace('_', '-').upper()
+
+            try:
+                knob_cls.default
+            except AttributeError:
+                fmt = ' {}'
+            else:
+                fmt = ' [{}]'
+
+            usage += fmt.format(metavar)
+
+    return usage
+
+
 def install_tool(configurable_class, command_name, log_file_name,
-                 positional_arguments=None, usage=None, debug_option=False,
+                 debug_option=False, use_private_ccache=True,
-                 use_private_ccache=True,
+                 uninstall_log_file_name=None):
-                 uninstall_log_file_name=None,
+    if uninstall_log_file_name is not None:
-                 uninstall_positional_arguments=None, uninstall_usage=None):
-    if (uninstall_log_file_name is not None or
-            uninstall_positional_arguments is not None or
-            uninstall_usage is not None):
         uninstall_kwargs = dict(
             configurable_class=configurable_class,
             command_name=command_name,
             log_file_name=uninstall_log_file_name,
-            positional_arguments=uninstall_positional_arguments,
-            usage=uninstall_usage,
             debug_option=debug_option,
         )
     else:
@@ -49,8 +68,7 @@ def install_tool(configurable_class, command_name, log_file_name,
             configurable_class=configurable_class,
             command_name=command_name,
             log_file_name=log_file_name,
-            positional_arguments=positional_arguments,
+            usage=_get_usage(configurable_class),
-            usage=usage,
             debug_option=debug_option,
             uninstall_kwargs=uninstall_kwargs,
             use_private_ccache=use_private_ccache,
@@ -59,7 +77,7 @@ def install_tool(configurable_class, command_name, log_file_name,
 
 
 def uninstall_tool(configurable_class, command_name, log_file_name,
-                   positional_arguments=None, usage=None, debug_option=False):
+                   debug_option=False):
     return type(
         'uninstall_tool({0})'.format(configurable_class.__name__),
         (UninstallTool,),
@@ -67,8 +85,7 @@ def uninstall_tool(configurable_class, command_name, log_file_name,
             configurable_class=configurable_class,
             command_name=command_name,
             log_file_name=log_file_name,
-            positional_arguments=positional_arguments,
+            usage=_get_usage(configurable_class),
-            usage=usage,
             debug_option=debug_option,
         )
     )
@@ -77,7 +94,6 @@ def uninstall_tool(configurable_class, command_name, log_file_name,
 class ConfigureTool(admintool.AdminTool):
     configurable_class = None
     debug_option = False
-    positional_arguments = None
     use_private_ccache = True
 
     @staticmethod
@@ -104,7 +120,7 @@ class ConfigureTool(admintool.AdminTool):
 
         for owner_cls, name in transformed_cls.knobs():
             knob_cls = getattr(owner_cls, name)
-            if cls.positional_arguments and name in cls.positional_arguments:
+            if knob_cls.cli_positional:
                 continue
 
             group_cls = owner_cls.group()
@@ -233,36 +249,40 @@ class ConfigureTool(admintool.AdminTool):
 
         return value
 
+    def __init__(self, options, args):
+        super(ConfigureTool, self).__init__(options, args)
+
+        self.transformed_cls = self._transform(self.configurable_class)
+        self.positional_arguments = []
+        knob_clss = {}
+
+        for owner_cls, name in self.transformed_cls.knobs():
+            knob_cls = getattr(owner_cls, name)
+            if knob_cls.cli_positional:
+                self.positional_arguments.append(name)
+                knob_clss[name] = knob_cls
+
+        for index, name in enumerate(self.positional_arguments):
+            knob_cls = knob_clss[name]
+            try:
+                value = self.args.pop(0)
+            except IndexError:
+                break
+
+            old_value = getattr(self.options, name, None)
+            try:
+                value = self._parse_knob(knob_cls, old_value, value)
+            except ValueError as e:
+                self.option_parser.error(
+                    "argument {0}: {1}".format(index + 1, e))
+
+            setattr(self.options, name, value)
+
     def validate_options(self, needs_root=True):
         super(ConfigureTool, self).validate_options(needs_root=needs_root)
 
-        if self.positional_arguments:
+        if self.args:
-            if len(self.args) > len(self.positional_arguments):
+            self.option_parser.error("Too many arguments provided")
-                self.option_parser.error("Too many arguments provided")
-
-            index = 0
-
-            transformed_cls = self._transform(self.configurable_class)
-            for owner_cls, name in transformed_cls.knobs():
-                knob_cls = getattr(owner_cls, name)
-                if name not in self.positional_arguments:
-                    continue
-
-                try:
-                    value = self.args[index]
-                except IndexError:
-                    break
-
-                old_value = getattr(self.options, name, None)
-                try:
-                    value = self._parse_knob(knob_cls, old_value, value)
-                except ValueError as e:
-                    self.option_parser.error(
-                        "argument {0}: {1}".format(index + 1, e))
-
-                setattr(self.options, name, value)
-
-                index += 1
 
     def _setup_logging(self, log_file_mode='w', no_file=False):
         if no_file:
@@ -298,8 +318,6 @@ class ConfigureTool(admintool.AdminTool):
         except core.KnobValueError as e:
             knob_cls = knob_classes[e.name]
             try:
-                if self.positional_arguments is None:
-                    raise ValueError
                 index = self.positional_arguments.index(e.name)
             except ValueError:
                 cli_name = knob_cls.cli_name or e.name.replace('_', '-')

ipapython/install/core.py

@@ -110,6 +110,7 @@ class KnobBase(PropertyBase):
     sensitive = False
     deprecated = False
     description = None
+    cli_positional = False
     cli_name = None
     cli_short_name = None
     cli_aliases = None
@@ -141,8 +142,9 @@ class KnobBase(PropertyBase):
 
 
 def Knob(type_or_base, default=_missing, sensitive=_missing,
-         deprecated=_missing, description=_missing, cli_name=_missing,
+         deprecated=_missing, description=_missing, cli_positional=_missing,
-         cli_short_name=_missing, cli_aliases=_missing, cli_metavar=_missing):
+         cli_name=_missing, cli_short_name=_missing, cli_aliases=_missing,
+         cli_metavar=_missing):
     class_dict = {}
     class_dict['_order'] = next(_counter)
 
@@ -159,6 +161,8 @@ def Knob(type_or_base, default=_missing, sensitive=_missing,
         class_dict['deprecated'] = deprecated
     if description is not _missing:
         class_dict['description'] = description
+    if cli_positional is not _missing:
+        class_dict['cli_positional'] = cli_positional
     if cli_name is not _missing:
         class_dict['cli_name'] = cli_name
     if cli_short_name is not _missing:

ipaserver/install/server/replicainstall.py

@@ -1444,6 +1444,8 @@ class Replica(BaseServer):
     replica_file = Knob(
         str, None,
         description="a file generated by ipa-replica-prepare",
+        cli_positional=True,
+        cli_name='replica_file',
     )
 
     setup_ca = Knob(BaseServer.setup_ca)