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

Replicate annotations #313

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Replicate annotations #313

wants to merge 2 commits into from

Conversation

fiksn
Copy link

@fiksn fiksn commented Nov 14, 2023

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. So
the logic is to skip all annotations with that prefix.

There is also a new replicator.v1.mittwald.de/strip-annotations annotation added using which kubernetes-replicator behaves the same way as before. Without that it copies everything (beside the mentioned "internal annotations").

Fixes #286

@oubeichen
Copy link

+1

@pbikki
Copy link

pbikki commented May 31, 2024

any plans on merging this ? we need this feature as well

Copy link
Contributor

@stippi2 stippi2 left a 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.

replicate/common/consts.go Outdated Show resolved Hide resolved
replicate/common/consts.go Outdated Show resolved Hide resolved
replicate/common/common.go Outdated Show resolved Hide resolved
copy := make(map[string]string, len(val))

strip, ok := val[StripAnnotations]
if !ok || strings.ToLower(strip) != "true" {
Copy link
Contributor

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?

Copy link
Contributor

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.

@fiksn
Copy link
Author

fiksn commented Sep 18, 2024

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.

fiksn and others added 2 commits October 7, 2024 12:51
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.

ServiceAccount replication doesn't preserve annotations
4 participants