2016-03-02 12:56 GMT+01:00 Jan Tluka <jtluka@redhat.com>:
Wed, Mar 02, 2016 at 12:06:31PM CET, jprochaz@redhat.com wrote:
>This patch adds new action called list_pools. It has two modes of
>execution, non-verbose and verbose. Non verbose, executed just with
>"lnst-ctl list_pools" command, prints out pool names and their paths
>in pool_name=/path/to/pool format.
>
>For verbose mode, option -v or --verbose must be entered. It prints
>pool names and their paths and in addition, it lists through all
>.xml files in pool directories and prints their contents in more
>human readable format.
>
>Pools are acquired from all the configs lnst-ctl would use in
>normal run.
>

I think you should also include that you're substituting the dump-pools
command, right? It's quite big change from the user point of view so
don't forget to include this in the commit message.

​ok, will do in v2 of the patchset, thanks​

 

>Signed-off-by: Jiri Prochazka <jprochaz@redhat.com>
>---
> lnst-ctl | 63 +++++++++++++++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 49 insertions(+), 14 deletions(-)
>
>diff --git a/lnst-ctl b/lnst-ctl
>index 6d656f8..5a32f7c 100755
>--- a/lnst-ctl
>+++ b/lnst-ctl
>@@ -24,6 +24,8 @@ from lnst.Common.Utils import mkdir_p
> from lnst.Controller.NetTestController import NetTestController, NetTestError
> from lnst.Controller.NetTestController import NoMatchError
> from lnst.Controller.NetTestResultSerializer import NetTestResultSerializer
>+from lnst.Controller.SlaveMachineParser import SlaveMachineParser
>+from lnst.Controller.SlavePool import SlavePool
> from lnst.Controller.XmlProcessing import XmlProcessingError
>
> RETVAL_PASS = 0
>@@ -35,7 +37,8 @@ def usage(retval=0):
>     """
>     print "Usage: %s [OPTIONS...] ACTION [RECIPES...]" % sys.argv[0]
>     print ""
>-    print "ACTION = [ run | config_only | deconfigure | match_setup ]"
>+    print "ACTION = [ run | config_only | deconfigure | match_setup | " \
>+                     "list_pools]"
>     print ""
>     print "OPTIONS"
>     print "  -A, --override-alias name=value define top-level alias that " \
>@@ -45,8 +48,8 @@ def usage(retval=0):
>     print "  -d, --debug                     emit debugging messages"
>     print "      --dump-config               dumps the join of all loaded " \
>               "configuration files on stdout and exits"
>-    print "      --dump-pools                dumps the available machine " \
>-              "pools and exits"
>+    print "  -v, --verbose                   verbose version of list_pools " \
>+              "command"
>     print "  -h, --help                      print this message"
>     print "  -m, --no-colours                disable coloured terminal output"
>     print "  -o, --disable-pool-checks       don't check the availability of " \
>@@ -67,6 +70,35 @@ def usage(retval=0):
>     print "  -x, --result=FILE               file to write xml_result"
>     sys.exit(retval)
>
>+def list_pools():
>+    # iterate over all pools
>+    for pool_name, pool_path in lnst_config.get_pools().items():
>+        print "%s (%s)" % (pool_name, pool_path)
>+        dentries = os.listdir(pool_path)
>+        # iterate over all slave machine cfgs
>+        for dirent in dentries:
>+            filepath = pool_path + "/" + dirent
>+            dirname, basename = os.path.split(filepath)
>+            if os.path.isfile(filepath) and re.search("\.xml$", filepath, re.I):
>+                parser = SlaveMachineParser(filepath)
>+                xml_data = parser.parse()
>+                # parse XML to dict
>+                res = SlavePool._process_machine_xml_data(SlavePool({}), "", xml_data)

The underscore means that this method should not be called directly.

​I know, I didn't want to write any duplicate code though. What would
you recommend as suggested approach to this? Rename and rework the method
or just write my own xml processing? Again, thanks for the feedback​


>+                # print in human-readable format
>+                print " ", "Filename: ", basename
>+                if res['params']:
>+                    print 3*" ", "Parameters:"
>+                for key in res['params']:
>+                    print 7*" ", key+":", res['params'][key]
>+                if res['interfaces']:
>+                    print 3*" ", "Interfaces:"
>+                for key in res['interfaces']:
>+                    print 5*" ", "id:", key, "\tnetwork:", res['interfaces'][key]['network']
>+                    for param in res['interfaces'][key]['params']:
>+                        print 7*" ", param+":", res['interfaces'][key]['params'][param]
>+                print ""
>+    return RETVAL_PASS
>+
> def store_alias(alias_def, aliases_dict):
>     try:
>         name, value = alias_def.split("=")
>@@ -189,7 +221,7 @@ def main():
>     try:
>         opts, args = getopt.getopt(
>             sys.argv[1:],
>-            "A:a:c:dhmoprs:t:ux:",
>+            "A:a:c:dhmoprs:t:ux:v",
>             [
>              "override_alias=",
>              "define_alias=",
>@@ -206,7 +238,7 @@ def main():
>              "multi-match",
>              "result=",
>              "pools=",
>-             "dump-pools"
>+             "verbose"
>             ]
>         )
>     except getopt.GetoptError as err:
>@@ -246,7 +278,7 @@ def main():
>     reduce_sync = False
>     multi_match = False
>     dump_config = False
>-    dump_pools = False
>+    verbose = False
>     pools = []
>     for opt, arg in opts:
>         if opt in ("-d", "--debug"):
>@@ -283,8 +315,8 @@ def main():
>             dump_config = True
>         elif opt in ("--pools"):
>             pools.extend(arg.split(","))
>-        elif opt in ("--dump-pools"):
>-            dump_pools = True
>+        elif opt in ("-v", "--verbose"):
>+            verbose= True
>
>     if xslt_url != None:
>         lnst_config.set_option("environment", "xslt_url", xslt_url)
>@@ -293,11 +325,6 @@ def main():
>         print lnst_config.dump_config()
>         return RETVAL_PASS
>
>-    if dump_pools:
>-        for pool_name, pool_path in lnst_config.get_pools().items():
>-            print pool_name + ' = ' + pool_path
>-        return RETVAL_PASS
>-
>     if coloured_output:
>         coloured_output = not lnst_config.get_option("colours",
>                                                      "disable_colours")
>@@ -315,12 +342,20 @@ def main():
>
>     action = args[0]
>     recipes = args[1:]
>-    if not action in ['run', 'config_only', 'deconfigure', 'match_setup']:
>+    if not action in ['run', 'config_only', 'deconfigure', 'match_setup',
>+                      'list_pools']:
>         logging.error("Action '%s' not recognised" % action)
>         usage(RETVAL_ERR)
>     if action == 'deconfigure':
>         NetTestController.remove_saved_machine_config()
>         return 0
>+    elif action == 'list_pools':
>+        logging.disable(logging.CRITICAL)
>+        for pool_name, pool_path in lnst_config.get_pools().items():
>+            print pool_name + ' = ' + pool_path
>+        if verbose:
>+            list_pools()
>+        return RETVAL_PASS
>
>     if recipes == []:
>         logging.error("No recipe specified!")
>--
>2.4.3
>_______________________________________________
>LNST-developers mailing list
>lnst-developers@lists.fedorahosted.org
>https://lists.fedorahosted.org/admin/lists/lnst-developers@lists.fedorahosted.org