Repository navigation
fix: fail early in forklift_pods_state when MTV operator is not installed - #355
Conversation
…lled - Add pre-check for MTV operator Subscription before the 5-minute TimeoutSampler loop - Add MtvOperatorNotInstalledError custom exception - Extract MTV_OPERATOR_NAME constant to utilities/constants.py - Fix pre-existing type annotation (controller_pod: str -> Pod) Closes #354
Review Summary by QodoFail early when MTV operator is not installed
WalkthroughsDescription• Add early validation for MTV operator Subscription in forklift_pods_state fixture - Raises MtvOperatorNotInstalledError immediately instead of timing out after 5 minutes - Provides actionable error message with namespace information • Extract MTV_OPERATOR_NAME constant to eliminate string duplication - Centralize "mtv-operator" string in utilities/constants.py - Update references in conftest.py, must_gather.py, and utils.py • Fix type annotation for controller_pod from str | None to Pod | None Diagramflowchart LR
A["forklift_pods_state fixture"] -->|check MTV Subscription| B["MTV_OPERATOR_NAME constant"]
B -->|exists?| C{Subscription found}
C -->|No| D["MtvOperatorNotInstalledError"]
C -->|Yes| E["Continue with pod checks"]
D -->|actionable message| F["Fail immediately"]
E -->|normal flow| G["Monitor pod state"]
File Changes1. conftest.py
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdd an early MTV Subscription existence check to the Changes
Sequence Diagram(s)sequenceDiagram
participant Pytest as Pytest (forklift_pods_state)
participant K8s as Kubernetes API
participant Poller as Pod Poller (TimeoutSampler)
Pytest->>K8s: GET Subscription in mtv_namespace (MTV_OPERATOR_NAME)
alt Subscription not found
K8s-->>Pytest: 404 / not found
Pytest-->>Pytest: raise MtvOperatorNotInstalledError(namespace)
else Subscription exists
K8s-->>Pytest: Subscription resource
Pytest->>Poller: start polling pods in mtv_namespace
Poller->>K8s: GET Pods (label selector)
K8s-->>Poller: Pod list
alt controller pod found
Poller-->>Pytest: controller pod running
else timeout/no pod
Poller-->>Pytest: raise ForkliftPodsNotRunningError
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
Code Review by Qodo
1.
|
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
|
/cherry-pick v2.11 v2.10 |
|
Cherry-pick requested for PR: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@exceptions/exceptions.py`:
- Around line 12-17: Update the docstring for the __init__ method of
MtvOperatorNotInstalledError to include Returns and Raises sections in addition
to Args: keep the existing Args entry for namespace: str, add a Returns section
that documents the return type (None) and a Raises section that documents any
exceptions this initializer may raise (e.g., ValueError or TypeError if you
validate namespace) or state "None" if it never raises; modify the __init__
docstring in the MtvOperatorNotInstalledError class accordingly to match the
repo standard.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: d95dc938-8c8b-4fc1-a0b5-4de4de66cd8d
📒 Files selected for processing (5)
conftest.pyexceptions/exceptions.pyutilities/constants.pyutilities/must_gather.pyutilities/utils.py
|
Persistent review updated to latest commit 8fb6f22 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLAUDE.md`:
- Line 237: Update the docstring rule to remove the contradiction by changing
the hard requirement to "Include Args, Returns, and Raises sections as
applicable" and explicitly state the two exceptions: omit Returns for __init__
methods (they always return None) and omit Raises when the function raises no
exceptions; ensure the rule references the section names "Args", "Returns", and
"Raises" and replaces any current wording that says "must" include all sections
to avoid forcing placeholder entries like "Returns: None".
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: 15c1a8cd-c4f7-4d6b-ad4c-f5d30798a8c1
📒 Files selected for processing (1)
CLAUDE.md
|
Persistent review updated to latest commit 35bee2c |
|
/verifed |
|
/verified |
…lled (#355) * fix: fail early in forklift_pods_state when MTV operator is not installed - Add pre-check for MTV operator Subscription before the 5-minute TimeoutSampler loop - Add MtvOperatorNotInstalledError custom exception - Extract MTV_OPERATOR_NAME constant to utilities/constants.py - Fix pre-existing type annotation (controller_pod: str -> Pod) Closes #354 * docs: clarify docstring rule - omit Returns for __init__ and Raises when none * fix: handle RBAC permissions gracefully in forklift_pods_state Subscription check - Wrap Subscription.exists check in try/except ForbiddenError to fall back to pod-based readiness check when RBAC permissions are insufficient - Clarify CLAUDE.md docstring rule to use "sections as applicable" wording * docs: allow kubernetes.dynamic.exceptions imports in CLAUDE.md rules * fix: catch NotFoundError in Subscription pre-check to provide actionable error
|
Cherry-picked PR fix: fail early in forklift_pods_state when MTV operator is not installed into v2.11: #356 |
…lled (#355) * fix: fail early in forklift_pods_state when MTV operator is not installed - Add pre-check for MTV operator Subscription before the 5-minute TimeoutSampler loop - Add MtvOperatorNotInstalledError custom exception - Extract MTV_OPERATOR_NAME constant to utilities/constants.py - Fix pre-existing type annotation (controller_pod: str -> Pod) Closes #354 * docs: clarify docstring rule - omit Returns for __init__ and Raises when none * fix: handle RBAC permissions gracefully in forklift_pods_state Subscription check - Wrap Subscription.exists check in try/except ForbiddenError to fall back to pod-based readiness check when RBAC permissions are insufficient - Clarify CLAUDE.md docstring rule to use "sections as applicable" wording * docs: allow kubernetes.dynamic.exceptions imports in CLAUDE.md rules * fix: catch NotFoundError in Subscription pre-check to provide actionable error
|
Cherry-picked PR fix: fail early in forklift_pods_state when MTV operator is not installed into v2.10: #357 |
…lled (#355) (#356) * fix: fail early in forklift_pods_state when MTV operator is not installed - Add pre-check for MTV operator Subscription before the 5-minute TimeoutSampler loop - Add MtvOperatorNotInstalledError custom exception - Extract MTV_OPERATOR_NAME constant to utilities/constants.py - Fix pre-existing type annotation (controller_pod: str -> Pod) Closes #354 * docs: clarify docstring rule - omit Returns for __init__ and Raises when none * fix: handle RBAC permissions gracefully in forklift_pods_state Subscription check - Wrap Subscription.exists check in try/except ForbiddenError to fall back to pod-based readiness check when RBAC permissions are insufficient - Clarify CLAUDE.md docstring rule to use "sections as applicable" wording * docs: allow kubernetes.dynamic.exceptions imports in CLAUDE.md rules * fix: catch NotFoundError in Subscription pre-check to provide actionable error Co-authored-by: Meni Yakove <441263+myakove@users.noreply.github.com>
…lled (#355) (#357) * fix: fail early in forklift_pods_state when MTV operator is not installed - Add pre-check for MTV operator Subscription before the 5-minute TimeoutSampler loop - Add MtvOperatorNotInstalledError custom exception - Extract MTV_OPERATOR_NAME constant to utilities/constants.py - Fix pre-existing type annotation (controller_pod: str -> Pod) Closes #354 * docs: clarify docstring rule - omit Returns for __init__ and Raises when none * fix: handle RBAC permissions gracefully in forklift_pods_state Subscription check - Wrap Subscription.exists check in try/except ForbiddenError to fall back to pod-based readiness check when RBAC permissions are insufficient - Clarify CLAUDE.md docstring rule to use "sections as applicable" wording * docs: allow kubernetes.dynamic.exceptions imports in CLAUDE.md rules * fix: catch NotFoundError in Subscription pre-check to provide actionable error Co-authored-by: Meni Yakove <441263+myakove@users.noreply.github.com>
|
New container for ghcr.io/redhatqe/mtv-api-tests:latest published |
Summary
forklift_pods_statefixture before the 5-minuteTimeoutSamplerloop. When the Subscription doesn't exist, raisesMtvOperatorNotInstalledErrorimmediately with an actionable error messageMTV_OPERATOR_NAMEconstant toutilities/constants.pyto eliminate hardcoded"mtv-operator"string duplication acrossconftest.py,utilities/must_gather.py, andutilities/utils.pycontroller_pod: str | None->Pod | NoneContext
Jenkins job
mtv-2.12-ocp-4.21-test-release-non-gate #8had all 35 tests across 3 child jobs wait 5 minutes each before failing with an unhelpful timeout error, because MTV was not installed on the cluster despite the deploy job reporting SUCCESS.Closes #354
Test plan
forklift_pods_stateraisesMtvOperatorNotInstalledErrorimmediately when MTV Subscription is missingSummary by CodeRabbit
Tests
Refactor
Documentation