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

RHOAIENG-15335: feat(odh-nbc/webhook): only update oauth-proxy image in CREATE events #437

Merged

Conversation

jiridanek
Copy link
Member

https://issues.redhat.com/browse/RHOAIENG-15335

This is an alternate solution, see also

Description

Don't touch OAuth Proxy parts when the Notebook is Updated, only set it once on Create.

How Has This Been Tested?

Not yet, so far only thinking about possible approaches to solve the problem above. This PR is one such possible approach, there may be better ones.

I don't like this solution myself much, but it's for sure simpler than the other one.

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

@atheo89
Copy link
Member

atheo89 commented Nov 6, 2024

I did the following test:

  • Created a notebook using the original image of the odh-notebook-controller before your changes.
    • notebook-b1 created at Nov 6, 2024, 11:35 AM
    • image: 'registry.redhat.io/openshift4/ose-oauth-proxy@sha256:4bef31eb993feb6f1096b51b4876c65a6fb1f4401fee97fa4f4542b6b7c9bc46'
  • Changed odh-notebook-controller image to include your changes.
    • quay.io/opendatahub/odh-notebook-controller:pr-437
    • Created a new notebook-a1 at Nov 6, 2024, 11:41 AM
  • Updated the oauth proxy image to on the odh-notebook-controller deployment to registry.redhat.io/openshift4/ose-oauth-proxy@sha256:4f8d66597feeb32bb18699326029f9a71a5aca4a57679d636b876377c2e95695
  • Check if any notebook has been affected
    • Nothing change so far on the old deployed notebooks
  • Created a new notebook and check the assigned oauth
    • c1 created at Nov 6, 2024, 11:46 AM
    • It has the new oauth image
    • The rest notebooks didn't get any restarts
  • Till write you this message at 12.02pm nothing has changed on the notebooks
  • One additional check was to restart the b1 and a1 notebooks and see if the oauthproxy image has been changed and it didn't which is good sign

So I would say /lgtm

@atheo89
Copy link
Member

atheo89 commented Nov 6, 2024

Tested also in Harshad's cluster everything looks good as well

@harshad16
Copy link
Member

Review the bits, seems like as pointed out Admission.Update does get triggered on culler being on.
It would be best, we only inject Oauth change on the creation and plan the rest in future.

/lgtm
/approve

Copy link

openshift-ci bot commented Nov 6, 2024

[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

@openshift-ci openshift-ci bot added the approved label Nov 6, 2024
@jiridanek
Copy link
Member Author

/override ci/prow/odh-notebook-controller-e2e
https://issues.redhat.com/browse/RHOAIENG-15492

Copy link

openshift-ci bot commented Nov 6, 2024

@jiridanek: Overrode contexts on behalf of jiridanek: ci/prow/odh-notebook-controller-e2e

In response to this:

/override ci/prow/odh-notebook-controller-e2e
https://issues.redhat.com/browse/RHOAIENG-15492

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.

@jiridanek
Copy link
Member Author

/override ci/prow/odh-notebook-controller-e2e https://issues.redhat.com/browse/RHOAIENG-15492

Copy link

openshift-ci bot commented Nov 6, 2024

@jiridanek: /override requires failed status contexts, check run or a prowjob name to operate on.
The following unknown contexts/checkruns were given:

  • https://issues.redhat.com/browse/RHOAIENG-15492

Only the following failed contexts/checkruns were expected:

  • ci/prow/images
  • ci/prow/kf-notebook-controller-pr-image-mirror
  • ci/prow/odh-notebook-controller-e2e
  • ci/prow/odh-notebook-controller-pr-image-mirror
  • ci/prow/odh-notebook-controller-unit
  • pull-ci-opendatahub-io-kubeflow-main-images
  • pull-ci-opendatahub-io-kubeflow-main-kf-notebook-controller-pr-image-mirror
  • pull-ci-opendatahub-io-kubeflow-main-odh-notebook-controller-e2e
  • pull-ci-opendatahub-io-kubeflow-main-odh-notebook-controller-pr-image-mirror
  • pull-ci-opendatahub-io-kubeflow-main-odh-notebook-controller-unit
  • tide

If you are trying to override a checkrun that has a space in it, you must put a double quote on the context.

In response to this:

/override ci/prow/odh-notebook-controller-e2e https://issues.redhat.com/browse/RHOAIENG-15492

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.

@jiridanek
Copy link
Member Author

/override ci/prow/odh-notebook-controller-e2e

Copy link

openshift-ci bot commented Nov 6, 2024

@jiridanek: Overrode contexts on behalf of jiridanek: ci/prow/odh-notebook-controller-e2e

In response to this:

/override ci/prow/odh-notebook-controller-e2e

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 9d606be into opendatahub-io:main Nov 6, 2024
12 checks passed
@jiridanek jiridanek deleted the jd_update_oauth_on_create branch November 7, 2024 07:30
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.

3 participants