Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/simplify configuration after port fix #46

Conversation

marcules
Copy link
Contributor

@marcules marcules commented Apr 12, 2022

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

  • Chart version bumped in Chart.yaml according to semver.
  • Variables are documented in the README.md

@dirsigler dirsigler self-requested a review April 12, 2022 19:07
@dirsigler
Copy link
Owner

@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

Copy link
Owner

@dirsigler dirsigler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added question.

charts/uptime-kuma/templates/ingress.yaml Outdated Show resolved Hide resolved
@marcules
Copy link
Contributor Author

marcules commented Apr 14, 2022

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

@marcules marcules marked this pull request as draft April 14, 2022 15:15
@marcules marcules force-pushed the feature/simplify-configuration-after-port-fix branch 2 times, most recently from e54501a to 14e16ea Compare April 18, 2022 15:31
@marcules marcules force-pushed the feature/simplify-configuration-after-port-fix branch from 14e16ea to a91b87a Compare April 18, 2022 15:35
@marcules marcules requested a review from dirsigler April 18, 2022 15:39
@marcules marcules force-pushed the feature/simplify-configuration-after-port-fix branch from a91b87a to 83d3720 Compare April 18, 2022 15:42
Copy link
Owner

@dirsigler dirsigler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@dirsigler dirsigler closed this Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants