search

fmjstudios / helm

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

(linkwarden) fix: Use object type for the SSO Configuration of Linkwarden #3

Closed CronixZero closed 11 months ago

CronixZero commented 11 months ago

What this PR does / why we need it

Fixes #2

Which issue this PR fixes

Checklist

CronixZero commented 11 months ago

Sorry, I read the CONTRIBUTING.md too late. I think commit fe25899a2a8a43dc6405bbfc4dbfe63efcfc8705 and 8bc16b775f47372a0a6c666a94bc9207a090e6c1 don't quite follow the commit scheme. I hope that it's not too big of an issue

mvprowess commented 11 months ago

Hey man, 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..

I think your enhancement is nice since it improves the autocompletion for chart users. Unfortunately I think it might be in vain since the command used to regenerate the schema when chart changes have occurred (make gen CHART=charts/linkwarden) would overwrite your changes on the next source changes unless you have figured out some way to alter the behavior of the readme-generator-for-helm?

The possibility I see is including defaults for the SSO configuration however that would mean shipping values which would have to be deleted if the feature isn't used and shipping defaults which require deletion seems counterproductive. Therefore I went for the opposite approach of including documented examples in the comments of values.yaml that people could use. This has also been somewhat enforced across all my charts as it's largely the same behavior as in Bitnami's charts which are widely used.

If you see some way around this issue I'd happily merge your changes!

CronixZero commented 11 months ago

Hey, it's fine. Take your time 😉

First of all, thank you for this repository and these Helm Charts.

Secondly, I wasn't aware that the schema was auto generated, that's too bad... I was trying to fix #2, which I created earlier, through this PR. Currently the SSO Values don't work because configuring those values results in the values.yaml being rejected by helm because it doesn't follow the schema.

It may be possible that I made a mistake when I configured the values but due to others with similiar issues I think that it may be useful to change something here.

Could it be that there's a deeper problem? It seems that the auto-generation task actually misinterprets the sso object list with a list of strings. I'm not really proficient in writing Helm Charts but would you be able to think of an issue just by that? Maybe there's something that has to be changed inside of the template or something like that.

CronixZero commented 11 months ago

If there's no other way I think adding defaults would be the best approach to this. I concur that it's bad for code maintainability but if we're not able to find another way it's between a hit to maintainability and a feature not working although the theoretical code is there for it

mvprowess commented 11 months ago

Hey @CronixZero,

so I think I have found a nice middle-ground. The metadata defining guidelines for the schema generation accepts multiple modifiers. If I set it to array, object instead of just array the the schema generator drops the property from the schema entirely, thereby mitigating your issue. Users still have the values.yaml with its' abundance of doc-comments so I think this is the best way to move forward, since maintaining the schema by hand sounds like a nightmare..

I have already deployed a fix here and published (two) new versions for the Linkwarden chart which include these changes (latest: 0.3.0). There's a new minor release because I changed the existingSecret API.

I was able to install the chart just fine with the new schema and all previous warning also disappeared so I will be closing this PR for now. Thank you for your work nonetheless. Please submit a new PR if you want to be included in the AUTHORS as I just noticed I forgot to add you 😅 They are your changes after all.