-
Notifications
You must be signed in to change notification settings - Fork 112
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
Remove biased annotations from prometheus.service.annotations #158
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Denis Iskandarov <d.iskandarov@gmail.com>
This would potentially break exiting installations that rely on this values being set by default |
@hagaibarel correct. |
This isn't my preferred way of maintaing an OSS project where we have a community and users to support. If we wanted to introduce a change that would break people's exiting installation, this should have been a major version release, not a patch (or even a minor).
Given that there isn't a "standard" in values file, it's common to provide sane defaults with an option to override / change them. Bottom line, I don't think this change has any meaningful value |
added comment to the issue. I can bump chart version to 2.0.0 |
I don't think this change justifies bumping a major version, nor has any significant value. There is an option to override the annotations if needed, and the defaults make sense and fit most use cases. |
The override option is not functioning 100% as one would think by default and is not covering all cases. |
Can you provide a real world use case from your experience where overriding didn't address the issue? |
I have provided exact example in the original issue linked to this PR. |
Why is this pull request needed and what does it do?
Makes the default prometheus.service.annotations blank
{}
map. Removing any bias.Which issues (if any) are related?
Fixes #157
Checklist:
Changes are automatically published when merged to
main
. They are not published on branches.Note on DCO
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.