search

opf / helm-charts

OPF helm chart repository
https://charts.openproject.org
GNU General Public License v3.0
12 stars 22 forks source link

allow setting the storageClass for /tmp and /app/tmp volumes #89

Closed johanneskastl closed 2 months ago

johanneskastl commented 8 months ago

charts/openproject/templates/_helpers.tpl: use .Values.persistence.storageClassName for /tmp and /app/tmp volumes, if it is defined

changeset-bot[bot] commented 8 months ago

🦋 Changeset detected

Latest commit: dfcf786129139a8625a36c5190f29eaada842d67

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package | Name | Type | | ------------------------ | ----- | | @openproject/helm-charts | Minor |

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

johanneskastl commented 8 months ago

Should I use a changeset for this?

oliverguenther commented 7 months ago

Hi @johanneskastl , indeed every change to the helm-chart needs a changeset, as with it we track the changelog after a release. I've added one for you!

This PR is a good default but I would imagine some people would want to set a different storageClassName for the tmp volumes as they have different use-cases. We could add a new tmpStorageClassName which is empty by default and do a coalesce?

johanneskastl commented 7 months ago

Thanks for adding the changeset. Funny name!

I agree it might be a good idea to offer that functionality, but I have no time in the next couple of days to implement this.

So for now I would propose to merge this and add the different storageClass functionality later on.

kastl-ars commented 2 months ago

Any news on this? Could this be merged (after a rebase, of course)? Or are there any objections?

johanneskastl commented 2 months ago

Any news on this? Could this be merged (after a rebase, of course)? Or are there any objections?

Sorry for the noise, I had some minutes and enhanced this PR finally to support either a tmpStorageClassName (if set) or falling back to the regular storageClassName.

@oliverguenther Is this how you had it in mind?

oliverguenther commented 2 months ago

Hi @johanneskastl , thanks a lot for the update. Yes this is exactly what I had in mind. I'll release this today 👍

oliverguenther commented 2 months ago

Released as 5.4.0 https://github.com/opf/helm-charts/releases/tag/openproject-5.4.0. Sorry for the delay!

oliverguenther commented 2 months ago

FYI As I'm also about to merge PR #104 , we will move the value from persistence.tmpStorageClassName to openproject.tmpVolumesStorageClassName for consistency with the other options (useTmpVolumes, size etc.).

This will be released as chart 7.0 even though it was just release today to adhere with semantic versioning. I'd recommend you update directly to that and use the updated syntax

kastl-ars commented 2 months ago

FYI As I'm also about to merge PR #104 , we will move the value from persistence.tmpStorageClassName to openproject.tmpVolumesStorageClassName for consistency with the other options (useTmpVolumes, size etc.).

This will be released as chart 7.0 even though it was just release today to adhere with semantic versioning. I'd recommend you update directly to that and use the updated syntax

Thanks for the warning, I am currently upgrading to 6.0.0 but will upgrade to 7.0.0 once it was released...

oliverguenther commented 2 months ago

FYI 7.0.0 is now released https://github.com/opf/helm-charts/releases/tag/openproject-7.0.0

kastl-ars commented 2 months ago

Thanks for the warning, I am currently upgrading to 6.0.0 but will upgrade to 7.0.0 once it was released...

In my case there was no change necessary, as I only set the persistence.storageClassName which is still used as a fallback in case the tmpVolumesStorageClassName is not set. So everything should be fine!