Merge ~ycheng-twn/checkbox-ng:0001_checkbox_cli_show into checkbox-ng:master

[Yuan-Chen Cheng]

add show subcommand to checkbox_cli.

It output the full job or job template so that we don't
need to visit the job definition file.

[Yuan-Chen Cheng (ycheng-twn)]

sample output for job

$ checkbox-cli show com.canonical.certification::rtc
origin: /usr/share/plainbox-provider-resource-generic/jobs/resource.pxu:282-292
id: com.canonical.certification::rtc
partial_id: rtc
summary: Creates resource info for RTC
plugin: resource
command:
 if [ -e /proc/driver/rtc ]
 then
     echo "state: supported"
 else
     echo "state: unsupported"
 fi
estimated_duration: 0.02

[Yuan-Chen Cheng (ycheng-twn)]

sample outout for job template

$ checkbox-cli show com.canonical.certification::wireless/wowlan_S5_{interface}_wakeonlan
origin: /usr/share/plainbox-provider-checkbox/units/wireless/wowlan.pxu:1-26
unit: template
template_resource: device
template_filter: device.category == 'WIRELESS' and device.mac != 'UNKNOWN'
full_id: com.canonical.certification::wireless/wowlan_S5_{interface}_wakeonlan
id: com.canonical.certification::wireless/wowlan_S5_{interface}_wakeonlan
_summary: Wake on Wireless LAN (WoWLAN) test from S5 - {interface} - wakeonlan
_purpose: Check that another system can wake up from S5 the SUT using WoWLAN function.
_steps:
 1. Ensure WoWLAN is enabled in BIOS.
 2. Initiate connection to an AP (using nmcli)
 3. Configure the device for WoWLAN, run the command:
    $ sudo iw phy phy0 wowlan enable magic-packet
 4. Press Enter for S5 (Soft Off).
 5. From another computer on the same network run the following command:
    $ wakeonlan {mac}
    If wakeonlan tool is not installed run:
    $ sudo apt install wakeonlan
 6. Resume Checkbox
_verification: Did the SUT wake up from S5?
plugin: user-interact-verify
command: poweroff
user: root
category_id: com.canonical.plainbox::wireless
estimated_duration: 120
template_unit: job

[Yuan-Chen Cheng (ycheng-twn)]

As use sideload, sample output:

$ checkbox-cli show com.canonical.certification::6lowpan/kconfig
Using sideloaded provider: plainbox-provider-checkbox, version 0.65.0.dev0 from /var/tmp/checkbox-providers/plainbox-provider-checkbox
origin: /var/tmp/checkbox-providers/plainbox-provider-checkbox/units/6lowpan/jobs.pxu:1-11
id: com.canonical.certification::6lowpan/kconfig
partial_id: 6lowpan/kconfig
summary: kernel config options for 6LoWPAN
description: Checks the kernel config options for 6LoWPAN / IEEE802.15.4 support
plugin: shell
command:
 for config in CONFIG_6LOWPAN CONFIG_IEEE802154 CONFIG_IEEE802154_6LOWPAN CONFIG_MAC802154; do
    grep -E "$config=(y|m)" /boot/config-"$(uname -r)" || exit 1
 done
estimated_duration: 1.2

[Maciej Kisielewski (kissiel)]

The branch uses unsupported syntax - Xenial runs python 3.5 which doesn't support f-strings.

Also worth noting is that a similar information is printed with:

checkbox-cli list -a all_jobs (also prints info about templates)

and

checkbox-cli list -a job (prints info only about concrete jobs)

Also, check the comments below.
Functions defined in this MR are easy to be tested with UTs.

3e1e841... by Yuan-Chen Cheng on 2022-06-09

clean up and modify accoording comment from MP.
Test this version in ubuntu:xenial and jammy, and both passed.

[Yuan-Chen Cheng (ycheng-twn)]

Modify code per comment from Maciej, and clean up a bit.

[Maciej Kisielewski (kissiel)]

Thank you for all the improvements!

I know I should have asked this earlier, but why only some parts of the records are printed?
Printing all of them would be so much simpler and possibly would carry more information with it.

I also found possible problems/room for improvement in the command's positional argument(s) handling.
If the code requires an argument then just remove the `nargs='?'` from the add_argument invocation. The whole branch from the invoked() method can go away as argparse would cover it for you. Also, if calling that command without an argument is wrong then it should not return None (which ends up being return code of 0).

As for the nargs, I'd prefer to see '+' there, as it means there must be at least one, while giving the user option to specify more than one name. This would be stored as a list so it'd be super easy to iterate over.

`attr1` and `attr2` don't really communicate their purposes.

The recursive part is not trivial and could use some comment or a docstring.

IMHO the search_and_show function has too many responsibilities:
* it defines the "filters" - which are constant, but are defined in a function that's called recursively, which introduces unnecessary performance penalty
* it understands what the object it searches for is
* traverses recursively the object tree
* it calls the printing function (increasing the complexity) and making it harder to test (maybe that's why there's no tests in this MR)

IMHO much clearer (and less complex) structure would be following set of functions and their respective responsibilities:

print_concrete_job(obj) - self explanatory
print_template() - self explanatory
find_object(obj_name) - traverses the object tree and returns the object with given name

Taken into consideration the `nargs='+'` advice the find_object could take an iterable of names enabling the name check to work on the whole list (which would be faster than searching for object while iterating over list of names - recursive tree traversal is much more expensive than checking if string is in a relatively short list of strings)

89c7ed7... by Yuan-Chen Cheng on 2022-06-12

refactoring and output by origin lines first, and then all attrs

9ee651c... by Yuan-Chen Cheng on 2022-06-13

break check to save some interation

[Maciej Kisielewski (kissiel)]

There's a few problems with the code. See below.

It still feels like the complexity of this implementation is unnecessarily high.

The whole implementation could be roughly ~20LOC. And because you probably want to land this soon, and my reviews introduce delays, let me share a simpler version of how this could be implemented.

class Show():
    def register_arguments(self, parser):
        parser.add_argument(
            'IDs', nargs='+', help=_("Show the definitions of objects"))

    def invoked(self, ctx):
        providers = ctx.sa.get_selected_providers()
        self._searched_names = ctx.args.IDs
        root = Explorer(providers).get_object_tree()
        self._traverse_obj_tree(root)

    def _traverse_obj_tree(self, obj):
        if obj.name in self._searched_names:
            self._print_obj(obj)
        for child in obj.children:
            self._traverse_obj_tree(child)

    def _print_obj(self, obj):
        for k,v in obj.attrs.items():
            print("{}: {}".format(k, v))

review: Needs Fixing

[Maciej Kisielewski (kissiel)]

The implementation I provided still needs following things:
* better help string
* docstrings
* tests

[Yuan-Chen Cheng (ycheng-twn)]

As the sample code provided use the value from obj.attr. Per previous
test and check, it does not have all the important fields I'd like to
see in the output for job template.

Ex: (With the code provided)

$ /usr/local/bin/checkbox-cli show com.canonical.certification::wireless/wowlan_S5_{interface}_wakeonlan
Using sideloaded provider: plainbox-provider-checkbox, version 0.65.0.dev0 from /var/tmp/checkbox-providers/plainbox-provider-checkbox
id: com.canonical.certification::wireless/wowlan_S5_{interface}_wakeonlan
partial_id: wireless/wowlan_S5_{interface}_wakeonlan
template_unit: job
template_resource: device
template_filter: device.category == 'WIRELESS' and device.mac != 'UNKNOWN'
template_imports: None
origin: /var/tmp/checkbox-providers/plainbox-provider-checkbox/units/wireless/wowlan.pxu:1-26

You can see that it miss certain important value like the command.

While the full comment for it is:

unit: template
template-resource: device
template-filter: device.category == 'WIRELESS' and device.mac != 'UNKNOWN'
id: wireless/wowlan_S5_{interface}_wakeonlan
_summary: Wake on Wireless LAN (WoWLAN) test from S5 - {interface} - wakeonlan
_purpose:
 Check that another system can wake up from S5 the SUT using WoWLAN function.
_steps:
 1. Ensure WoWLAN is enabled in BIOS.
 2. Initiate connection to an AP (using nmcli)
 3. Configure the device for WoWLAN, run the command:
    $ sudo iw phy phy0 wowlan enable magic-packet
 4. Press Enter for S5 (Soft Off).
 5. From another computer on the same network run the following command:
    $ wakeonlan {mac}
    If wakeonlan tool is not installed run:
    $ sudo apt install wakeonlan
 6. Resume Checkbox
_verification:
  Did the SUT wake up from S5?
plugin: user-interact-verify
command: poweroff
user: root
category_id: com.canonical.plainbox::wireless
estimated_duration: 120
flags: preserve-locale

If we use obj._impl._raw_data, it misses something that would be useful for job object.

That's why I think just using the exact content from the origin file, with it's
existing format and data order, which will be not only the best quality in general,
(formatting, order of fields) but also we have the shortest code needed to have a good output.

The other way is to make sure all data field exists in obj.attr, which need more code change if I see it correctly.

[Maciej Kisielewski (kissiel)]

(previous comment deleted as LP deleted indentation in the code)

My knee-jerk reaction for re-reading file is to say it's inefficient and we shouldn't do it, but then, we only have to do it once (and one file) per listed unit. This means that when listing one unit, only one extra file open is done, which is bearable. In other words, you're absolutely right, at this stage it's the cleanest solution.
Because there's a concept of 'virtual' units, possibly not containing origin, we must be catch the potential problems stemming from that, so I propose the print to look something like this:

def _print_obj(self, obj):
    try:
        path, line_range = obj.attrs['2origin'].rsplit(':', maxsplit=1)
        start_index, end_index = [int(i) for i in line_range.split('-')]
        with open(path, 'rt', encoding='utf-8') as pxu:
            # origin uses human-like numbering (starts with 1), so we need
            # to substract 1. The range in origin is inclusive,
            # so the end_index is right
            record = pxu.readlines()[start_index - 1 : end_index]
            print(''.join(record))
    except (ValueError, KeyError):
        print("Could not read the record for {}!".format(obj.attrs['id']))
    except OSError as exc:
        print("Could not read '{}' containing record for '{}'!".format(
            path, obj.attrs['id']))

PS Shame that `attrs` is so limited for templates (broken?:)).

4efe835... by Yuan-Chen Cheng on 2022-06-17

m

[Yuan-Chen Cheng (ycheng-twn)]

go https://code.launchpad.net/~ycheng-twn/checkbox-ng/+git/checkbox-ng/+merge/424936

close this one.

Unmerged commits

4efe835... by Yuan-Chen Cheng on 2022-06-17

m

9ee651c... by Yuan-Chen Cheng on 2022-06-13

break check to save some interation

89c7ed7... by Yuan-Chen Cheng on 2022-06-12

refactoring and output by origin lines first, and then all attrs

3e1e841... by Yuan-Chen Cheng on 2022-06-09

clean up and modify accoording comment from MP.
Test this version in ubuntu:xenial and jammy, and both passed.

553ee38... by Yuan-Chen Cheng on 2022-06-06

add show to checkbox_cli that can display a full job or template.

diff --git a/checkbox_ng/launcher/checkbox_cli.py b/checkbox_ng/launcher/checkbox_cli.py
index 8df0bd0..646e00e 100644
--- a/checkbox_ng/launcher/checkbox_cli.py
+++ b/checkbox_ng/launcher/checkbox_cli.py
@@ -61,6 +61,7 @@ def main():
         'launcher': Launcher,
         'list': List,
         'run': Run,
+        'show': Show,
         'startprovider': StartProvider,
         'submit': Submit,
         'show': Show,
diff --git a/checkbox_ng/launcher/subcommands.py b/checkbox_ng/launcher/subcommands.py
index a5bf6fd..a8b2c33 100644
--- a/checkbox_ng/launcher/subcommands.py
+++ b/checkbox_ng/launcher/subcommands.py
@@ -806,6 +806,24 @@ class List():
         print_objs(ctx.args.GROUP, ctx.sa, ctx.args.attrs)
 
 
+class Show():
+    def register_arguments(self, parser):
+        parser.add_argument(
+            'NAMES', nargs='+',
+            help=_("Show the jobs or objects with the names provided"))
+
+    @property
+    def sa(self):
+        return self.ctx.sa
+
+    def invoked(self, ctx):
+        if ctx.args.NAMES is None:
+            print("You need to provide at least a job or a object name as the argument.")
+            return
+        job_name_set = set(ctx.args.NAMES)
+        show_objs(ctx.sa, job_name_set)
+
+
 class ListBootstrapped():
     @property
     def sa(self):
@@ -932,6 +950,7 @@ def print_objs(group, sa, show_attrs=False, filter_fun=None):
     _show(obj, "")
 
 
+<<<<<<< checkbox_ng/launcher/subcommands.py
 class Show():
     def register_arguments(self, parser):
         parser.add_argument(
@@ -970,3 +989,77 @@ class Show():
             # provider and service does not have origin
             for k,v in obj.attrs.items():
                 print("{}: {}".format(k, v))
+=======
+def find_objects(target_obj, obj_name_set):
+    """
+    Resursively find obj and its children any obj with name in the
+    obj_name_set.
+
+    Note: We will remove the found object name from the obj_name_set.
+      If you want keep the original job_name_set, make
+      a copy of the job_name_set before you call this function.
+    """
+    output = dict()
+
+    if (target_obj.name in obj_name_set):
+        output[target_obj.name] = target_obj
+        obj_name_set.remove(target_obj.name)
+        if obj_name_set == 0:
+            return output
+
+    for child in target_obj.children:
+        output = {**output, **(find_objects(child, obj_name_set))}
+
+    return output
+
+
+def show_objs(sa, job_name_set):
+    providers = sa.get_selected_providers()
+    obj = Explorer(providers).get_object_tree()
+
+    job_obj_dict = find_objects(obj, job_name_set.copy())
+
+    def get_origin_line_range(origin):
+        try:
+            line_range = origin.split(":")[-1]
+            origin_filename = origin[: - len(line_range) - 1]
+            range_ar = list(map(int, line_range.split("-")))
+            return {"filename": origin_filename, "range":range_ar}
+        except:
+            return {}
+
+    def print_file_range(file, file_range):
+        with open(file) as of:
+            line_no = 0
+            for line in of:
+                line_no += 1
+                if file_range[0] <= line_no and line_no <= file_range[1]:
+                    print(line, end="")
+                if line_no > file_range[1]:
+                    break
+
+    def show_obj(obj):
+        output_by_origin = False
+        if obj.attrs.get("origin", None) is not None:
+            print("origin:", obj.attrs.get("origin"))
+
+            origin = obj.attrs.get("origin")
+            origin_range_info = get_origin_line_range(origin)
+            if "filename" in origin_range_info:
+                print_file_range(
+                    origin_range_info["filename"],
+                    origin_range_info["range"])
+                output_by_origin = True
+
+        if not output_by_origin:
+            print(obj.group + ":", obj.name)
+            for key, item in obj.attrs.items():
+                print("{}: {}".format(key, item))
+
+    if len(job_obj_dict) == 0:
+        raise SystemExit("Can't find any in {}.".format(job_name_set))
+
+    for _key, obj_value in job_obj_dict.items():
+        print("=" * 60)
+        show_obj(obj_value)
+>>>>>>> checkbox_ng/launcher/subcommands.py