samba-tool: all subcommands know --version

Before `samba-tool -V` would give you the version,
but `samba-tool spn -V` would complain.

An ad-hoc selection of sub-commands already supported --version,
depending on whether VersionOptions was manually added to the
takes_options dict. The .run() methods of these subcommands all take a
'versionopts' keyword argument, but never use it. If it was set (i.e.,
argv contained "--version"), the process never gets to .run(), so the
value of versionopts.version is always None in run(). After this
commit we can remove VersionOptions/versionopts from sub-commands.

BUG: https://bugzilla.samba.org/show_bug.cgi?id=15770

Signed-off-by: Douglas Bagnall <douglas.bagnall@catalyst.net.nz>
Reviewed-by: Jennifer Sutton <jennifersutton@catalyst.net.nz>

python/samba/netcmd/__init__.py

@@ -30,6 +30,7 @@ from samba.getopt import Option, OptionParser
 from samba.logger import get_samba_logger
 from samba.samdb import SamDB
 from samba.dcerpc.security import SDDLValueError
+from samba import getopt as options
 
 from .encoders import JSONEncoder
 
@@ -281,10 +282,16 @@ class Command(object):
             option_class=Option)
         parser.add_options(self.takes_options)
         optiongroups = {}
+
+        # let samba-tool subcommands process --version
+        if "versionopts" not in self.takes_optiongroups:
+            self.takes_optiongroups["_versionopts"] = options.VersionOptions
+
         for name in sorted(self.takes_optiongroups.keys()):
             optiongroup = self.takes_optiongroups[name]
             optiongroups[name] = optiongroup(parser)
             parser.add_option_group(optiongroups[name])
+
         if self.use_colour:
             parser.add_option("--color",
                               help="use colour if available (default: auto)",
@@ -313,12 +320,36 @@ class Command(object):
             return -1
 
         # Filter out options from option groups
+        #
+        # run() methods on subclasses receive all direct options as
+        # keyword arguments, but options that come from OptionGroups
+        # (for example, --configfile from SambaOpts group) are removed
+        # from the direct keyword arguments list, and the option group
+        # itself becomes a keyword argument. The option can be
+        # accessed as an attribute of that (e.g. sambaopts.configfile).
+        #
+        # This allows for option groups to grow without needing to
+        # change the signature for all samba-tool tools.
+        #
+        # _versionopts special case.
+        # ==========================
+        #
+        # The _versionopts group was added automatically, and is
+        # removed here. It only has the -V/--version option, and that
+        # will have triggered already if given (as will --help, and
+        # errors on unknown options).
+        #
+        # Some subclasses take 'versionopts' which they expect to
+        # receive but never use.
+
         kwargs = dict(opts.__dict__)
-        for option_group in parser.option_groups:
+
+        for og_name, option_group in optiongroups.items():
             for option in option_group.option_list:
                 if option.dest is not None and option.dest in kwargs:
                     del kwargs[option.dest]
-        kwargs.update(optiongroups)
+            if og_name != '_versionopts':
+                kwargs[og_name] = option_group
 
         if kwargs.get('output_format') == 'json':
             self.preferred_output_format = 'json'

python/samba/netcmd/main.py

@@ -17,7 +17,6 @@
 
 """The main samba-tool command implementation."""
 
-from samba import getopt as options
 
 from samba.netcmd import SuperCommand
 
@@ -52,10 +51,6 @@ class cache_loader(dict):
 class cmd_sambatool(SuperCommand):
     """Main samba administration tool."""
 
-    takes_optiongroups = {
-        "versionopts": options.VersionOptions,
-    }
-
     subcommands = cache_loader()
 
     subcommands["computer"] = None

selftest/knownfail.d/samba-tool--version

@@ -1,6 +1,4 @@
 ^samba\.tests\.samba_tool\.help\.samba\.tests\.samba_tool\.help\.HelpTestCase\.test_help_version\(none\)
-^samba\.tests\.samba_tool\.help\.samba\.tests\.samba_tool\.help\.HelpTestCase\.test_sub_command_version\(none\)
 ^samba\.tests\.samba_tool\.help\.samba\.tests\.samba_tool\.help\.HelpTestCase\.test_version_beats_help\(none\)
 ^samba\.tests\.samba_tool\.help\.samba\.tests\.samba_tool\.help\.HelpTestCase\.test_help_version\(fileserver\)
-^samba\.tests\.samba_tool\.help\.samba\.tests\.samba_tool\.help\.HelpTestCase\.test_sub_command_version\(fileserver\)
 ^samba\.tests\.samba_tool\.help\.samba\.tests\.samba_tool\.help\.HelpTestCase\.test_version_beats_help\(fileserver\)