Skip to content

Conversation

@Snomaan6846
Copy link

@Snomaan6846 Snomaan6846 commented Dec 2, 2025

Signed-off-by: Snomaan6846 [email protected]

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED

Description

Adding MLServer ServingRuntime image param map for ODH Operator.

Jira-Link : https://issues.redhat.com/browse/RHOAIENG-39825

Summary by CodeRabbit

  • Chores
    • Updated internal configuration mappings to support additional image parameters.

✏️ Tip: You can customize this high-level summary in your review settings.

Signed-off-by: Snomaan6846 <[email protected]>

rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 2, 2025

Walkthrough

A single mapping entry is added to imageParamMap in the model controller support module, associating "mlserver-image" with the environment variable "RELATED_IMAGE_ODH_MLSERVER_IMAGE".

Changes

Cohort / File(s) Summary
Image Parameter Mapping Update
internal/controller/components/modelcontroller/modelcontroller_support.go
Adds new mapping entry: "mlserver-image" → "RELATED_IMAGE_ODH_MLSERVER_IMAGE" to imageParamMap

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~5 minutes

  • Verify the mapping key and environment variable reference are correct and consistent with existing entries
  • Confirm this mapping aligns with related image configuration for MLServer in the broader system

Poem

🐰 A mapping hops into place so neat,
mlserver-image, ODH image greet,
One line, simple and true,
Configuration shines anew! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: adding a new MLServer ServingRuntime image parameter mapping for the ODH Operator, which aligns with the code change in modelcontroller_support.go.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 09f502e and 0fad9d7.

📒 Files selected for processing (1)
  • internal/controller/components/modelcontroller/modelcontroller_support.go (1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Check requirement to update E2E test suite
internal/controller/components/modelcontroller/modelcontroller_support.go

[error] 1-1: CI step 'actions/github-script' failed: No e2e tests updated and skip checkbox not checked.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
  • GitHub Check: Build/push catalog image
  • GitHub Check: Run tests and collect coverage on internal and pkg
  • GitHub Check: kube-linter
  • GitHub Check: golangci-lint
🔇 Additional comments (1)
internal/controller/components/modelcontroller/modelcontroller_support.go (1)

21-34: Mapping addition looks consistent; please verify wiring and e2e expectations

The new entry

"mlserver-image": "RELATED_IMAGE_ODH_MLSERVER_IMAGE",

is consistent with the existing naming pattern and fits naturally into imageParamMap.

Two follow‑ups to double‑check before merge:

  1. Env var & manifest wiring
    Ensure RELATED_IMAGE_ODH_MLSERVER_IMAGE is actually defined (e.g., CSV, deployment, or env injection) and that the "mlserver-image" parameter is referenced in the relevant MLServer ServingRuntime manifests/templates.

  2. CI e2e requirement
    The pipeline reports: "No e2e tests updated and skip checkbox not checked."
    Either extend an existing e2e test that validates image parameters for ServingRuntimes to cover mlserver-image, or if there is no meaningful e2e impact, mark the "skip e2e updates" checkbox per the repo's policy.

Overall, the code change itself looks good; this is just about wiring and tests/CI policy.


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.

❤️ Share

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

@openshift-ci
Copy link

openshift-ci bot commented Dec 2, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign asanzgom for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@github-actions
Copy link
Contributor

github-actions bot commented Dec 2, 2025

❌ E2E Update Requirement Check Failed

No e2e tests were added/updated as part of this PR.

Action required from the PR author: Please either:

  1. Add or update e2e tests relevant to the PR changes in the tests/e2e/ directory, OR
  2. Evaluate the possibility of opting-out of this requirement:
    1. Inspect the opt-out guidelines, to determine if the nature of the PR changes allows for skipping this requirement
    2. If opt-out is applicable, please proceed to provide justification in the PR description (#### E2E update requirement opt-out justification section)
      • Note: In case your PR description does not adhere to our PR template and is missing the ### E2E test suite update requirement section (or any of its subsections), you can find the original PR template in .github/PULL_REQUEST_TEMPLATE.md, and restore the missing section from there
    3. After adding the justification, check the "Skip requirement to update E2E test suite for this PR" checkbox
    4. Submit/save changes to the PR description. The check will be triggered automatically and should pass

For more info, please refer to: workflow run details

Why this check exists:

  • E2E tests help ensure that changes work correctly in a real environment and don't break existing functionality
  • by default, every code-related PR should include an extension/update to the E2E test suite to cover the new changes
  • if the PR author is in doubt whether this requirement is applicable for their PR, please refer to the PR template for guidance about appropriate/inappropriate cases for opting-out

@Snomaan6846 Snomaan6846 marked this pull request as draft December 2, 2025 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

1 participant