samba-tool: separate ._run() from command resolution

Prior to this commit, in super-commands, the first half of the _run()
is resolving what sub-command to run, and the second half is working
out what to print if that failed. Some issues with that are:

 * it looks a little bit complicated.

 * the tests can't use the tool's resolution code, because it runs
   immediately, while the tests first want to fiddle with self.outf
   and so on.

 * it makes it harder to subclass and override the resolution code, so
   instead we do strange things like where we subclass dict as in
   main.py.

So we split it into ._resolve() and ._run().

There are a few tests that break. We mark these as flapping, rather
than knownfail, so as to avoid going into extremely fine-grain filters
for tests that will be fixed within a few commits.

Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet@samba.org>

diff --git a/python/samba/netcmd/__init__.py b/python/samba/netcmd/__init__.py
index 21738cf51fb408a9b0dabbb51303f3b305ea6fca..a2ad180b2c3857e56fa42ebbda27d4e9c51f28c8 100644 (file)
--- a/python/samba/netcmd/__init__.py
+++ b/python/samba/netcmd/__init__.py
@@ -158,9 +158,15 @@ class Command(object):
     def message(self, text):
         self.outf.write(text + "\n")
 
+
+    def _resolve(self, path, *argv):
+        """This is a leaf node, the command that will actually run."""
+        self.command_name = path
+        return (self, argv)
+
     def _run(self, *argv):
-        parser, optiongroups = self._create_parser(argv[0])
-        opts, args = parser.parse_args(list(argv))
+        parser, optiongroups = self._create_parser(self.command_name)
+        opts, args = parser.parse_args([self.command_name] + list(argv))
         # Filter out options from option groups
         args = args[1:]
         kwargs = dict(opts.__dict__)
@@ -229,43 +235,60 @@ class SuperCommand(Command):
 
     subcommands = {}
 
-    def _run(self, myname, subcommand=None, *args):
-        if subcommand in self.subcommands:
-            return self.subcommands[subcommand]._run(
-                "%s %s" % (myname, subcommand), *args)
-        elif subcommand not in [ '--help', 'help', None, '-h', '-V', '--version' ]:
-            print("%s: no such subcommand: %s\n" % (myname, subcommand))
-            args = []
-
-        if subcommand == 'help':
-            # pass the request down
-            if len(args) > 0:
-                sub = self.subcommands.get(args[0])
-                if isinstance(sub, SuperCommand):
-                    return sub._run("%s %s" % (myname, args[0]), 'help',
-                                    *args[1:])
-                elif sub is not None:
-                    return sub._run("%s %s" % (myname, args[0]), '--help',
-                                    *args[1:])
-
-            subcommand = '--help'
+    def _resolve(self, path, *args):
+        """This is an internal node. We need to consume one of the args and
+        find the relevant child, returning an instance of that Command.
+
+        If there are no children, this SuperCommand will be returned
+        and its _run() will do a --help like thing.
+        """
+        self.command_name = path
+
+        # We collect up certain option arguments and pass them to the
+        # leaf, which is why we iterate over args, though we really
+        # expect to return in the first iteration.
+        deferred_args = []
+
+        for i, a in enumerate(args):
+            if a in self.subcommands:
+                sub_args = args[i + 1:] + tuple(deferred_args)
+                sub_path = f'{path} {a}'
+
+                sub = self.subcommands[a]
+                return sub._resolve(sub_path, *sub_args)
 
+            elif a in [ '--help', 'help', None, '-h', '-V', '--version' ]:
+                # we pass these to the leaf node.
+                if a == 'help':
+                    a = '--help'
+                deferred_args.append(a)
+                continue
+
+            # they are talking nonsense
+            print("%s: no such subcommand: %s\n" % (path, a), file=self.outf)
+            return (self, [])
+
+        # We didn't find a subcommand, but maybe we found e.g. --version
+        print("%s: missing subcommand\n" % (path), file=self.outf)
+        return (self, deferred_args)
+
+    def _run(self, *argv):
         epilog = "\nAvailable subcommands:\n"
         subcmds = list(self.subcommands.keys())
         subcmds.sort()
         max_length = max([len(c) for c in subcmds])
         for cmd_name in subcmds:
             cmd = self.subcommands[cmd_name]
-            if not cmd.hidden:
-                epilog += "  %*s  - %s\n" % (
-                    -max_length, cmd_name, cmd.short_description)
-        epilog += "For more help on a specific subcommand, please type: %s <subcommand> (-h|--help)\n" % myname
-
-        parser, optiongroups = self._create_parser(myname, epilog=epilog)
-        args_list = list(args)
-        if subcommand:
-            args_list.insert(0, subcommand)
-        opts, args = parser.parse_args(args_list)
+            if cmd.hidden:
+                continue
+            epilog += "  %*s  - %s\n" % (
+                -max_length, cmd_name, cmd.short_description)
+
+        epilog += ("For more help on a specific subcommand, please type: "
+                   f"{self.command_name} <subcommand> (-h|--help)\n")
+
+        parser, optiongroups = self._create_parser(self.command_name, epilog=epilog)
+        opts, args = parser.parse_args([self.command_name] + list(argv))
 
         parser.print_help()
         return -1

diff --git a/selftest/flapping.d/samba-tool b/selftest/flapping.d/samba-tool
new file mode 100644 (file)
index 0000000..f277c8e
--- /dev/null
+++ b/selftest/flapping.d/samba-tool
@@ -0,0 +1,3 @@
+^samba.tests.netcmd.
+^samba.tests.blackbox.samba_dnsupdate
+^samba4.ldap.password_lockout.python

diff --git a/source4/scripting/bin/samba-tool b/source4/scripting/bin/samba-tool
index f8a70a6b295ef04fcfde7af1b3ef6d74a8308019..b5f784beff7c8630671d08fda2ebab070903bbf7 100755 (executable)
--- a/source4/scripting/bin/samba-tool
+++ b/source4/scripting/bin/samba-tool
@@ -32,16 +32,10 @@ signal.signal(signal.SIGINT, signal.SIG_DFL)
 
 from samba.netcmd.main import cmd_sambatool
 cmd = cmd_sambatool()
-subcommand = None
-args = ()
-
-if len(sys.argv) > 1:
-    subcommand = sys.argv[1]
-    if len(sys.argv) > 2:
-        args = sys.argv[2:]
 
 try:
-    retval = cmd._run("samba-tool", subcommand, *args)
+    command, args = cmd._resolve("samba-tool", *sys.argv[1:])
+    retval = command._run(*args)
 except SystemExit as e:
     retval = e.code
 except Exception as e:

diff --git a/source4/scripting/bin/samba_dnsupdate b/source4/scripting/bin/samba_dnsupdate
index 1ce53f5dc390556fec9da817c3f8fcd5591cd12f..0db490e21e957d418230e489a1d25e3446a6479a 100755 (executable)
--- a/source4/scripting/bin/samba_dnsupdate
+++ b/source4/scripting/bin/samba_dnsupdate
@@ -590,7 +590,8 @@ def call_samba_tool(d, op="add", zone=None):
         cmd = cmd_dns()
         if opts.verbose:
             print(f'Calling samba-tool dns {op} --use-kerberos off -P {args}')
-        ret = cmd._run("dns", op, "--use-kerberos", "off", "-P", *args)
+        command, args = cmd._resolve("dns", op, "--use-kerberos", "off", "-P", *args)
+        ret = command._run(*args)
         if ret == -1:
             if opts.fail_immediately:
                 sys.exit(1)