Skip to content

fix: cluster sanity does not need to execute when operator or cluster health is run#1184

Merged
sheltoncyril merged 1 commit intoopendatahub-io:mainfrom
dbasunag:cluster_sanity
Mar 9, 2026
Merged

fix: cluster sanity does not need to execute when operator or cluster health is run#1184
sheltoncyril merged 1 commit intoopendatahub-io:mainfrom
dbasunag:cluster_sanity

Conversation

@dbasunag
Copy link
Copy Markdown
Collaborator

@dbasunag dbasunag commented Mar 6, 2026

Pull Request

Summary

No need to run cluster sanity when we are running operator or cluster sanity - they are the same exact checks.

Related Issues

How it has been tested

  • Locally
  • Jenkins

Additional Requirements

  • If this PR introduces a new test image, did you create a PR to mirror it in disconnected environment?
  • If this PR introduces new marker(s)/adds a new component, was relevant ticket created to update relevant Jenkins job?

Summary by CodeRabbit

  • Chores
    • Test configuration updated to conditionally skip cluster sanity checks when tests are run with the cluster_health or operator_health markers; an informational log is emitted and the sanity verification is bypassed in those cases.

@dbasunag dbasunag requested a review from a team as a code owner March 6, 2026 23:59
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 6, 2026

The following are automatically added/executed:

  • PR size label.
  • Run pre-commit
  • Run tox
  • Add PR author as the PR assignee
  • Build image based on the PR

Available user actions:

  • To mark a PR as WIP, add /wip in a comment. To remove it from the PR comment /wip cancel to the PR.
  • To block merging of a PR, add /hold in a comment. To un-block merging of PR comment /hold cancel.
  • To mark a PR as approved, add /lgtm in a comment. To remove, add /lgtm cancel.
    lgtm label removed on each new commit push.
  • To mark PR as verified comment /verified to the PR, to un-verify comment /verified cancel to the PR.
    verified label removed on each new commit push.
  • To Cherry-pick a merged PR /cherry-pick <target_branch_name> to the PR. If <target_branch_name> is valid,
    and the current PR is merged, a cherry-picked PR would be created and linked to the current PR.
  • To build and push image to quay, add /build-push-pr-image in a comment. This would create an image with tag
    pr-<pr_number> to quay repository. This image tag, however would be deleted on PR merge or close action.
Supported labels

{'/hold', '/lgtm', '/build-push-pr-image', '/wip', '/cherry-pick', '/verified'}

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 7, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 6035abb0-5fc3-4f11-b512-717511d6a5e8

📥 Commits

Reviewing files that changed from the base of the PR and between a6697f4 and c456c74.

📒 Files selected for processing (1)
  • tests/conftest.py

📝 Walkthrough

Walkthrough

Adds a guard in the cluster_sanity_scope_session fixture to collect session markers and return early (skipping verify_cluster_sanity) when any test is marked with cluster_health or operator_health.

Changes

Cohort / File(s) Summary
Test configuration
tests/conftest.py
Collects marker names from session items and, if cluster_health or operator_health is present, logs an info message and returns early to skip verify_cluster_sanity; otherwise calls verify_cluster_sanity as before.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Security findings (actionable)

  • Risk: Skipping cluster sanity based on markers can allow tests to run against unhealthy clusters if markers are misapplied or manipulated. Mitigation: ensure marker usage is strictly controlled, documented, and validated; consider asserting marker provenance or adding an explicit opt-in flag. Relevant CWE: CWE-20 (Improper Input Validation).
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: preventing cluster sanity execution when operator or cluster health checks are run, avoiding redundant verification.
Description check ✅ Passed The description includes all required template sections: summary explaining the rationale, related JIRA issue, testing confirmation (local only), and checklist items.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@tests/conftest.py`:
- Around line 680-683: Replace the fragile substring check of markexpr
(request.config.getoption(name="markexpr")) with an inspection of the collected
test items' actual markers: iterate request.session.items and for each test item
use item.get_closest_marker or item.iter_markers to detect presence of the
markers "cluster_health" or "operator_health"; if any collected item has those
markers, call LOGGER.info(...) and return. Update the block that currently
references markexpr to perform this collected-items marker scan instead so
path-based selection and negated expressions are handled correctly.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: f1e7cfee-29d6-44f3-95e6-73bbe12b17f8

📥 Commits

Reviewing files that changed from the base of the PR and between d476993 and a6697f4.

📒 Files selected for processing (1)
  • tests/conftest.py

Comment thread tests/conftest.py Outdated
Copy link
Copy Markdown
Contributor

@sheltoncyril sheltoncyril left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@sheltoncyril sheltoncyril merged commit b8e4bfe into opendatahub-io:main Mar 9, 2026
9 checks passed
@github-actions
Copy link
Copy Markdown

github-actions bot commented Mar 9, 2026

Status of building tag latest: success.
Status of pushing tag latest to image registry: success.

@dbasunag
Copy link
Copy Markdown
Collaborator Author

dbasunag commented Mar 9, 2026

/cherry-pick 3.4ea1

@rhods-ci-bot
Copy link
Copy Markdown
Contributor

Cherry pick action created PR #1185 successfully 🎉!
See: https://github.com/opendatahub-io/opendatahub-tests/actions/runs/22859060044

dbasunag added a commit that referenced this pull request Mar 9, 2026
… health is run (#1184) (#1185)

Co-authored-by: Debarati Basu-Nag <dbasunag@redhat.com>
@dbasunag
Copy link
Copy Markdown
Collaborator Author

dbasunag commented Mar 9, 2026

/cherry-pick 3.3

@rhods-ci-bot
Copy link
Copy Markdown
Contributor

Cherry pick action created PR #1186 successfully 🎉!
See: https://github.com/opendatahub-io/opendatahub-tests/actions/runs/22859294324

dbasunag added a commit that referenced this pull request Mar 9, 2026
… health is run (#1184) (#1186)

Co-authored-by: Debarati Basu-Nag <dbasunag@redhat.com>
@dbasunag dbasunag deleted the cluster_sanity branch March 19, 2026 00:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants