Skip to content

Conversation

harshad16
Copy link
Member

Description

Read pipeline runtime image name from the imagestream tag
Related-to: opendatahub-io/notebooks#1070

How Has This Been Tested?

Tested it in cluster via:

workbenches:
    devFlags:
        manifests:
            - contextDir: components/odh-notebook-controller/config
              sourcePath: ''
              uri: 'https://github.com/harshad16/odh-kubeflow/archive/test-mani.tar.gz'

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.

Project coverage is 68.12%. Comparing base (690694f) to head (2ab35a7).
Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...otebook-controller/controllers/notebook_runtime.go 0.00% 11 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main     #603       +/-   ##
===========================================
+ Coverage   56.47%   68.12%   +11.65%     
===========================================
  Files          10        8        -2     
  Lines        2686     1754      -932     
===========================================
- Hits         1517     1195      -322     
+ Misses       1054      475      -579     
+ Partials      115       84       -31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@atheo89
Copy link
Member

atheo89 commented May 12, 2025

Tested this part of work in combination with standarize-manifests from notebooks repo, using the following devFlag:

      devFlags:
        manifests:
          - contextDir: components/odh-notebook-controller/config
            sourcePath: ''
            uri: 'https://github.com/harshad16/odh-kubeflow/archive/test-mani.tar.gz'
          - contextDir: manifests
            sourcePath: ''
            uri: 'https://github.com/harshad16/odh-notebooks/archive/standardize-manifests.tar.gz'

The changes looks good, the nbc populates the image_urlon the runtime json metadata from the imagestream field spec.from.name

Only one thing, could you add a comment clarifying that the image_url field is populated by the NBC and not directly from the ImageStream due to Kustomize's limitation in parsing structured annotation data like JSON — it might help others understand why it's handled this way.

/lgtm

@jiridanek
Copy link
Member

jiridanek commented May 12, 2025

I'm trying to decide whether I like this better, or the kustomize transformer.

Probably this solution is better, because everything runs under our control, whereas the transformer executes in an image that's controlled by the rhods-operator team. That could cause problems.

Attention: Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.

Needs testing.

@jiridanek
Copy link
Member

jiridanek commented May 13, 2025

Needs testing.

It still needs testing, other than that /lgtm

@jstourac
Copy link
Member

/lgtm

@jiridanek
Copy link
Member

/lgtm

@harshad16
Copy link
Member Author

Thanks for the review
/approve

Copy link

openshift-ci bot commented May 16, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: harshad16

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

The pull request process is described 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

@jiridanek
Copy link
Member

/override "Red Hat Konflux"

Copy link

openshift-ci bot commented May 16, 2025

@jiridanek: Overrode contexts on behalf of jiridanek: Red Hat Konflux

In response to this:

/override "Red Hat Konflux"

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@openshift-merge-bot openshift-merge-bot bot merged commit 8041e65 into opendatahub-io:main May 16, 2025
14 of 15 checks passed
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.

5 participants