feat(ISV-7124): Fix static-checks loading and add error handling

[RichardPlesnik]

Merge Request Checklists

• Development is done in feature branches
• Code changes are submitted as pull request into a primary branch [Provide reason for non-primary branch submissions]
• Code changes are covered with unit and integration tests.
• Code passes all automated code tests:
  • Linting
  • Code formatter - Black
  • Security scanners
  • Unit tests
  • Integration tests
• Code is reviewed by at least 1 team member
• Pull request is tagged with "risk/good-to-go" label for minor changes

Changes were tested here:
RichardPlesnik/community-operators-pipeline-preprod#11

• first test run shows error handling before dependency fix
• second test run shows that common tests are launched and working correctly (operator with missing name fails)

[qodo-code-review]

Review Summary by Qodo

Fix static-checks loading with error handling and jsonschema dependency

🐞 Bug fix ✨ Enhancement 🧪 Tests

Walkthroughs

Description

• Add comprehensive error handling for static-checks module loading
  - Distinguish between missing modules and missing dependencies
  - Raise RuntimeError for missing dependencies instead of silently failing
• Add jsonschema to production dependencies to resolve import failures
• Add detailed debug logging for checks loading process
• Add test coverage for missing dependency and unexpected error scenarios

Diagram

flowchart LR
  A["Module Loading"] --> B["Error Detection"]
  B --> C["Missing Module?"]
  C -->|Yes| D["Log & Continue"]
  C -->|No| E["Missing Dependency"]
  E --> F["Raise RuntimeError"]
  B --> G["Other Exceptions"]
  G --> F
  H["Add jsonschema Dependency"] --> I["Resolve Import Failures"]

Loading

File Changes

1. operatorcert/operator_repo/checks/__init__.py Error handling +31/-2

Add error handling and logging to checks loading

• Enhanced get_checks() function with granular error handling for module imports
• Distinguish between missing module files and missing import dependencies
• Raise RuntimeError for missing dependencies instead of silently ignoring them
• Add debug logging for module loading process and loaded check counts
• Add traceback printing for debugging import failures

operatorcert/operator_repo/checks/init.py

---

2. tests/operator_repo/checks/test_checks.py 🧪 Tests +38/-1

Add tests for error handling scenarios

• Update test_get_checks_missing_modules() to use proper ModuleNotFoundError message format
• Add test_get_checks_missing_dependency() to verify RuntimeError on missing dependencies
• Add test_get_checks_unexpected_error() to verify RuntimeError on unexpected exceptions
• Ensure tests validate error messages contain relevant details

tests/operator_repo/checks/test_checks.py

---

3. pyproject.toml Dependencies +1/-0

Add jsonschema to production dependencies

• Add jsonschema (>=4.0.0) to production dependencies
• Resolves ModuleNotFoundError when checks modules import jsonschema

pyproject.toml

---

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (2)   📘 Rule violations (0)   📎 Requirement gaps (0)

🐞\ ≡ Correctness (1) ◔ Observability (1)

1. Brittle import error parsing 🐞 ≡

Description

In get_checks(), ModuleNotFoundError is classified by substring-matching str(e), which can wrongly
treat non-“module missing” import failures as safe-to-ignore and return an empty check set. This can
mask suite misconfiguration because callers (e.g., the CLI) accept arbitrary suite strings and will
then silently list/run no checks.

Code

operatorcert/operator_repo/checks/init.py[R129-134]

+            module_path = f"{suite_name}.{module_name}"
+            error_message = str(e)
+            if f"No module named '{module_path}'" in error_message:
+                # Nonexistent module
+                log.debug("Module %s not found: %s", module_path, e)
+            else:

Relevance

⭐⭐⭐ High

Substring-matching str(e) is brittle; ModuleNotFoundError has structured fields (e.name) to
disambiguate missing module vs dependency.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

get_checks() decides whether to ignore ModuleNotFoundError by checking whether the exception message
contains the full “No module named '<suite>.<type>'” substring; this is a brittle heuristic and is
not a strict equality check. The CLI accepts a free-form suite string and passes it directly into
get_checks()/run_suite, so a misclassified import error can lead to silently returning no checks
rather than surfacing a suite loading problem; the tests also hard-code the message format,
reinforcing the brittle assumption.

operatorcert/operator_repo/checks/init.py[97-147]
operatorcert/operator_repo/cli.py[189-200]
operatorcert/operator_repo/cli.py[250-260]
tests/operator_repo/checks/test_checks.py[67-81]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`get_checks()` uses substring matching on `str(ModuleNotFoundError)` to decide whether a missing import should be ignored (module file absent) or should fail (missing dependency). This heuristic can misclassify some import failures and silently return an empty checks set.

### Issue Context
`get_checks()` is used via CLI/entrypoints with user-provided suite strings, so suite-loading errors should be classified robustly.

### Fix Focus Areas
- operatorcert/operator_repo/checks/__init__.py[124-139]

### Suggested fix
- Prefer `ModuleNotFoundError.name` (and/or `e.msg`) over parsing `str(e)`.
- Treat “module missing” as ignorable only when the missing name exactly equals the attempted module path **and** the message indicates a plain missing module (not additional context like “is not a package”).
- Optionally, use `importlib.util.find_spec(module_path)` to check module existence before importing, and only ignore when `find_spec` is `None` for the leaf module.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Direct traceback printing 🐞 ◔

Description

get_checks() calls traceback.print_exc() and logs only at debug before raising RuntimeError,
bypassing the configured logger and potentially duplicating stack traces upstream. This makes
failures noisy/uncontrolled and inconsistent with the rest of the module’s error reporting.

Code

operatorcert/operator_repo/checks/init.py[R136-147]

+                error_msg = f"Failed to import {module_path} due to error: {e}"
+                log.debug(error_msg)
+                traceback.print_exc()
+                raise RuntimeError(error_msg) from e
+        except Exception as e:
+            # Unexpected error - SHOULD FAIL
+            error_msg = (
+                f"Unexpected error loading module {suite_name}.{module_name}: {e}"
+            )
+            log.debug(error_msg)
+            traceback.print_exc()
+            raise RuntimeError(error_msg) from e

Relevance

⭐⭐⭐ High

traceback.print_exc() bypasses logging; log.exception would integrate with configured handlers and
avoid duplicate/uncontrolled stderr output.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new error-handling path prints tracebacks directly to stderr via traceback.print_exc() for both
missing-dependency and unexpected errors. Elsewhere in the same module, exception reporting is done
via log.exception(...), indicating a project convention of logger-based stack traces rather than
raw printing.

operatorcert/operator_repo/checks/init.py[124-147]
operatorcert/operator_repo/checks/init.py[157-175]
operatorcert/operator_repo/checks/init.py[136-147]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`get_checks()` prints stack traces using `traceback.print_exc()` and logs the error at `debug` before raising. This bypasses logging configuration and can cause duplicated/noisy tracebacks.

### Issue Context
The module already uses `log.exception(...)` in `run_check()` for consistent exception reporting.

### Fix Focus Areas
- operatorcert/operator_repo/checks/__init__.py[136-147]

### Suggested fix
- Remove both `traceback.print_exc()` calls.
- Replace the `log.debug(error_msg)` call in those exception branches with `log.exception(error_msg)` (or `log.error(..., exc_info=True)`) so stack traces flow through the configured logger.
- Keep `raise RuntimeError(error_msg) from e` to preserve exception chaining.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

ⓘ The new review experience is currently in Beta. Learn more

[tomasbakk]

I had a look at the test PR mentioned here and the extension of a module itself and they both look good to me 👍

[tomasbakk]

I forgot to click on approve after I added my comment.