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

Remove biased annotations from prometheus.service.annotations #158

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

den-is
Copy link
Contributor

@den-is den-is commented Dec 28, 2023

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:

  • I have bumped the chart version according to versioning.
  • I have updated the chart changelog with all the changes that come with this pull request according to changelog.
  • Any new values are backwards compatible and/or have sensible default.
  • I have signed off all my commits as required by DCO.

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.

Signed-off-by: Denis Iskandarov <d.iskandarov@gmail.com>
@hagaibarel hagaibarel self-assigned this Dec 28, 2023
@hagaibarel
Copy link
Collaborator

hagaibarel commented Dec 28, 2023

This would potentially break exiting installations that rely on this values being set by default

@den-is
Copy link
Contributor Author

den-is commented Dec 28, 2023

@hagaibarel correct.
well, c'est la vie.
But this is easy to fix for anyone.
Currently I'm kinda fixing my setup for myself too, by providing null and I could ignore the "issues" completely.
I just wanted to make this chart for the community better.
Otherwise for how long are you going to haul this """not-that-smart""" non-standard piece in values.yaml?

@hagaibarel
Copy link
Collaborator

c'est la vie

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).

Otherwise for how long are you going to haul this """not-that-smart""" non-standard piece in values.yaml?

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

@den-is
Copy link
Contributor Author

den-is commented Dec 28, 2023

added comment to the issue. I can bump chart version to 2.0.0

@hagaibarel
Copy link
Collaborator

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.

@den-is
Copy link
Contributor Author

den-is commented Dec 28, 2023

The override option is not functioning 100% as one would think by default and is not covering all cases.

@hagaibarel
Copy link
Collaborator

Can you provide a real world use case from your experience where overriding didn't address the issue?

@den-is
Copy link
Contributor Author

den-is commented Dec 28, 2023

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.
Real world fact is that maps in Helm are not overridden but merged.

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.

Prometheus.service.annotations in the default values.yaml should be unbiased
2 participants