-
Notifications
You must be signed in to change notification settings - Fork 163
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
Deprecate kfdefApplication field in OdhApplications #1592
Deprecate kfdefApplication field in OdhApplications #1592
Conversation
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Tested this on my end. The changes work fine for me.
Once KfDefs are no longer supported, this functionality needs to be removed.
c29e1a8
to
636fda1
Compare
New changes are detected. LGTM label has been removed. |
Removed the unused type (diff). |
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: manaswinidas 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 |
Readding the LGTM because of the removal of the unused type. |
Closes: #1510
Description
Remove the reliance on
kfdefApplications
field in theOdhApplication
CRs (when there are no KfDefs installed).When the new Operator is installed, it does not use KfDefs... the CRD does not exist, the items do not exist. In #1537 we removed the error when no KfDef CRD exists.
This is to adjust us back to working --
kfdefApplications
is not needed when no KfDefs are found and are completely ignored. Moving forward we'll use the "component model" and each component will offer resources if they wish for Dashboard to render.Couple screenshots...
Should be noted, I could not actually deprecate the field. K8s doesn't allow it.
How Has This Been Tested?
notebookController.enabled = false
) but that'll fallback on JupyterHub tile (which we should be getting rid of in Remove JH References #534)Test Impact
No tests were changed or added.
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main