dirsigler / uptime-kuma-helm

This Helm Chart installs Uptime-Kuma from @louislam to your Kubernetes Cluster.
https://helm.irsigler.cloud
GNU General Public License v3.0
151 stars 53 forks source link

Feature/simplify configuration after port fix #46

Closed marcules closed 2 years ago

marcules commented 2 years ago

Description of the change

After comparing deployment.yaml with statefulset.yaml and pvc.yaml with statefulset.yaml I've seen that there are differences introduced in a regression when the sts was first added and deployment.yaml was first removed and added in again - part of this fix normalizes both configurations.

Additionally, now, that the port-validation fix is introduced in upstream uptime-kuma a exported service in the environment variables would not cause the same issue with invalid port numbers anymore - therefore these changes were simplified and moved into helper, as well as using the pod/container port name instead of the value, making it possible to configure the exposed port in the container image through values.yaml and also set a different port in the service through values.yaml as well.

Benefits

Make the service easier to configure and make it so deployment+pvc and statefulset behave the same.

Possible drawbacks

n/a

Applicable issues

n/a

Additional information

Variables were not updated, I think this happens automatically now, right? @dirsigler - if that is the case, maybe that step can be removed from the PR-template?

Checklist

dirsigler commented 2 years ago

@marcules Thank you very much for your changes. It is true, the whole deployment vs. statefulset topic added a bit of overhead which affected the overall Chart quality.

Value changes are added when the pre-commit framework is installed and used. Means:

# Install framework:
https://pre-commit.com/#installation

# run framework manually in git repo
pre-commit run -a

# install git hooks permanently for local checkout
pre-commit install
marcules commented 2 years ago

Hi @dirsigler I'll look into this in a couple of days:

[X] your change req. is valid, it is not strictly necessary in that case - but it also "doesn't hurt" - I'll change it nevertheless [X] the changes to upstrea uptime-kuma were not yet released in a docker image, I'll wait for that to happen and also update the docker image here [ ] the override / setting of UPTIME_KUMA_PORT would not be strictly necessary after the fix, but I would leave it in so users can run on a non-standard port, what do you think? It probably is not necessary to set the (pod/container internal) port at all, because the exposed service port is configurable and can map to that standard port; with this change both of them could be configured independently, just not sure if it's needed @dirsigler [ ] pod-labels should maybe not be set for all selector labels? The previous configuration was/is consistent between deployment and statefulset, but IMHO "extra labels" and "selector labels" are not the same and should not be conflated - do you think we should keep the extra labels in selector-labels (consistent with previous impl.) - or move them out? @dirsigler [X] Have a nice easter holiday