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

bump appVersion to 5.10, allow multiple replicas (+HPA) for Deployment, allow setting externalService annotations, allow disabling allocateLoadBalancerNodePorts, add redis-ha subchart+config #25

Merged
merged 1 commit into from
Dec 9, 2023

Conversation

tamcore
Copy link
Contributor

@tamcore tamcore commented Dec 9, 2023

Sorry for the big PR. Didn't want to create 10 individual ones for such small things :)

  • bump chart version to 1.7.0
  • bump app version from 5.6 to 5.10 (this bit should probably be automated)
  • externalService: allow configuring annotations (as spec.loadBalancerIP is deprecated: Deprecate Service.Spec.LoadBalancerIP kubernetes/kubernetes#107235)
  • externalService: allow disabling of allocateLoadBalancerNodePorts which is not required with LoadBalancer implementations like MetalLB
  • deployment: allow multiple replicas, as since v5.10 traccar supports redis
  • deployment: allow configuring a custom deployment strategy, as Recreate is no longer needed, if the setup is configured for HA
  • add a manifest for HPA, because why not
  • add basic configuration to enable redis-ha (setting redis-ha.enabled deploys Redis and configures traccar to use it)

@filippolmt
Copy link
Contributor

Hi, sorry to ask, but I would also add the dependency to redis to the PR, with a default configuration if active, and I would also add a test with the redis deployment so that your processing is consistent.

@tamcore tamcore force-pushed the chart-updates branch 5 times, most recently from e2c2ad2 to 9cf0620 Compare December 9, 2023 12:51
… setting externalService annotations, allow disabling allocateLoadBalancerNodePorts
@tamcore
Copy link
Contributor Author

tamcore commented Dec 9, 2023

Done

@tamcore tamcore changed the title bump appVersion to 5.10, allow multiple replicas (+HPA) for Deployment, allow setting externalService annotations, allow disabling allocateLoadBalancerNodePorts bump appVersion to 5.10, allow multiple replicas (+HPA) for Deployment, allow setting externalService annotations, allow disabling allocateLoadBalancerNodePorts, add redis-ha subchart+config Dec 9, 2023
@tananaev
Copy link
Member

tananaev commented Dec 9, 2023

@filippolmt does this look good to merge? Not an expert in this, so need someone else to double check everything.

@filippolmt
Copy link
Contributor

Hi @tananaev, sorry if I jumped in to give advice on machining, but the good @tamcore did a great job and I took the liberty of making a suggestion, the only thing that doesn't sit right with me in this PR is the dependency that redis repository, I would have stayed on the bitnami repository, sorry @tamcore, how come you made this decision?

@tamcore
Copy link
Contributor Author

tamcore commented Dec 9, 2023

@filippolmt my initial attempt was indeed to use the bitnami version. but helm being helm it did some weird stuff and had problems using functions from the bitnami common chart, because it's used in both of them and only worked, if either the redis or mysql subchart were disabled.

so i resorted to fully including the dandydeveloper redis-ha chart, which is IMHO more "production grade" than bitnami's anyways and is also widely used by other projects such as ArgoCD (https://github.com/argoproj/argo-helm/blob/main/charts/argo-cd/Chart.yaml#L19-L23)

Edit: The error i was facing, where it tries to use the common subchart from the mysql subchart during templating of the redis subchart. I don't think it's worth it to finding workarounds to this, when there are better options out there.
Error: template: traccar/charts/redis/templates/replicas/service.yaml:12:14: executing "traccar/charts/redis/templates/replicas/service.yaml" at <include "common.labels.standard" (dict "customLabels" .Values.commonLabels "context" $)>: error calling include: template: traccar/charts/mysql/charts/common/templates/_labels.tpl:6:27: executing "common.labels.standard" at <include "common.names.name" .>: error calling include: template: traccar/charts/mysql/charts/common/templates/_names.tpl:6:18: executing "common.names.name" at <.Chart.Name>: nil pointer evaluating interface {}.Name

@filippolmt
Copy link
Contributor

I thank you for your reply @tamcore . @tananaev as far as I am concerned you can merge the PR, the tests were successful.
In any case in the future I would recommend expanding the tests on the chart, currently it only verifies that the deployment is successful.

@tananaev tananaev merged commit d5c530d into traccar:main Dec 9, 2023
1 check passed
@tananaev
Copy link
Member

tananaev commented Dec 9, 2023

Merged, thanks everyone.

@tananaev
Copy link
Member

tananaev commented Dec 9, 2023

Looks like CI failed on master: https://github.com/traccar/traccar-helm/actions/runs/7151883969

@tamcore
Copy link
Contributor Author

tamcore commented Dec 9, 2023

Addressed with #26. Sorry for the oversight 🙈

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.

3 participants