-
Notifications
You must be signed in to change notification settings - Fork 494
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
bugfix: fixing disableprometheusannotation casing #2664
bugfix: fixing disableprometheusannotation casing #2664
Conversation
This should be noted as a breaking change (if merged). |
@rivToadd, please replace this change from v1alpha1 to v1beta1 and use the conversion. |
+1 we cannot merge this in v1alpha1 as it is a breaking change |
@pavolloffay IMO this is an acceptable breaking change given the functionality as written will never work as intended. I would prefer we keep this as is to allow v1alpha1 users to use this feature as expected. |
I am not for nor against the change, but it should be clearly marked in the release notes as a matter of principle. Maybe there should be a versioning policy established for the operator to explain what level of stability users can expect granted the api clearly stays alpha – but so does much within the cloud native ecosystem so at some point one becomes desensitized to the meaning of the word. |
@Starefossen this will absolutely be marked as a breaking change, per our changelog guidelines. We do our best to minimize breaking changes despite our alpha status and we're in the midst of moving to a beta status CRD where breaking changes will hopefully be rare. There are features that we put behind featuregates for this purpose too. It would probably be worth defining our guidelines somewhere and we can discuss this at our next SIG meeting. |
From the SIG meeting:
|
109c0de
to
3cc85e9
Compare
f3f67df
to
9bf454e
Compare
var enableAnnotations bool | ||
// enableAnnotations is true only if both disable variables are false. | ||
// DisableAutomaticPrometheusAnnotations takes precedence over DisablePrometheusAnnotations. | ||
if instance.Spec.Observability.Metrics.DisableAutomaticPrometheusAnnotations { |
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.
you'll need to add this to the v1beta1 type too
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.
looks fixed in beta version
per my discussion with @rivToadd, this isn't needed anymore as the fix is in v1beta1 now. |
Description:
change disableprometheusannotation to lower camel case format
Link to tracking Issue(s):
#2608
observability.metrics.DisablePrometheusAnnotations
does not work #2608Testing:
Documentation: