-
Notifications
You must be signed in to change notification settings - Fork 510
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
fix(opentelemetry-operator): remove default resource values #1457
base: main
Are you sure you want to change the base?
Conversation
@Starefossen if you're not using this chart as a subchart you can do:
But we can follow the same pattern/reasoning as #745 |
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Starefossen please include an entry in UPGRADING.md like https://github.com/open-telemetry/opentelemetry-helm-charts/blob/main/charts/opentelemetry-collector/UPGRADING.md#0531-to-0540
Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Starefossen lets also add a warning in NOTES.txt like the collector chart:
opentelemetry-helm-charts/charts/opentelemetry-collector/templates/NOTES.txt
Lines 39 to 41 in 50fb31f
{{- if not .Values.resources }} | |
[WARNING] No resource limits or requests were set. Consider setter resource requests and limits for your collector(s) via the `resources` field. | |
{{ end }} |
Having default values makes it impossible to remove the CPU limits since the default values will always be merged in with anything set by the user. Setting CPU limits are considered an anti pattern ref. https://komodor.com/learn/kubernetes-cpu-limits-throttling/