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.

Sign up for GitHub

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

merge labels when building RB/CRB by ClusterPropagationPolicy #3239

Merged
merged 1 commit into from
Mar 13, 2023
Merged

merge labels when building RB/CRB by ClusterPropagationPolicy #3239

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg/detector/detector.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,7 +479,7 @@ func (d *ResourceDetector) ApplyClusterPolicy(object *unstructured.Unstructured,
"try again later after binding is garbage collected, see https://github.com/karmada-io/karmada/issues/2090")
}
// Just update necessary fields, especially avoid modifying Spec.Clusters which is scheduling result, if already exists.
bindingCopy.Labels = binding.Labels
bindingCopy.Labels = util.DedupeAndMergeLabels(bindingCopy.Labels, binding.Labels)
bindingCopy.OwnerReferences = binding.OwnerReferences
bindingCopy.Finalizers = binding.Finalizers
bindingCopy.Spec.Resource = binding.Spec.Resource
Expand Down Expand Up @@ -523,7 +523,7 @@ func (d *ResourceDetector) ApplyClusterPolicy(object *unstructured.Unstructured,
"try again later after binding is garbage collected, see https://github.com/karmada-io/karmada/issues/2090")
}
// Just update necessary fields, especially avoid modifying Spec.Clusters which is scheduling result, if already exists.
bindingCopy.Labels = binding.Labels
bindingCopy.Labels = util.DedupeAndMergeLabels(bindingCopy.Labels, binding.Labels)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we don't need to modify this line, because we don't currently support cluster scope resources for the dependency distribution feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion, this is not just for the dependency distribution feature, but customized labels as well.
For example, if we have a webhook which would patch some custmized info into the labels of resourcebindings to group them, after I scale the workload once, the info would be lost.
This might not be a good example, but I think even if it is a label for temporary usage, we shouldn't abandon them.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds reasonable., and it doesn't seem to hurt.

Copy link
Member

Choose a reason for hiding this comment

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

Do you think we should add some E2E to protect this case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Humm......I think it is nice to have.
I hope to help add some.
May I commit them in this PR? Or open a new one?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good to me.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks~

bindingCopy.OwnerReferences = binding.OwnerReferences
bindingCopy.Finalizers = binding.Finalizers
bindingCopy.Spec.Resource = binding.Spec.Resource
Expand Down