-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Allow for custom property comparators #1536
Allow for custom property comparators #1536
Conversation
/assign @njuettner |
I've checked this change in live application and it seems to be working |
It's rebased and ready to be reviewed |
2f88350
to
511dea3
Compare
rebased |
Fixes issue kubernetes-sigs#1463 Co-authored-by: Alastair Houghton <alastair@alastairs-place.net>
511dea3
to
f008e89
Compare
/assign @Raffo |
@tariq1890 @ytsarev Maybe you want to review this one as well? It allows any provider to define custom comparators which can help with custom properties or bugs that cause repeated updates |
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.
Thanks for the PR @sheerun , I think that's great! I added a few comments, but I think overall this is good to ship.
Did you test an image with that? We are aiming at a release of externalDNS next week and I want to make sure that this would be good to ship in case we get it in.
@Raffo I fixed your suggestions, mind to approve? :) |
/lgtm |
@sheerun: you cannot LGTM your own PR. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
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.
Sorry for nit-picking, here's a few more suggestions.
Co-authored-by: Raffaele Di Fazio <raffo@github.com>
Co-authored-by: Raffaele Di Fazio <raffo@github.com>
No problem, updated |
Restarting the tests. |
The failure seems to be a legit one @sheerun :
can you please fix it? |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Raffo, sheerun 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 |
It build on top of #1534 and fixes #1463 with the same base idea but (allow providers to define their property comparator) but also enhancements from #1534 which this PR succeeds. The main changes are:
func (string, *string, *string)
bool tofunc (string, string, string) bool
to avoid issues with improper handling of nil values (with current signature empty string is treated the same way as nil value, if there is a need to treat them differently in the future, the signature can be of course changed back).