[jaeger] Replace outdated bitnami charts #696
Conversation
…stic chart) Signed-off-by: Jonah Kowall <[email protected]>
Signed-off-by: Jonah Kowall <[email protected]>
Signed-off-by: Jonah Kowall <[email protected]>
Signed-off-by: Jonah Kowall <[email protected]>
Signed-off-by: Jonah Kowall <[email protected]>
Signed-off-by: Jonah Kowall <[email protected]>
|
We don't support Kafka in the new chart, can remove it. And why not chart from confluence, main contributor? |
Okay I will remove it. Strimzi is a CNCF project and is used to run Kafka on Kubernetes so I figured it was the right choice. |
Signed-off-by: Jonah Kowall <[email protected]>
ct.yaml
Outdated
| - elastic=https://helm.elastic.co | ||
| - bitnami=https://charts.bitnami.com/bitnami | ||
| validate-chart-schema: false | ||
| validate-yaml: false |
There was a problem hiding this comment.
Why would we not want to validate? It's just a recipe for bugs.
There was a problem hiding this comment.
Yeah sorry, was debugging but fixed it now.
Signed-off-by: Jonah Kowall <[email protected]>
Signed-off-by: Jonah Kowall <[email protected]>
| {{- else -}} | ||
| {{- $clusterName := default "elasticsearch" $es.clusterName }} | ||
| {{- $host = printf "%s-master" $clusterName }} | ||
| {{- end }} |
There was a problem hiding this comment.
I really don't understand why we need all this complexity. I think it's none of Jaeger's business how users want to provision ES. Just look at this, a dozen files in our chart have to mention ES:
$ rg -l elastic
charts/jaeger/Chart.lock
charts/jaeger/Chart.yaml
charts/jaeger/README.md
ct.yaml
charts/jaeger/values.yaml
charts/jaeger/templates/elasticsearch-secret.yaml
charts/jaeger/templates/es-rollover-cronjob.yaml
charts/jaeger/templates/es-lookback-cronjob.yaml
charts/jaeger/templates/allinone-deploy.yaml
charts/jaeger/templates/es-index-cleaner-cronjob.yaml
charts/jaeger/templates/es-rollover-hook.yml
charts/jaeger/templates/spark-cronjob.yaml
charts/jaeger/templates/_helpers.tpl
charts/jaeger/templates/storage-configmap.yaml
I wouldn't even have any dependency on it in the Jaeger chart. But if we really-really want to make it very easy to setup Jaeger with ES via --set provisionDataStore.elasticsearch=true, fine, we have the default elasticsearch: settings in values.yaml, that is passed through to ES chart. If users want to customize those settings, they can edit that section. Why do we need any other logic? All the convoluted logic above is not tested in the CI, so I have no idea if it works.
This is not helping users, it's making them learn some completely arbitrary rules / DSL instead of relying on standard configs.
There was a problem hiding this comment.
Those were already there before; I can clean it up if you want. I didn't try to clean up all of those templates.
There was a problem hiding this comment.
I know, but this PR is now adding these 10 more lines of some obscure override logic. Are we trying to guess which name ES chart gives to its endpoint?
But to the larger point - yes I think we need to revisit the overall chart organization and remove all custom overrides logic. I think we need to keep it simple - if you want defaults, we provide them. If you want to customize - you provide full configuration.
There was a problem hiding this comment.
The challenge is that the Elastic official chart uses different settings than the older Bitnami one, so the goal is to make it work. If we want to fix the fundamental issues, we should revisit all that logic. I can give it a go if you'd like.
Signed-off-by: Jonah Kowall <[email protected]>
Signed-off-by: Jonah Kowall <[email protected]>
Signed-off-by: Jonah Kowall <[email protected]>
Signed-off-by: Jonah Kowall <[email protected]>
| @@ -0,0 +1,15 @@ | |||
| #!/bin/bash | |||
| set -e | |||
| ct install --config ct.yaml \ | |||
There was a problem hiding this comment.
is this different from how lint-test workflow calls this? If not, I would prefer to remove this shell script, add a Makefile with a target test containing this exact command, and change the workflow to call make test-es
#696 (comment) --------- Signed-off-by: Yuri Shkuro <[email protected]>
2 participants
What this PR does
Remove bitnami charts which are dead and not updated
Added charts for Strimzi (Kafka) and Elastic official chart
Which issue this PR fixes
Fixes security issues
Checklist
[jaeger]or[jaeger-operator])