search

fmjstudios / helm

🪖 A collection of MIT-licensed Helm Charts
MIT License
13 stars 13 forks source link

(linkwarden) fix: Storage, SSO and generic Helm template related fixes #4

Closed kehralexander closed 11 months ago

kehralexander commented 11 months ago

What this PR does / why we need it

When storage type is set to S3 the PVC is not being created (correct) but referenced (incorrect) as a volume mount. Furhter there were missing S3 related variables and additional helm template issues. This PR stops all the abote that from happending.

Checklist

kehralexander commented 11 months ago

This PR does not replace PR https://github.com/fmjstudios/helm/pull/3 but actually builds upon it, hence it should be merged first.

mvprowess commented 11 months ago

Hey @kehralexander, first of all thank you for contributing to my charts! Likewise sorry for getting to your PR so late, I've had a lot to do..

Your changes look really good. Also looks like I missed quite a lot there 🤦‍♂️... I haven't gotten to actually deploying these charts on my own cluster since I'm currently reworking it.

However I am not quite sure about the introduction of the replica parameter. I initially had it in the values as well but explicitly scrapped it since I thought Linkwarden didn't support a replicated deployment. Also, multiple apps writing to a single database could potentially cause more harm than improvement.

Maybe you have more experience with Linkwarden than me? Feel free to correct me here.

Lastly don't forget to add yourself to our AUTHORS file in the .github directory. I will try to use this file to display all contributors as some point later when I get the time. Maybe as the basis for some thing like a .all-contributorsrc.

mvprowess commented 11 months ago

Hey @kehralexander, I have just published new a new minor 0.3.0 including most of the fixes you submitted. I have however not included the replicas property due to the aforementioned questions.

I have also taken the liberty of including you in the AUTHORS since they are your changes after all 🙇🏼‍♂️

I will be closing this PR for now.

kehralexander commented 11 months ago

@FMJdev Thank you very much for implementing the changes!

However I am not quite sure about the introduction of the replica parameter. I initially had it in the values as well but explicitly scrapped it since I thought Linkwarden didn't support a replicated deployment. Also, multiple apps writing to a single database could potentially cause more harm than improvement.

Multiple replicas into one database is usually fine with postgres (if the application creates transactions correctly), while I'm not sure about linkwardens transaction handling I've been using it with 2 replicas just fine.

it looks like they're using prisma ORM and nothing self-baked so I'd assume it should be fine. Linkwardens cloud offering might also work by simply having many replicas (I'd guess but I don't know)

Of course more than 1 replica only makes sense with either ReadWriteMany (not tested by me) or S3 storage.