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

[kube-prometheus-stack] prometheus-operator0.59.1/Prometheus2.38.0/all dependencies #2440

Merged
merged 1 commit into from
Sep 14, 2022

Conversation

Allex1
Copy link
Contributor

@Allex1 Allex1 commented Sep 9, 2022

What this PR does / why we need it

Which issue this PR fixes

(optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged)

  • Bumps the kube-state-metrics dependency version to latest

Special notes for your reviewer

Checklist

  • DCO signed
  • Chart Version bumped
  • Title of the PR starts with chart name (e.g. [prometheus-couchdb-exporter])

Copy link
Member

@monotek monotek left a comment

Choose a reason for hiding this comment

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

this should be a breaking changhe as the default labels are changed in the subchart.
please add update instructions to readme.

@Allex1
Copy link
Contributor Author

Allex1 commented Sep 12, 2022

this should be a breaking changhe as the default labels are changed in the subchart.
please add update instructions to readme.

@monotek I'm assuming you're referring to #927

@Allex1 Allex1 force-pushed the BUMP branch 2 times, most recently from 60de346 to 6cc5287 Compare September 12, 2022 16:55
@Allex1 Allex1 changed the title [kube-prometheus-stack] BUMP kube-state-metrics dependency to 4.18.* [kube-prometheus-stack] prometheus-operator 0.59.1/Prometheus 2.37.1/KSM 4.18.* dep Sep 12, 2022
@Allex1
Copy link
Contributor Author

Allex1 commented Sep 12, 2022

@mrueg @monotek because 40.0.0 is such a round number I've updated prom-op and prom as well.

Copy link
Member

@monotek monotek left a comment

Choose a reason for hiding this comment

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

the crds, dashboards and rules are not upgraded (see scripts in hack directory).

the readme should have upgrade instructions for the labels, not just a note that it will break.

edit: sorry, i had node exporter in mind: #2212

please update it too. upgrade instructions can be used from its readme: https://github.com/prometheus-community/helm-charts/tree/main/charts/prometheus-node-exporter#3x-to-4x

please test the update at least once. the release lable might be needed to set now for node exporter.

Copy link
Member

@mrueg mrueg left a comment

Choose a reason for hiding this comment

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

values.yaml needs the prometheus-operator and config-reloader tags updated to v0.59.1 as well

@Allex1 Allex1 changed the title [kube-prometheus-stack] prometheus-operator 0.59.1/Prometheus 2.37.1/KSM 4.18.* dep [kube-prometheus-stack] prometheus-operator0.59.1/Prometheus2.37.1/all dependencies Sep 12, 2022
@mrueg
Copy link
Member

mrueg commented Sep 12, 2022

Thanos can be updated to https://github.com/thanos-io/thanos/releases/tag/v0.28.0 as well

charts/kube-prometheus-stack/README.md Outdated Show resolved Hide resolved
charts/kube-prometheus-stack/README.md Outdated Show resolved Hide resolved
@Allex1
Copy link
Contributor Author

Allex1 commented Sep 12, 2022

@monotek @mrueg seems this needs some more work/testing.
Before we finish this can you guys pls have a look at #2430 and #2429
To me it would make sense to merge them first.

@Allex1 Allex1 force-pushed the BUMP branch 3 times, most recently from 948026e to d52d0aa Compare September 12, 2022 21:24
@monotek
Copy link
Member

monotek commented Sep 13, 2022

@monotek @mrueg seems this needs some more work/testing. Before we finish this can you guys pls have a look at #2430 and #2429 To me it would make sense to merge them first.

the mentioned prs are added.

i've created #2453 to be able to use "releaseLabel: true" in node-exporter to get the needed labels added.

charts/kube-prometheus-stack/Chart.lock Outdated Show resolved Hide resolved
@monotek monotek changed the title [kube-prometheus-stack] prometheus-operator0.59.1/Prometheus2.37.1/all dependencies [kube-prometheus-stack] prometheus-operator0.59.1/Prometheus2.38.0/all dependencies Sep 13, 2022
@monotek monotek force-pushed the BUMP branch 4 times, most recently from ed6f04a to 8561234 Compare September 13, 2022 17:24
Copy link
Member

@monotek monotek left a comment

Choose a reason for hiding this comment

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

i've added some changes to the readme, updated some more image tags, removed some unneeded comments, updated node exporter to 4.2.0 and added the release label to the values. please check if update works as described.

@Allex1
Copy link
Contributor Author

Allex1 commented Sep 13, 2022

@monotek I hope to find some time and test the upgrade in the following days.

monotek
monotek previously approved these changes Sep 13, 2022
@monotek
Copy link
Member

monotek commented Sep 13, 2022

update in a kind cluster from 39.0.0 worked for me.
had to use --force-conflicts on some crds though.

@mrueg
do you see any issues?

@Allex1
Copy link
Contributor Author

Allex1 commented Sep 13, 2022

update in a kind cluster from 39.0.0 worked for me. had to use --force-conflicts on some crds though.

@mrueg do you see any issues?

Tested as well in kind and confirmed it's working fine. Should we add the --force-conflicts flag in the upgrade docs ?

@monotek
Copy link
Member

monotek commented Sep 13, 2022

update in a kind cluster from 39.0.0 worked for me. had to use --force-conflicts on some crds though.
@mrueg do you see any issues?

Tested as well in kind and confirmed it's working fine. Should we add the --force-conflicts flag in the upgrade docs ?

Not sure. Maybe it's unwanted to overwrite directly. Problem should have existed before and we don't added it too.

@monotek monotek merged commit 857fe28 into prometheus-community:main Sep 14, 2022
@totoroot
Copy link

i've added some changes to the readme, updated some more image tags, removed some unneeded comments, updated node exporter to 4.2.0 and added the release label to the values. please check if update works as described.

@monotek With your changes added to this PR you basically just removed comments which were added with PR #2411, because you deemed them to be not useful although @moroviintaas and @Xtigyro disagreed.

The chart's values.yaml has over 3000 LOC and I agree that this might not be ideal, but removing comments other consider helpful isn't the way to go IMO. I'd personally wish for changes like that to be done more transparently.

stamzid pushed a commit to Unstructured-IO/prometheus-community-helm-charts that referenced this pull request Mar 3, 2023
Matiasmct pushed a commit to giffgaff/prometheus-charts-backup that referenced this pull request May 16, 2023
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.

4 participants