Remove newly unnecessary code

Signed-off-by: Matthias Büchse <matthias.buechse@alasca.cloud>

Author: mbuechse

Tests/scs-compliance-check.py

@@ -5,7 +5,7 @@
 #
 # (c) Eduard Itrich <eduard@itrich.net>
 # (c) Kurt Garloff <kurt@garloff.de>
-# (c) Matthias Büchse <matthias.buechse@cloudandheat.com>
+# (c) Matthias Büchse <matthias.buechse@alasca.cloud>
 # SPDX-License-Identifier: Apache-2.0
 
 """Main SCS compliance checker
@@ -26,7 +26,6 @@
 import getopt
 import datetime
 import subprocess
-from collections import defaultdict
 from itertools import chain
 import logging
 import yaml
@@ -153,10 +152,6 @@ def select_valid(versions: list) -> list:
     return [version for version in versions if version['_explicit_validity']]
 
 
-def suppress(*args, **kwargs):
-    return
-
-
 def invoke_check_tool(exe, args, env, cwd):
     """run check tool and return invokation dict to use in the report"""
     try:
@@ -182,7 +177,7 @@ def invoke_check_tool(exe, args, env, cwd):
     return invokation
 
 
-def compute_results(stdout):
+def compute_results(stdout, permissible_ids=()):
     """pick out test results from stdout lines"""
     result = {}
     for line in stdout:
@@ -192,15 +187,18 @@ def compute_results(stdout):
         value = TESTCASE_VERDICTS.get(parts[1].strip().upper())
         if value is None:
             continue
-        result[parts[0].strip()] = value
+        testcase_id = parts[0].strip()
+        if permissible_ids and testcase_id not in permissible_ids:
+            logger.warning(f"ignoring invalid result id: {testcase_id}")
+            continue
+        result[testcase_id] = value
     return result
 
 
 class CheckRunner:
     def __init__(self, cwd, assignment, verbosity=0):
         self.cwd = cwd
         self.assignment = assignment
-        self.memo = {}
         self.num_abort = 0
         self.num_error = 0
         self.verbosity = verbosity
@@ -212,61 +210,30 @@ def run(self, check, testcases=()):
         args = check.get('args', '').format(**assignment)
         env = {key: value.format(**assignment) for key, value in check.get('env', {}).items()}
         env_str = " ".join(f"{key}={value}" for key, value in env.items())
-        memo_key = f"{env_str} {check['executable']} {args}".strip()
-        logger.debug(f"running {memo_key!r}...")
-        invocation = self.memo.get(memo_key)
-        if invocation is None:
-            check_env = {**os.environ, **env}
-            invocation = invoke_check_tool(check["executable"], args, check_env, self.cwd)
-            invocation = {
-                'id': str(uuid.uuid4()),
-                'cmd': memo_key,
-                'result': 0,  # keep this for backwards compatibility
-                'results': compute_results(invocation['stdout']),
-                **invocation
-            }
-            if self.verbosity > 1 and invocation["stdout"]:
-                print("\n".join(invocation["stdout"]))
-                self.spamminess += 1
-            # the following check used to be "> 0", but this is quite verbose...
-            if invocation['rc'] or self.verbosity > 1 and invocation["stderr"]:
-                print("\n".join(invocation["stderr"]))
-                self.spamminess += 1
-            self.memo[memo_key] = invocation
+        cmd = f"{env_str} {check['executable']} {args}".strip()
+        logger.debug(f"running {cmd!r}...")
+        check_env = {**os.environ, **env}
+        invocation = invoke_check_tool(check["executable"], args, check_env, self.cwd)
+        invocation = {
+            'id': str(uuid.uuid4()),
+            'cmd': cmd,
+            'results': compute_results(invocation['stdout'], permissible_ids=testcases),
+            **invocation
+        }
+        if self.verbosity > 1 and invocation["stdout"]:
+            print("\n".join(invocation["stdout"]))
+            self.spamminess += 1
+        # the following check used to be "> 0", but this is quite verbose...
+        if invocation['rc'] or self.verbosity > 1 and invocation["stderr"]:
+            print("\n".join(invocation["stderr"]))
+            self.spamminess += 1
         logger.debug(f".. rc {invocation['rc']}, {invocation['critical']} critical, {invocation['error']} error")
         self.num_abort += invocation["critical"]
         self.num_error += invocation["error"]
         # count failed testcases because they need not be reported redundantly on the error channel
         self.num_error + len([value for value in invocation['results'].values() if value < 0])
         return invocation
 
-    def get_invocations(self):
-        return {invocation['id']: invocation for invocation in self.memo.values()}
-
-
-class ResultBuilder:
-    def __init__(self, name):
-        self.name = name
-        self._raw = defaultdict(list)
-
-    def record(self, id_, **kwargs):
-        self._raw[id_].append(kwargs)
-
-    def finalize(self, permissible_ids=None):
-        final = {}
-        for id_, ls in self._raw.items():
-            if permissible_ids is not None and id_ not in permissible_ids:
-                logger.warning(f"ignoring invalid result id: {id_}")
-                continue
-            # just in case: sort by value (worst first)
-            ls.sort(key=lambda item: item['result'])
-            winner, runnerups = ls[0], ls[1:]
-            if runnerups:
-                logger.warning(f"multiple result values for {id_}")
-                winner = {**winner, 'runnerups': runnerups}
-            final[id_] = winner
-        return final
-
 
 def print_report(subject: str, title: str, testcase_lookup: dict, targets: dict, results: dict, partial=False, verbose=False):
     print(f"{subject} {title}:")
@@ -294,12 +261,12 @@ def print_report(subject: str, title: str, testcase_lookup: dict, targets: dict,
             print(f"  - {category}:")
             for tc_id in offenders:
                 print(f"    - {tc_id}:")
-                _, testcase = testcase_lookup[tc_id]
+                testcase = testcase_lookup[tc_id]
                 if 'description' in testcase:  # used to be `verbose and ...`, but users need the URL!
                     print(f"      > {testcase['description'].strip()}")
 
 
-def create_report(argv, config, spec, results, invocations):
+def create_report(argv, config, spec, invocations):
     return {
         # these fields are essential:
         # results are no longer specific to version!
@@ -320,7 +287,7 @@ def create_report(argv, config, spec, results, invocations):
             "sections": config.sections,
             "forced_version": config.version or None,
             "forced_tests": None if config.tests is None else config.tests.pattern,
-            "invocations": invocations,
+            "invocations": {invocation['id']: invocation for invocation in invocations},
         },
     }
 
@@ -358,15 +325,16 @@ def main(argv):
         title += f" [tests: '{config.tests.pattern}']"
         partial = True
     # collect all testcases we need
-    all_testcases = set()
+    all_testcase_ids = set()
     for version in versions:
-        for target, testcase_ids in version['targets'].items():
-            all_testcases.update(testcase_ids)
+        for testcase_ids in version['targets'].values():
+            all_testcase_ids.update(testcase_ids)
     # collect scripts to be run
     testcase_lookup = spec['testcases']
+    tc_script_lookup = spec['tc_scripts']
     script_info = {}
-    for tc_id in all_testcases:
-        script, testcase = testcase_lookup[tc_id]
+    for tc_id in all_testcase_ids:
+        script = tc_script_lookup[tc_id]
         if 'executable' not in script:
             continue  # manual check
         if config.sections and script.get('section') not in config.sections:
@@ -375,18 +343,18 @@ def main(argv):
             continue
         item = script_info.get(id(script))
         if item is None:
-            _, testcases = script_info[id(script)] = (script, [])
+            _, testcase_ids = script_info[id(script)] = (script, [])
         else:
-            _, testcases = item
-        testcases.append(tc_id)
+            _, testcase_ids = item
+        testcase_ids.append(tc_id)
     # run scripts
-    builder = ResultBuilder('(dummy)')
-    for item in script_info.values():
-        script, testcases = item
-        invocation = runner.run(script, testcases=sorted(testcases))
-        for id_, value in invocation["results"].items():
-            builder.record(id_, result=value, invocation=invocation['id'])
-    results = builder.finalize(permissible_ids=all_testcases)
+    invocations = [
+        runner.run(script, testcases=sorted(testcases))
+        for script, testcases in script_info.values()
+    ]
+    results = {}
+    for invocation in invocations:
+        results.update(invocation['results'])
     # now report: to console if requested, and likewise for yaml output
     if not config.quiet:
         # print a horizontal line if we had any script output
@@ -395,7 +363,7 @@ def main(argv):
         for version in versions:
             print_report(config.subject, f"{title} {version['version']}", testcase_lookup, version['targets'], results, partial, config.verbose)
     if config.output:
-        report = create_report(argv, config, spec, results, runner.get_invocations())
+        report = create_report(argv, config, spec, invocations)
         with open(config.output, 'w', encoding='UTF-8') as fileobj:
             yaml.safe_dump(report, fileobj, default_flow_style=False, sort_keys=False, explicit_start=True)
     return min(127, runner.num_abort + (0 if config.critical_only else runner.num_error))

Tests/scs_cert_lib.py

@@ -57,13 +57,15 @@ def _resolve_spec(spec: dict):
     # - modules, referenced by id
     # - versions, referenced by name (unfortunately, the field is called "version")
     # step 1. build lookups
-    testcase_lookup = {}  # {testcase['id']: testcase for testcase in spec['testcases']}
+    testcase_lookup = {}
+    tc_script_lookup = {}
     for script in spec.get('scripts', ()):
         for testcase in script.get('testcases', ()):
             id_ = testcase['id']
             if id_ in testcase_lookup:
                 raise RuntimeError(f"duplicate testcase {id_}")
-            testcase_lookup[id_] = (script, testcase)
+            testcase_lookup[id_] = testcase
+            tc_script_lookup[id_] = script
     module_lookup = {module['id']: module for module in spec['modules']}
     version_lookup = {version['version']: version for version in spec['versions']}
     # step 2. check for duplicates:
@@ -76,6 +78,7 @@ def _resolve_spec(spec: dict):
     spec['versions'] = version_lookup
     # step 3a. add testcase lookup
     spec['testcases'] = testcase_lookup
+    spec['tc_scripts'] = tc_script_lookup
     # step 4. resolve references
     # step 4a. resolve references to modules in includes
     # in this step, we also normalize the include form
@@ -172,7 +175,9 @@ def eval_buckets(results, testcase_ids) -> dict:
     """
     by_value = defaultdict(list)
     for testcase_id in testcase_ids:
-        value = results.get(testcase_id, {}).get('result')
+        value = results.get(testcase_id, {})
+        if isinstance(value, dict):
+            value = value.get('result')
         by_value[value].append(testcase_id)
     return by_value
 

compliance-monitor/monitor.py

@@ -244,9 +244,7 @@ class PrecomputedVersion:
     def __init__(self, version, testcases):
         self.name = version['version']
         self.validity = version['validity']
-        self.listed = bool(version['_explicit_validity'])
         self.targets = version['targets']
-        self.testcases = testcases  # TODO remove (see evaluate)
 
     def evaluate(self, scope_results):
         """evaluate the results for this version and return the canonical JSON output"""
@@ -257,8 +255,6 @@ def evaluate(self, scope_results):
                 'result': evaluate(scope_results, tc_ids),
             }
         return {
-            'testcases': self.testcases,  # FIXME move towards scope eval dict
-            'results': scope_results,  # FIXME move towards scope eval dict
             'result': target_results['main']['result'],
             'targets': target_results,
             'validity': self.validity,
@@ -270,7 +266,7 @@ class PrecomputedScope:
     def __init__(self, spec):
         self.name = spec['name']
         self.spec = spec
-        self.testcases = {tc_id: item[1] for tc_id, item in spec['testcases'].items()}
+        self.testcases = spec['testcases']
         self.versions = {
             version['version']: PrecomputedVersion(version, self.testcases)
             for version in spec['versions'].values()
@@ -299,6 +295,8 @@ def evaluate(self, scope_results, include_drafts=False):
         passed = [vname for vname in relevant if version_results[vname]['result'] == 1]
         return {
             'name': self.name,
+            'testcases': self.testcases,
+            'results': scope_results,
             'versions': version_results,
             'relevant': relevant,
             'passed': passed,
@@ -312,18 +310,18 @@ def evaluate(self, scope_results, include_drafts=False):
     def update_lookup(self, target_dict):
         """Create entries in a lookup mapping for each testcase that occurs in this scope.
 
-        This mapping from triples (scope uuid, version name, testcase id) to testcase facilitates
+        This mapping from pairs (scope uuid, testcase id) to testcase facilitates
         evaluating result sets from database queries a great deal, because then just one lookup operation
         tells us whether a result row can be associated with any known testcase, and if so, whether the
         result is still valid (looking at the testcase's lifetime).
 
-        In the future, the mapping could even be simplified by deriving a unique id from each triple that
+        In the future, the mapping could even be simplified by deriving a unique id from each pair that
         could then be stored (redundantly) in a dedicated database column, and the mapping could be from
-        just one id (instead of a triple) to testcase.
+        just one id (instead of a pair) to testcase.
         """
         scope_uuid = self.spec['uuid']
         for tc_id, testcase in self.testcases.items():
-            target_dict[(scope_uuid, '*', tc_id)] = testcase
+            target_dict[(scope_uuid, tc_id)] = testcase
 
 
 def import_cert_yaml(yaml_path, target_dict):
@@ -484,6 +482,10 @@ async def post_report(
             except UniqueViolation:
                 raise HTTPException(status_code=409, detail="Conflict: report already present")
             if 'versions' not in document:
+                # If this key is missing, this means we have a newer-style report that doesn't redundantly list
+                # results per version. One reason for this change is that the meaning of a testcase identifier
+                # no longer depends on the scope version, and we can quite simply read off the results from the
+                # invocations. -- Use the dummy version '*' as long as the db schema still expects a version.
                 document['versions'] = {'*': {
                     tc_id: {'result': result, 'invocation': inv_id}
                     for inv_id, invocation in document['run']['invocations'].items()
@@ -507,13 +509,13 @@ def convert_result_rows_to_dict2(
     # collect result per subject/scope/version
     preliminary = defaultdict(lambda: defaultdict(dict))  # subject -> scope
     missing = set()
-    for subject, scope_uuid, version, testcase_id, result, checked_at, report_uuid in rows:
-        testcase = scopes_lookup.get((scope_uuid, version, testcase_id))
+    for subject, scope_uuid, _, testcase_id, result, checked_at, report_uuid in rows:
+        testcase = scopes_lookup.get((scope_uuid, testcase_id))
         if not testcase:
             # it can be False (testcase is known but version too old) or None (testcase not known)
             # only report the latter case
             if testcase is None:
-                missing.add((scope_uuid, version, testcase_id))
+                missing.add((scope_uuid, testcase_id))
             continue
         # drop value if too old
         lifetime = testcase.get('lifetime')  # leave None if not present; to be handled by add_period

compliance-monitor/templates/details.md.j2

@@ -28,8 +28,8 @@ No recent test results available.
 | testcase id | result | description |
 |---|---|---|
 {% for testcase_id in target_result.testcases -%}
-{% set testcase = version_result.testcases[testcase_id] -%}
-{% set res = version_result.results[testcase_id] if testcase_id in version_result.results else dict(result=0) -%}
+{% set testcase = scope_result.testcases[testcase_id] -%}
+{% set res = scope_result.results[testcase_id] if testcase_id in scope_result.results else dict(result=0) -%}
 | {% if res.result != 1 %}⚠️ {% endif %}{{ testcase.id }} |
 {#- #} {% if res.report -%}
 [{{ res.result | verdict_check }}]({{ report_url(res.report, version, testcase_id) }})

compliance-monitor/templates/report.md.j2

@@ -4,34 +4,10 @@
 - subject: {{ report.subject }}
 - scope: [{{ report.spec.name }}]({{ scope_url(report.spec.uuid) }})
 - checked at: {{ report.checked_at }}
-
-## Results
-
-{# % if report.versions %}{% for version, version_results in report.versions.items() %}{% if version_results %}
-### {{ version }}
-
-| test case | result | invocation |
-|---|---|---|
-{% for testcase_id, result_data in version_results.items() -%}
-| {{ testcase_id }} {: #{{ version + '_' + testcase_id }} } | {{ result_data.result | verdict_check }} | [{{ result_data.invocation }}](#{{ result_data.invocation }}) |
-{% endfor %}
-{% endif %}
-{% endfor %}{% endif % #}
-
-## Run
-
-### Variable assignment
-
-| key | value |
-|---|---|
-{% for key, value in report.run.assignment.items() -%}
-| `{{ key }}` | `{{ value }}` |
-{% endfor %}
-
-### Check tool invocations
+- variable assignment: {% set comma = joiner(", ") %}{% for key, value in report.run.assignment.items() -%}{{comma()}}`{{ key }}`=`{{ value }}`{% endfor %}
 
 {% for invid, invdata in report.run.invocations.items() %}
-#### Invocation {{invid}} {: #{{ invid }} }
+## Invocation {{invid}} {: #{{ invid }} }
 
 - cmd: `{{ invdata.cmd }}`
 - rc: {{ invdata.rc }}