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

[prometheus-node-exporter] feat: allow changing the default revisionHistoryLimit #3677

Merged
merged 1 commit into from Aug 7, 2023
Merged

[prometheus-node-exporter] feat: allow changing the default revisionHistoryLimit #3677

merged 1 commit into from Aug 7, 2023

Conversation

tberreis
Copy link
Contributor

@tberreis tberreis commented Aug 6, 2023

What this PR does / why we need it

You may want to clean up old ReplicaSets so that these don't appear in metrics, OPA violations, ArgoCD UI and so on.
Therefore you want to overwrite the Kubernetes default revision history limit.

The Prometheus Node Exporter DaemonSet has been adapted to allow the user changing the default revisionHistoryLimit.

Which issue this PR fixes

N/A

Special notes for your reviewer

N/A

Checklist

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

Signed-off-by: Thomas Berreis <thomas@berreis.de>
Copy link
Member

@zeritti zeritti left a comment

Choose a reason for hiding this comment

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

Thank you, @tberreis, for your PR.

@zeritti zeritti merged commit 86e1fbc into prometheus-community:main Aug 7, 2023
4 checks passed
@tberreis tberreis deleted the prometheus-node-exporter branch August 8, 2023 04:09
Matiasmct pushed a commit to giffgaff/prometheus-charts that referenced this pull request Aug 25, 2023
@rouke-broersma
Copy link

@tberreis fyi we are running a custom admission controller that sets the revision history limit to 0 for all deployments etc in the cluster. It would have been better if instead of forcing the revision history limit to 10 you would have made it so the revision history limit is only set if set by the user. Now we are getting sync-loops in argocd until we set the revision history limit to 0 manually in the values. Not a big deal but setting the default in the chart is kind of a breaking change and it's not really neccesary.

@tberreis
Copy link
Contributor Author

tberreis commented Sep 4, 2023

@tberreis fyi we are running a custom admission controller that sets the revision history limit to 0 for all deployments etc in the cluster. It would have been better if instead of forcing the revision history limit to 10 you would have made it so the revision history limit is only set if set by the user. Now we are getting sync-loops in argocd until we set the revision history limit to 0 manually in the values. Not a big deal but setting the default in the chart is kind of a breaking change and it's not really neccesary.

Totally agree! That's how I started with this topic but on some repositories I had to remove the conditional (see kubereboot/charts#47 (review) for example).
Therefore I decided to use the Kubernetes defaults here. Sorry to hear that it broke your customization.

@rouke-broersma
Copy link

It's not a big deal and I appreciate that someone is going around making the revision history limit configurable 😁

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.

None yet

3 participants