[Service Mesh] include SM notebook patches in one function #1955
Conversation
| path: '/metadata/annotations/opendatahub.io~1service-mesh', | ||
| value: String(enableServiceMesh), | ||
| }); | ||
| export const getServiceMeshPatches = (enableServiceMesh: boolean): Patch[] => [ |
There was a problem hiding this comment.
Simple inquiry -- do we plan to support these patches not being included or should they always be included since they share the on/off state. (such as feature flag off)
There was a problem hiding this comment.
I think these patches should always be included.
Reasoning being that in particular, the inject-oauth alongside the service-mesh patch/annotation is used by the notebook-controller (see PR https://github.com/opendatahub-io/kubeflow/pull/172/files) to decide if the oauth-proxy sidecar should be injected.
So for inject-oauth particularly this must be included, and I think similar reasoning applies for the other two.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: andrewballantyne 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 |
Closes #1954
Description
During review of #1088, it was left as an open comment to consolidate the three notebook annotation/label patch functions into one for code cleanness and to ensure future development doesn't "disable" one of the patches.
This PR implements this by wrapping the three patches into one function, and calling it the same as before but now flattening the returned array of patches.
How Has This Been Tested?
Tested by installing operator v2 with service mesh: see opendatahub-io/opendatahub-operator#605. Then, tested creating notebooks from the jupyter tile and from a data science project to ensure that the correct annotations and label are still added to the notebook object when using Service Mesh.
Test Impact
I would be happy to add tests, but would need some directions on how to add testing for this type of PR (patching of NB resource).
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main