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

bugfix: fixing disableprometheusannotation casing #2664

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

rivToadd
Copy link
Contributor

@rivToadd rivToadd commented Feb 26, 2024

Description:

change disableprometheusannotation to lower camel case format

Link to tracking Issue(s):
#2608

Testing:

Documentation:

@rivToadd rivToadd requested a review from a team as a code owner February 26, 2024 07:46
@rivToadd rivToadd closed this Feb 26, 2024
@rivToadd rivToadd reopened this Feb 26, 2024
@Starefossen
Copy link
Contributor

This should be noted as a breaking change (if merged).

@yuriolisa
Copy link
Contributor

yuriolisa commented Feb 26, 2024

@rivToadd, please replace this change from v1alpha1 to v1beta1 and use the conversion.

@pavolloffay
Copy link
Member

+1 we cannot merge this in v1alpha1 as it is a breaking change

@jaronoff97
Copy link
Contributor

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

@Starefossen
Copy link
Contributor

Starefossen commented Feb 27, 2024

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.

@jaronoff97
Copy link
Contributor

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

@jaronoff97
Copy link
Contributor

From the SIG meeting:

  • on the topic of this PR
    • what will happen when after making this CR change?
    • if the upper case doesn't work, then this is a bug fix. If not this is a breaking change
    • the base assumption is that we should never making a breaking change in the CRD
    • could we make an upgrade routine to convert from older version to newer version
    • maybe define a second one with the lowercase... (this is what we think should be done)
      • mark the old field deprecated
      • remove all references
      • add a warning for the old field
  • For CRDs
    • a breaking change cannot be made without changing the version
    • stability around operator features
      • featureflags exist, but they're not exhaustive and they exist to change behavior not really to opt-in/opt-out
      • we could document it, but it could rot pretty easily...
      • something like kubebuilder struct tags maybe?
    • we don't have stability guarantees currently, we will take this topic async.

@rivToadd rivToadd force-pushed the default-prometheus-annotation-bugfix branch from 109c0de to 3cc85e9 Compare March 1, 2024 07:38
@rivToadd rivToadd marked this pull request as draft March 1, 2024 17:51
@jaronoff97 jaronoff97 changed the title bugfix: fixing diableprometheusannotation casing bugfix: fixing disableprometheusannotation casing Mar 4, 2024
@rivToadd rivToadd force-pushed the default-prometheus-annotation-bugfix branch from f3f67df to 9bf454e Compare March 7, 2024 04:54
@rivToadd rivToadd marked this pull request as ready for review March 7, 2024 04:56
var enableAnnotations bool
// enableAnnotations is true only if both disable variables are false.
// DisableAutomaticPrometheusAnnotations takes precedence over DisablePrometheusAnnotations.
if instance.Spec.Observability.Metrics.DisableAutomaticPrometheusAnnotations {
Copy link
Contributor

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

Copy link
Contributor Author

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

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.

observability.metrics.DisablePrometheusAnnotations does not work
5 participants