-
Notifications
You must be signed in to change notification settings - Fork 719
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
Rename pause annotation #2783
Rename pause annotation #2783
Conversation
New eck.k8s.elastic.co/managed annotation, with inverted boolean semantics: false stops reconciliation. True is implied if not specified by the user. Legacy pause annotation is still supported for existing deployments.
Co-Authored-By: Charith Ellawala <52399125+charith-elastic@users.noreply.github.com>
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.
LGTM.
There are a couple of nits that I didn't catch the first time but those are pretty minor and can be fixed later as well.
pkg/controller/common/unmanaged.go
Outdated
managed, newExists := parse(ManagedAnnotation, true) | ||
paused, legacyExists := parse(LegacyPauseAnnoation, false) | ||
if legacyExists && !newExists { | ||
log.V(-1).Info(fmt.Sprintf("Using legacy annotation %s. Please consider moving to %s", LegacyPauseAnnoation, ManagedAnnotation), "namespace", meta.Namespace, "name", meta.Name) |
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.
We currently don't use the -1 level for anything else that I saw. I'm a little bit skeptical about starting to use it for this? I'm open to it but could use some persuading. My gut feeling is that no one reads warning messages and that they should be errors or info messages.
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.
Good point will change back to info.
This should have the |
It’s not a breaking change imo because we still support the old annotation. |
Yeah that makes sense, but I wonder what else we put it under so that it can be found in release notes or closed PRs. Is it time maybe to add support for a |
I add the |
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.
LGTM
New
eck.k8s.elastic.co/managed
annotation, with inverted booleansemantics: false stops reconciliation. True is implied if not
specified by the user.
Legacy pause annotation is still supported for existing deployments.
Fixes #2594