search

cetic / helm-zabbix

Helm Chart For Zabbix
https://artifacthub.io/packages/helm/cetic/zabbix
Apache License 2.0
56 stars 57 forks source link

Implement secret/volume mounting for server & proxy #50

Closed Lillecarl closed 2 years ago

Lillecarl commented 2 years ago

WIP status since this is my first ever public modification to a Helm chart.

What this PR does / why we need it:

Allows mounting things into zabbix server and zabbix proxy

Which issue this PR fixes

None

Special notes for your reviewer:

First time I touch a Helm chart, it works in my env and linter passes, but if everything is "best practice" I wouldn't know.

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

Lillecarl commented 2 years ago

@MartinHell https://gist.github.com/Lillecarl/4a0ecc4546f8eae2d681d96d22a56215 Here's an example w/ both secret and PVC. In case you're unsire of the "api" :)

MartinHell commented 2 years ago

@MartinHell gist.github.com/Lillecarl/4a0ecc4546f8eae2d681d96d22a56215 Here's an example w/ both secret and PVC. In case you're unsire of the "api" :)

Thanks! I noticed you've made the change in the templates/statefulset-zabbix-server.yaml :+1:

So maybe the next thing would be to add the configuration example to the documentation and the values.yaml file in this repo.

I'll see if I can test this out in my environment as well. For now I guess you'll need to wait for @aeciopires to have a look at this PR:)

Lillecarl commented 2 years ago

@MartinHell Did you test?

aeciopires commented 2 years ago

Hello @Lillecarl!

Thank you very much for your contribution to the project. I've been away for a few weeks. I'll look at the Pull Request carefully and make my thoughts before the merge.

aeciopires commented 2 years ago

Hi @Lillecarl!

I liked of your modifications.

I agree with @MartinHell. Can you add config default in values.yaml file? Can you add too examples of use in docs/example/kind/values.yaml?

This help other peoples undestand how to use this funcionality.

Greetings

Lillecarl commented 2 years ago

@aeciopires I'll fix it next week! :)

Thanks for your replies!

sa-ChristianAnton commented 2 years ago

In recently published version 3.0.0 of this chart, we have added the capability to add extraVolumes and extraVolumeMounts to every of the components deployed by this chart, by adding the appropriate yaml snippet into the values.yaml file. I believe this PR is obsolete with this addition. I am sorry not having commented before that we were doing this, but as we have been doing quite extensive changes which took some time to implement, I didn't notice your pull request in the meanwhile.

aeciopires commented 2 years ago

I agree @sa-ChristianAnton.

@Lillecarl can you test new version of chart and send us feedback. If necessary, other issue and pull request will can open.

Have a nice weekend.