helm-charts icon indicating copy to clipboard operation
helm-charts copied to clipboard

[kube-prometheus-stack][tooling] Migrate the jsonnet tooling to use native golang scripts

Open mallardduck opened this issue 5 months ago • 7 comments

What this PR does / why we need it

Replacing the Python based (which is really C<->C++) jsonnet tooling with Golang native tooling.

Which issue this PR fixes

  • Addresses: https://github.com/prometheus-community/helm-charts/issues/5852

Special notes for your reviewer

Checklist

  • [x] DCO signed
  • [n/a] Chart Version bumped - Not sure if needs to be bumped as no effective changes were made?
  • [x] Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

mallardduck avatar Jul 01 '25 00:07 mallardduck

I have mixed feeling here. I agreed that the python code was spaghetti code, but not sure how many kube-prometheus-stack maintainers can maintain go code.

If agreed, I will do a deeper review here.

jkroepke avatar Jul 01 '25 08:07 jkroepke

I have mixed feeling here. I agreed that the python code was spaghetti code, but not sure how many kube-prometheus-stack maintainers can maintain go code.

If agreed, I will do a deeper review here.

Same feeling, but in reality the entire ecosystem is in go (k8s, helm, prometheus, alertmanager, etc) so I think it makes sense.

In the end it's all about opinion. For the record I prefer golang, and I use it pretty much every day. I can also review this later.

GMartinez-Sisti avatar Jul 01 '25 09:07 GMartinez-Sisti

I also use go on a daily basis but I would tend to not change things that work but if more people want to do that, it's fine for me and we can review and maintain this

The main issue with this PR is that's it's a big bang and it's doing too much. 85 changed files and more than 3000 added lines is too much.

If @jkroepke and @GMartinez-Sisti are fine with this (wait for their response) then I would prefer that you split this PR into multiple ones with at least:

  • 1 for skaffolding
  • 1 for rules and switching to use the new go one
  • 1 for dashboards and switching to use the new go one
  • 1 to update the resources

QuentinBisson avatar Jul 01 '25 09:07 QuentinBisson

then I would prefer that you split this PR into multiple ones

This is always a good approach, so I support it.

Also I would only expect the checksum on the header of the generated files to change, the files have changes on the content and github doesn't make it easy to understand what actually changed.

GMartinez-Sisti avatar Jul 01 '25 10:07 GMartinez-Sisti

the files have changes on the content and github doesn't make it easy to understand what actually changed.

The only changes to files are superficial ones related to how yaml is marshalled. Of note:

  • k8s-coredns.yaml path is corrected - the python relative path was incorrect for source VS dest paths. (So this fixed a bug)
  • All dashboards have a superficial change of field order change (golang vs python order of allValue field)
  • Some description fields use different yaml string format; I think this is because python was splitting lines at 1000 characters.

So it will be very hard to get an exact 1:1 result out of golang and python due to subtle differences in yaml library implementations.


then I would prefer that you split this PR into multiple ones

This is always a good approach, so I support it.

TBH I wasn't sure how much interest there would be in this, so didn't know how to prepare it. I'm happy to split up this effort into multiple PRs if that's the preferred way to digest it.

As mentioned this all came from an idea to add this to internal tooling my team uses, so pulling it out of the original source where I created it is why it's quite a big bang PR. As I have time this week, I will split up this PR into the suggested PRs and close this one.

mallardduck avatar Jul 01 '25 13:07 mallardduck

Hi, could you please skip the changes on the template for now? I'm unable to perform a review. Somehow the Github UI is terrible slow on the file diffs.

jkroepke avatar Jul 04 '25 10:07 jkroepke

Appreciate the reviews and feedback - just wanted to clarify my game plan here. I do want to keep this moving too; but actual work projects came up & I over looked the holiday, so both those delayed any progress on this so far.

🤞 I'm hoping this week will be better for finding time to prioritize this. I had already started splitting up the changes based on this feedback. So I wasn't planning on directly modifying or submitting this PR further.

  • 1 for skaffolding
  • 1 for rules and switching to use the new go one
  • 1 for dashboards and switching to use the new go one
  • 1 to update the resources

Sorry all, I haven't been able to circle back to allocate time to this community work. Hopefully in the coming weeks 🤞 .

mallardduck avatar Jul 08 '25 13:07 mallardduck