-
Notifications
You must be signed in to change notification settings - Fork 106
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
Replicate annotations #313
base: master
Are you sure you want to change the base?
Conversation
+1 |
any plans on merging this ? we need this feature as well |
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.
Left a suggestion for a (maybe?) better name for the prefix constant and left a comment.
copy := make(map[string]string, len(val)) | ||
|
||
strip, ok := val[StripAnnotations] | ||
if !ok || strings.ToLower(strip) != "true" { |
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.
Not sure what unexpected effects it could have to make copying the labels the default. Maybe it is safer to have to explicitly enable the copying?
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.
Just thought of another argument in favor switching the default behavior: If this PR was merged, and the replicator is updated in a cluster, nothing will happen yet (because). The labels will only be copied for resources that are updated after the new replicator is deployed. That means the behavior would be more consistent if users have to specifically activate the behavior on resources where they need it.
Thanks for the review. I have no issue with the more descriptive constant names. Perhaps I can incorporate everything in one new commit instead of clicking on the GH GUI. Regarding what the default behavior should be: I thought about it and people perceive the current behavior as a bug. Which means it should probably get changed so it will just work by default. Of course a few people might still want the old behavior, but I believe this will be a minority, and more importantly it should be a conscious decision (hence the option to do it with an annotation). If something breaks for people upgrading it will be because they were relying on a specific implementation detail which is never a good thing. |
Co-authored-by: Stephan Aßmus <[email protected]>
It appears this was delibrately omited, but there are some use-cases where you need to copy over existing annotations too.
The only problem is that if
replicator.v1.mittwald.de/*
annotations were copied this could create an infinite loop. Sothe logic is to skip all annotations with that prefix.
There is also a new
replicator.v1.mittwald.de/strip-annotations
annotation added using whichkubernetes-replicator
behaves the same way as before. Without that it copies everything (beside the mentioned "internal annotations").Fixes #286