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

fix: don't need match number for labels/annotations for service diff #2129

Merged
merged 1 commit into from
May 24, 2024

Conversation

jiuker
Copy link
Contributor

@jiuker jiuker commented May 23, 2024

don't need match number for labels/annotations for service diff
other operator will update labels or annotations to our service.
image

fix: don't need match number for labels/annotations
@jiuker jiuker changed the title fix: don't need match number for labels/annotations fix: don't need match number for labels/annotations for service diff May 23, 2024
@jiuker jiuker requested a review from vadmeste May 23, 2024 11:13
Copy link
Member

@pjuarezd pjuarezd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't remove it, we need to check if labels specified in .spec.minioServiceLabels are present, and annotations in .spec.minioServiceAnnotations are present. Removing this check is ignore if the secret has expected labels and annotations.

What are you trying to solve?, maybe there is other way to fix it @jiuker

Copy link
Contributor

@ramondeklein ramondeklein left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pjuarezd It’s still checking the labels/annotations, but it only removes the length comparison. It’s not needed, so it’s essentially a code cleanup. Initial PR description is confusing…

A unit test to ensure proper functionality is in place, so LGTM.

@pjuarezd
Copy link
Member

@pjuarezd It’s still checking the labels/annotations, but it only removes the length comparison. It’s not needed, so it’s essentially a code cleanup. Initial PR description is confusing…

A unit test to ensure proper functionality is in place, so LGTM.

I see, essentially still checks for the service to have the expected labels and annotations without preventing have other additional labels added by a third party controller or manually added.

@pjuarezd pjuarezd merged commit a57c3e0 into minio:master May 24, 2024
30 checks passed
@jiuker
Copy link
Contributor Author

jiuker commented May 24, 2024

@pjuarezd It’s still checking the labels/annotations, but it only removes the length comparison. It’s not needed, so it’s essentially a code cleanup. Initial PR description is confusing…
A unit test to ensure proper functionality is in place, so LGTM.

I see, essentially still checks for the service to have the expected labels and annotations without preventing have other additional labels added by a third party controller or manually added.

Yeah.

@TZ-zzz
Copy link

TZ-zzz commented Jun 13, 2024

Hi, I believe the code removed was added in #2095 which fixed the issue of labels of service not mtaching the expected labels when deleting labels. Could you explain why this change is no longer needed?

@jiuker
Copy link
Contributor Author

jiuker commented Jun 14, 2024

Look at the description @TZ-zzz

@TZ-zzz
Copy link

TZ-zzz commented Jun 14, 2024

Look at the description @TZ-zzz

@jiuker I have looked at description and still not sure why removing length comparison will keep service labels matching expected labels when users try to delete existing labels.

There was a bug that if user deletes existing labels from CR, the operator does not reconcile that change. The code removed in this PR was the fix for that bug.

@jiuker
Copy link
Contributor Author

jiuker commented Jun 14, 2024

Why do you want to delete them? @TZ-zzz

@TZ-zzz
Copy link

TZ-zzz commented Jun 14, 2024

@jiuker Add it by mistake. It makes sense to remove the code and allow other operators to add labels directly to the service. If this is intentional, then I have no further questions.

@jiuker
Copy link
Contributor Author

jiuker commented Jun 14, 2024

This is done because there are operators on other networks who can modify the labels and annotations of a service without permission, which is a relatively large scenario and I think it is reasonable @TZ-zzz

@TZ-zzz
Copy link

TZ-zzz commented Jun 14, 2024

@jiuker I see. Thank you for explaining.

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.

4 participants