Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove JUPYTER_IMAGE env variable from Notebook CR #505

Closed
wants to merge 1 commit into from

Conversation

atheo89
Copy link
Member

@atheo89 atheo89 commented Jan 14, 2025

Related to: https://issues.redhat.com/browse/RHOAIENG-7551

Description

As the JUPYTER_IMAGE env variable is not used, we can clean up our code

How Has This Been Tested?

Should be tested in conjunction with dashboard PR changes:

  1. Modify rhoai-operator DSC .yaml with devFlags that correspond to workbench changes
    workbenches:
      devFlags:
        manifests:
          - contextDir: components/odh-notebook-controller/config
            uri: 'https://github.com/atheo89/kubeflow/tarball/RHOAIENG-7551'
      managementState: Managed
  1. Modify rhods-dashboard deployment to use quay.io/opendatahub/odh-dashboard:pr-3407 image

  2. Deploy a workbench on RHOAI. Run the following command env | grep JUPYTER_IMAGE on workbench's terminal to ensure that the JUPYTER_IMAGE environment variable is not present. As well as should check on the Notebook CR that there is no env.JUPYTER_IMAGE.

  3. Drop the internal registry and re-run step 3.
    oc edit configs.imageregistry.operator.openshift.io -n openshift-image-registry change spec.ManagmentState to Removed

  4. Inspect the odh-notebook-controller deployment logs for suspicious entries related to JUPYTER_IMAGE. Check the logs of the odh-notebook-controller deployment. Ensure that there are no logs/errors mentioning JUPYTER_IMAGE.

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

Copy link

openshift-ci bot commented Jan 14, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link

openshift-ci bot commented Jan 14, 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 ask for approval from atheo89. 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

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 71.12%. Comparing base (330d816) to head (24ab7a7).

Additional details and impacted files
@@             Coverage Diff             @@
##             main     #505       +/-   ##
===========================================
+ Coverage   55.27%   71.12%   +15.85%     
===========================================
  Files           9        7        -2     
  Lines        2276     1330      -946     
===========================================
- Hits         1258      946      -312     
+ Misses        922      319      -603     
+ Partials       96       65       -31     

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

@atheo89 atheo89 changed the title Draft - RHOAIENG 7551 Remove JUPYTER_IMAGE env variable from Notebook CR Jan 14, 2025
@atheo89
Copy link
Member Author

atheo89 commented Jan 14, 2025

The discussion of this env removal is under discussion JFR: https://redhat-internal.slack.com/archives/C060A5FJEAD/p1736866386274129

@atheo89
Copy link
Member Author

atheo89 commented Jan 14, 2025

Related with dashboard PR: opendatahub-io/odh-dashboard#3407

@atheo89 atheo89 marked this pull request as ready for review January 14, 2025 15:52
@openshift-ci openshift-ci bot requested review from dibryant and paulovmr January 14, 2025 15:52
@atheo89 atheo89 requested review from jiridanek and harshad16 and removed request for paulovmr and dibryant January 14, 2025 15:56
@atheo89
Copy link
Member Author

atheo89 commented Jan 16, 2025

After discussing with the team, we have decided not to proceed with removing the JUPYTER_IMAGE environment variable from the notebook CR, as it is currently used in our QE tests on the ODS-CI test platform.
We will revisit the possibility of its removal once we transition away from this dependency.
I am closing this PR for now.

@atheo89 atheo89 closed this Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants