-
Notifications
You must be signed in to change notification settings - Fork 680
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
bring removeduplicates to plugin #892
Conversation
I think the deletiion of duplicates.go and duplicates_test.go are missing. With them moved to the framework better to delete from |
/ok-to-test |
/cc @ingvagabund |
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.
Thank You @JaneLiuL. I have found only few nits. Also as mentioned by @knelasevero it would be great to remove both
https://github.com/kubernetes-sigs/descheduler/blob/master/pkg/descheduler/strategies/duplicates.go and https://github.com/kubernetes-sigs/descheduler/blob/master/pkg/descheduler/strategies/duplicates_test.go in the same commit. So we can see the difference of both files and compare it with the new plugin files.
/retest |
/assign @ingvagabund |
pkg/framework/plugins/removeduplicates/removeduplicates_test.go
Outdated
Show resolved
Hide resolved
pkg/framework/plugins/removeduplicates/removeduplicates_test.go
Outdated
Show resolved
Hide resolved
@JaneLiuL few more nits which are more of a cosmetic nature. I will leave the lgtm part for @a7i wrt. #892 (comment) comment. /approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ingvagabund The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
thanks @ingvagabund may i update at next pr? i will bring the toomayrestarts plugin tonight after this pr merge. |
@ingvagabund would you help to have a look for this pr? :) |
/lgtm |
@JaneLiuL Thank You for your patience |
No description provided.