feat(helm): add an option to specify no PVC or a preexisting one#1433
feat(helm): add an option to specify no PVC or a preexisting one#1433Gylesie wants to merge 1 commit intoseerr-team:developfrom
Conversation
|
cc @M0NsTeRRR |
| claimName: {{ include "jellyseerr.configPersistenceName" . }} | ||
| claimName: {{ if .Values.config.persistence.existingClaim }}{{ .Values.config.persistence.existingClaim }}{{- else }}{{ include "jellyseerr.configPersistenceName" . }}{{- end }} | ||
| {{- else }} | ||
| emptyDir: {} |
There was a problem hiding this comment.
Why add an empty dir if there is no persistence ?
There was a problem hiding this comment.
I saw it having it done like this in the chart I got inspired from. I think it might be slightly more performant rather than relying on the container's writeable layer that utilizes Copy on Write, since emptyDir is just a folder on a node and would bypass this overhead.
There was a problem hiding this comment.
Okay, but it's for the configuration, so performance is not really needed. So we can keep it as it exists.
|
Hello, Thanks for your PR! Since it's a new feature, you need to bump the To generate the README, use helm-docs by running the Also, if you want to use an existing PVC, as you mentioned, set |
Thanks for the tip! I'll do that. While you are absolutely right, I think having an option akin to |
Okay, I didn't see many Helm charts doing this, but I don't disagree with your comment, so we can do it this way 👍. |
| # -- Creating PVC to store configuration | ||
| config: | ||
| persistence: | ||
| # -- set to true to use pvc |
There was a problem hiding this comment.
| # -- set to true to use pvc | |
| # -- Set to true to use PVC |
| # -- Name of the permanent volume to reference in the claim. | ||
| # Can be used to bind to existing volumes. | ||
| volumeName: "" | ||
| # -- specify an existing `PersistentVolumeClaim` to use |
There was a problem hiding this comment.
| # -- specify an existing `PersistentVolumeClaim` to use | |
| # -- Specify an existing `PersistentVolumeClaim` to use |
M0NsTeRRR
left a comment
M0NsTeRRR
left a comment
There was a problem hiding this comment.
Except for minor typos and the suggested changes we discussed, it looks good to me :)
gauthier-th
changed the title
|
@Gylesie can you please fix the requested changes? |
|
This pull request has merge conflicts. Please resolve the conflicts so the PR can be successfully reviewed and merged. |
|
This PR is stale because it has been open 30 days with no activity. Please address the feedback or provide an update to keep it open. |
|
@M0NsTeRRR do you want to fix the pending comments yourself? |
|
This PR is stale because it has been open 30 days with no activity. Please address the feedback or provide an update to keep it open. |
3 participants
Description
Adds an option to specify no PVC to be created for the persistence in which case an emptyDir is used as a volume. Also, adds an option to use an existing PVC instead of defining a new one.
I am new to helm templating, my changes were inspired by MoJo2600/pihole-kubernetes chart. Please do check for any mistakes.
Also, I did not know how to regenerate the readme. Maybe it is regenerated as part of CI/CD pipeline? Be free to commit some fixes if need be.
Screenshot (if UI-related)
N/A
To-Dos
Issues Fixed or Closed
N/A