-
Notifications
You must be signed in to change notification settings - Fork 178
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
Add shm to ServingRuntime volumes and volumeMounts #1761
Add shm to ServingRuntime volumes and volumeMounts #1761
Conversation
@DaoDaoNoCode Can you add test coverage for this, I think just adding unit test coverage for the |
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
I can see the changes in the console.
I think andrew added the no tests allowed of this PR @lucferbux |
Oh you are right! Let me do a review then! |
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.
Tested and overal looks great, but I wouldn't assume there's only one container, I haven't seen any Custom Serving Runtime with more than one container but if the spec supports multiple, let's just not assume that.
1aa1077
to
92113b8
Compare
@lucferbux Can you review it again? I updated the PR based on the feedback. |
/retest |
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
ok, the logic seems great, I've tested both new Serving Runtime and updating and old one, as well as adding a volume to a preexisting Custom Serving Runtime with two containers, one with a volume called shmdev
and it works as expected (btw, these are good testing ideas for the future)
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: lucferbux 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 #1685
Description
Apply shm
volumes
andvolumeMounts
when creating or updating (this is for backward compatibility, let me know if we want only to apply this to the future created serving runtimes @lucferbux ) the serving runtime.How Has This Been Tested?
and the following in the spec:
Test Impact
N/A
Request review criteria:
Self checklist (all need to be checked):
If you have UI changes:
After the PR is posted & before it merges:
main