feat: add mfe-k8s-volume-mounts patch - #267
Conversation
|
Hi @arbrandes, @DawoudSheraz, @brian-smith-tcril. While working with the new This PR adds the Please let me know what you think! |
| name: config | ||
| {%- if MFE_HOST_EXTRA_FILES %} | ||
| {{ patch("mfe-k8s-volume-mounts") | indent(12) }} | ||
| {%- endif %} |
There was a problem hiding this comment.
Can mfe-k8s-volume-mounts and mfe-k8s-volumes be merged in a single patch?
There was a problem hiding this comment.
@DawoudSheraz, thanks for your quick review!
I don’t think it’s really possible to unify them, since the volumes and volumeMounts blocks live in different parts of the Deployment spec. Each patch hook only injects into a single position, so combining them wouldn’t work. Also, they cover two different concerns, so keeping them separate helps maintain a clear separation of responsibilities.
There was a problem hiding this comment.
Hmm. If you look at deployments.yml template in tutor, it has a k8s-deployments patch applied at the end of file (https://github.com/overhangio/tutor/blob/release/tutor/templates/k8s/deployments.yml#L483). The idea is that if one wants to customize the deployment, they can do so using this patch. Won't a similar pattern work here? In the new patch, we can define the volume and volume mount. Sorry if I am missing some k8s technical context here.
There was a problem hiding this comment.
@DawoudSheraz The pattern you mentioned is designed for injecting complete resources through patches like k8s-deployments. That works when we’re defining entire Deployments or sections of the spec, but here, our goal is simply to add to the MFE deployment.
The two patches target different parts of the Deployment spec:
mfe-k8s-volumesinjects volume definitions underspec.template.spec.volumesmfe-k8s-volume-mountsinjects mounts underspec.template.spec.containers[].volumeMounts
A merge could theoretically work if the sections were adjacent but we have the config volume defined in between.
Even if they were next to each other, they still require different indentation and context (one at the pod level, the other inside a container).
It would be ideal to declare volumes and mount them together, but given Kubernetes’s structure, it’s necessary to keep them separated.
Let me know if I’ve captured your idea correctly, or if there’s anything else you’d like to comment on!
github-project-automation
Bot
moved this from In review
to Done
in Tutor project management
5 participants
Description
This pull request introduces a new patch,
mfe-k8s-volume-mounts, to improve how custom volumes are mounted in the MFE Kubernetes deployment. Now that the host extra file changes were applied at #264, I noticed that there was no patch referencing the volume mounts, which prevented themfe-k8s-volumespatch from actually being applied in Kubernetes environments.The documentation has been updated to guide users on defining and mounting volumes using the new patch, and a changelog entry records the addition.
Note
This patch is necessary for
mfe-k8s-volumesto work properly, as the corresponding volume mount patch was previously missing.For further information about the importance of this PR, please take a look at: feat: add volume patches to mfe service and k8s #264
Changes
mfe-k8s-volume-mountspatch to allow mounting custom volumes (such as ConfigMaps or PVCs) into themfecontainer in Kubernetes deployments, complementing the existingmfe-k8s-volumespatch. (tutormfe/patches/k8s-deployments)mfe-k8s-volumesandmfe-k8s-volume-mountstogether, including a practical example for mounting a ConfigMap.mfe-k8s-volume-mountspatch. (README.rst)