search

temporalio / helm-charts

Temporal Helm charts
MIT License
294 stars 321 forks source link

feat: updated grafana and prometheus helm dependencies #424

Closed dotdc closed 1 month ago

dotdc commented 11 months ago

What was changed

Updated Temporal's Helm chart dependencies:

While it's a huge bump, I've raised the temporal minor version as it shouldn't be a breaking change in most cases. Let me know if you prefer to bump the chart's version differently.

Why?

Take advantage of products updates while reducing security risks.

Checklist

  1. Closes

N/A

  1. How was this tested: 
helm template temporal . -n temporal | kubectl apply -f -
  1. Any docs updates needed?

N/A

CLAassistant commented 11 months ago

CLA assistant check
All committers have signed the CLA.

dotdc commented 10 months ago

Hi,

Could someone review this PR? I can also bump the charts further as new versions came out since then.

cc @mindaugasrukas

mindaugasrukas commented 10 months ago

@alexshtin took over ownership of this project. While this PR LGTM, In general, the architecture of having a direct dependency on Grafana, Prometheus, Cassandra, and Elasticsearch is incorrect. That said, you can install whatever indirect dependencies you like and don't rely on this helm-chart. For example, if you want to switch from Cassandra to Postgres, you don't need to add this direct dependency here, and you can install whatever Postgres version you like. The only requirement here is to have the correct dependencies installed.

dotdc commented 10 months ago

I was not sure for ownership, but thank you for your reply 👍 We have a workaround, but it would be nice to update the default for everyone.

robholland commented 2 months ago

Please feel free to re-bump these. Leave the chart version as-is though, that gets bumped by our tooling on release. I will ensure it gets reviewed and merged.

dotdc commented 2 months ago

Hi @robholland, versions have been bumped again, also left the chart version as-is like requested.

robholland commented 1 month ago

I've tested that end to end metrics from Temporal Server -> Grafana is working. I'll merge this, but please note that we'll soon be removing grafana and prometheus from our chart.

node-exporter will not come up without adjustments on Docker Desktop on Darwin, but ignoring that as we'll soon remove these dependencies anyway (as they aren't actually dependencies at all).