feat(PDRIVE-687): add mypy-based pre-commit hook for type checking - #131
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request adds mypy tooling, separates single-resource and multi-resource Kubernetes API selection, updates callers and tests, and improves type annotations and structured error handling. ChangesMypy tooling and type contracts
Resource selection and runtime updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ValidationCollector
participant OcApiUtils
participant KubernetesAPI
ValidationCollector->>OcApiUtils: select_single_resource(resource_type)
OcApiUtils->>KubernetesAPI: object(ignore_not_found=True)
KubernetesAPI-->>OcApiUtils: resource or None
OcApiUtils-->>ValidationCollector: resource or None
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/in_cluster_checks/rules/network/ovnk8s_validations.py (1)
35-46: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPropagate API failures instead of marking the prerequisite as unmet.
select_single_resource()can raise when theocquery fails. The broadexcept Exceptioncatches that failure and returnsPrerequisiteResult.not_met(...). This reports an API outage as an inapplicable rule and drops the original failure context. Catch only explicitly expected conditions, or let command failures propagate.As per path instructions, unexpected command failures must propagate so the framework can report them as
SKIPwith context.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/in_cluster_checks/rules/network/ovnk8s_validations.py` around lines 35 - 46, Update the validation method containing the network_obj lookup and network_type check so unexpected failures from select_single_resource() propagate instead of being converted to PrerequisiteResult.not_met(). Remove the broad Exception handling or narrow it to explicitly expected conditions, while preserving the existing not-met results for a missing resource and non-OVNKubernetes network type.Source: Path instructions
🧹 Nitpick comments (4)
tests/linters/check_mypy.py (1)
13-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnnotate
mainwith its return type.Use
def main() -> None:. The function has no value return.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/linters/check_mypy.py` around lines 13 - 14, Update the main function signature to explicitly declare a None return type, preserving its existing behavior and implementation.Source: Coding guidelines
src/in_cluster_checks/runner.py (1)
64-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGive
node_executorsa concrete mapping type.
dict | Noneaccepts arbitrary key and value types. It does not let Mypy validate the mapping passed todomain.verify()at Line 207. Ifbuild_host_executors()returnsdict[str, NodeExecutor], use that type for the attribute.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/in_cluster_checks/runner.py` around lines 64 - 65, Update the node_executors attribute declaration in the runner class to use the concrete type dict[str, NodeExecutor] | None, matching build_host_executors() and enabling type checking when passed to domain.verify().Source: Coding guidelines
pyproject.toml (1)
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the Mypy lower bound with Python 3.12.
mypy.inisetspython_version = 3.12, butmypy>=1.0.0permits older releases. Verify the first Mypy release that supports this target. Raise the lower bound or pin the development environment to a compatible version.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` at line 37, Update the Mypy dependency entry in pyproject.toml to require the first release compatible with the Python 3.12 target configured by mypy.ini, rather than allowing older incompatible versions; preserve the existing development dependency format.src/in_cluster_checks/core/printer.py (1)
314-320: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the docstrings with the list contract.
At Line 314 and Line 388, the annotations use
List[Dict[str, Any]]. The docstrings still describe a dictionary and show anin_cluster_ruleswrapper.InClusterCheckRunner.runpasses the list directly to the output methods at Lines 214-221. Update the argument and return documentation to show a list of report dictionaries.Suggested documentation update
- results: Dictionary with rule results in Insights format + results: List of report dictionaries in Insights format - Formatted results dictionary: + Formatted list of report dictionaries:Also applies to: 388-416
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/in_cluster_checks/core/printer.py` around lines 314 - 320, Update the docstrings for print_to_json and the corresponding output method near line 388 to match their List[Dict[str, Any]] contracts: describe results as a list of report dictionaries and remove references to a dictionary or in_cluster_rules wrapper. Ensure any return documentation also reflects the list-based report output, without changing implementation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/in_cluster_checks/core/operations.py`:
- Around line 474-475: Update the code around select_single_resource to handle a
None network_obj before accessing model.spec.defaultNetwork.type. Return an
appropriate absent-resource result or explicitly enforce that the resource
exists, while preserving the current type lookup when present.
In `@src/in_cluster_checks/rules/k8s/k8s_validations.py`:
- Around line 253-258: Update the UnExpectedSystemOutput construction in the pod
timestamp parsing error path to populate ip with the actual executor address
from self.get_host_ip() instead of an empty string, while preserving the
existing command, output, message, and exception chaining.
In `@src/in_cluster_checks/rules/network/dns_validations.py`:
- Line 18: Restore the ClassVar import and annotate the mutable class attributes
objective_hosts, supported_profiles, and links with their existing collection
types using ClassVar, resolving Ruff RUF012 while preserving their current
values and behavior.
- Around line 46-51: Update the failure handling around run_oc_command and
UnExpectedSystemOutput to preserve both returned streams: combine
dns_config_output with stderr and pass the combined raw output in the exception
context, while retaining the existing command, host, and message details.
In `@tests/linters/check_mypy.py`:
- Around line 19-33: Update the Mypy subprocess handling around subprocess.run
and the errors filtering loop so it fails closed: distinguish diagnostics
intentionally excluded by the existing SafeCmdString rule from absent or
unrecognized stdout/stderr output, configuration or CLI failures, and other
non-zero return codes. When the result is not a clean run with all diagnostics
intentionally filtered, print stderr and propagate a non-zero exit status;
preserve the current success behavior only when no errors occurred or every
detected Mypy diagnostic was explicitly excluded.
In `@tests/rules/network/test_ovs_validations.py`:
- Around line 180-181: Remove the class-level mutation of
DataCollectorScenarioParams in the scenarios setup: do not assign
tested_object_mock_dict on shared scenarios[0] and scenarios[1] objects.
Construct each scenario, including its oc_api.select_single_resource Mock,
within test setup or otherwise create fresh per-test scenario instances so no
mutable mock state is shared across parametrized tests.
---
Outside diff comments:
In `@src/in_cluster_checks/rules/network/ovnk8s_validations.py`:
- Around line 35-46: Update the validation method containing the network_obj
lookup and network_type check so unexpected failures from
select_single_resource() propagate instead of being converted to
PrerequisiteResult.not_met(). Remove the broad Exception handling or narrow it
to explicitly expected conditions, while preserving the existing not-met results
for a missing resource and non-OVNKubernetes network type.
---
Nitpick comments:
In `@pyproject.toml`:
- Line 37: Update the Mypy dependency entry in pyproject.toml to require the
first release compatible with the Python 3.12 target configured by mypy.ini,
rather than allowing older incompatible versions; preserve the existing
development dependency format.
In `@src/in_cluster_checks/core/printer.py`:
- Around line 314-320: Update the docstrings for print_to_json and the
corresponding output method near line 388 to match their List[Dict[str, Any]]
contracts: describe results as a list of report dictionaries and remove
references to a dictionary or in_cluster_rules wrapper. Ensure any return
documentation also reflects the list-based report output, without changing
implementation behavior.
In `@src/in_cluster_checks/runner.py`:
- Around line 64-65: Update the node_executors attribute declaration in the
runner class to use the concrete type dict[str, NodeExecutor] | None, matching
build_host_executors() and enabling type checking when passed to
domain.verify().
In `@tests/linters/check_mypy.py`:
- Around line 13-14: Update the main function signature to explicitly declare a
None return type, preserving its existing behavior and implementation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e8d628e-1065-4968-b266-47716aa262d8
📒 Files selected for processing (24)
.pre-commit-config.yamlmypy.inipyproject.tomlsrc/in_cluster_checks/core/data_collector_runner.pysrc/in_cluster_checks/core/operations.pysrc/in_cluster_checks/core/printer.pysrc/in_cluster_checks/core/rule.pysrc/in_cluster_checks/rules/hw_fw_details/hw_fw_base.pysrc/in_cluster_checks/rules/k8s/k8s_validations.pysrc/in_cluster_checks/rules/network/dns_validations.pysrc/in_cluster_checks/rules/network/nmstate_validations.pysrc/in_cluster_checks/rules/network/ovnk8s_validations.pysrc/in_cluster_checks/rules/network/ovs_base.pysrc/in_cluster_checks/rules/resources_utilization/resources_utilization.pysrc/in_cluster_checks/rules/storage/storage_validations.pysrc/in_cluster_checks/runner.pysrc/in_cluster_checks/utils/oc_api_utils.pysrc/in_cluster_checks/utils/parsing_utils.pytests/linters/check_mypy.pytests/rules/k8s/test_k8s_validations.pytests/rules/network/test_ovnk8s_validations.pytests/rules/network/test_ovs_validations.pytests/rules/storage/test_storage_validations.pytests/unit/utils/test_oc_api_utils.py
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
85bd270 to
29852e0
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/in_cluster_checks/rules/k8s/k8s_validations.py (2)
206-219: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDo not report
PASSEDafter an incomplete pod scan.When a namespace query fails,
continueremoves that namespace from validation. When a namespace exceeds the limit, slicing removes the remaining pods.run_rule()can then returnRuleResult.passed()even though infrastructure pods were not checked.Raise
UnExpectedSystemOutputfor required query failures. For truncated results, propagate an incomplete-scan state and returnwarningorskip, or paginate through all pods. Replaceself.loggerwith framework-managed result handling.As per coding guidelines, rules must not use
self.logger.
As per path instructions, required query failures must becomeUnExpectedSystemOutput.
Based on learnings, required query failures must propagate instead of being silently skipped.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/in_cluster_checks/rules/k8s/k8s_validations.py` around lines 206 - 219, Update the pod validation flow in run_rule() to propagate required namespace query failures as UnExpectedSystemOutput instead of continuing, and replace self.logger usage with framework-managed result handling. Do not silently discard pods when pod_objects exceeds MAX_PODS_PER_NAMESPACE: either paginate all results or carry an incomplete-scan state that prevents RuleResult.passed() and returns warning or skip.Sources: Coding guidelines, Path instructions, Learnings
534-539: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPropagate malformed namespace timestamps.
If
deletionTimestampis malformed, this fallback classifies the namespace as not recently terminating and returns a warning. RaiseUnExpectedSystemOutputwith the namespace, raw timestamp, and command context instead of returningFalse, consistent with_is_old_pod().As per path instructions, unexpected system output must propagate with diagnostic context instead of using a fallback value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/in_cluster_checks/rules/k8s/k8s_validations.py` around lines 534 - 539, Update the namespace termination timestamp handling in the shown validation method: when parsing deletionTimestamp raises ValueError or AttributeError, raise UnExpectedSystemOutput instead of returning False. Include the namespace, raw deletion timestamp, and command context in the exception, matching the diagnostic propagation behavior of _is_old_pod().Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/in_cluster_checks/utils/oc_api_utils.py`:
- Around line 394-418: Restore field_selector to select_resources and forward it
to _log_and_build_selector_kwargs using explicit keyword arguments, ensuring
all_namespaces is passed to the correct parameter. Update the sibling call in
src/in_cluster_checks/utils/oc_api_utils.py lines 433-457 similarly; both
affected sites require changes.
---
Outside diff comments:
In `@src/in_cluster_checks/rules/k8s/k8s_validations.py`:
- Around line 206-219: Update the pod validation flow in run_rule() to propagate
required namespace query failures as UnExpectedSystemOutput instead of
continuing, and replace self.logger usage with framework-managed result
handling. Do not silently discard pods when pod_objects exceeds
MAX_PODS_PER_NAMESPACE: either paginate all results or carry an incomplete-scan
state that prevents RuleResult.passed() and returns warning or skip.
- Around line 534-539: Update the namespace termination timestamp handling in
the shown validation method: when parsing deletionTimestamp raises ValueError or
AttributeError, raise UnExpectedSystemOutput instead of returning False. Include
the namespace, raw deletion timestamp, and command context in the exception,
matching the diagnostic propagation behavior of _is_old_pod().
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c38aefa2-7eb1-4225-ae42-f9ef9b44fe96
📒 Files selected for processing (8)
.claude/skills/new-rule/SKILL.mdpyproject.tomlsrc/in_cluster_checks/rules/k8s/k8s_validations.pysrc/in_cluster_checks/rules/network/ovnk8s_validations.pysrc/in_cluster_checks/utils/oc_api_utils.pytests/rules/k8s/test_k8s_validations.pytests/rules/network/test_ovnk8s_validations.pytests/unit/utils/test_oc_api_utils.py
🚧 Files skipped from review as they are similar to previous changes (6)
- .claude/skills/new-rule/SKILL.md
- src/in_cluster_checks/rules/network/ovnk8s_validations.py
- tests/unit/utils/test_oc_api_utils.py
- pyproject.toml
- tests/rules/network/test_ovnk8s_validations.py
- tests/rules/k8s/test_k8s_validations.py
|
Regarding the two "outside diff range" comments from the review summary: 1. The More importantly, Raising This differs from 2. The |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #131 +/- ##
=======================================
Coverage ? 86.48%
=======================================
Files ? 57
Lines ? 6606
Branches ? 0
=======================================
Hits ? 5713
Misses ? 893
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/in_cluster_checks/utils/oc_api_utils.py`:
- Around line 350-357: The selector-related annotations in
_log_and_build_selector_kwargs should be fully parameterized: use
list[oc.APIObject] (or the project’s documented API-object type) for selected
resources and specify Dict[str, str] | None for label_selector and
field_selector. Preserve the existing selector behavior while replacing bare
collection/dictionary types.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c9c95c74-e1f3-4040-b5d0-bae49a81e064
📒 Files selected for processing (2)
pyproject.tomlsrc/in_cluster_checks/utils/oc_api_utils.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
8532019 to
2867acb
Compare
Add a pre-commit hook that runs mypy to catch type errors at commit time. Reports all mypy errors, with [assignment] errors filtered to SafeCmdString violations only. - Add mypy.ini configuration for type checking - Add check_mypy.py wrapper with SafeCmdString-specific filtering - Add safecmdstring-mypy-check hook to .pre-commit-config.yaml - Add mypy to dev dependencies Assisted-by: Claude Code (Claude Opus 4.6) <noreply@anthropic.com>
3 participants
Add a pre-commit hook that runs mypy to catch type errors at commit time. Reports all mypy errors, with [assignment] errors filtered to SafeCmdString violations only.
Assisted-by: Claude Code (Claude Opus 4.6) noreply@anthropic.com
Summary by CodeRabbit
Bug Fixes
Improvements
Quality