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

change all PredicateFunc to use SelectionPredicate #30529

Merged
merged 3 commits into from
Aug 20, 2016

Conversation

hongchaodeng
Copy link
Contributor

@hongchaodeng hongchaodeng commented Aug 12, 2016

What?

  • This PR changes all PredicateFunc in registry to return SelectionPredicate instead of Matcher interface.

Why?

  • We want to pass SelectionPredicate to storage layer. Matcher interface did not expose enough information for indexing.

This change is Reviewable

@hongchaodeng
Copy link
Contributor Author

ref: #29888

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Aug 12, 2016
@hongchaodeng hongchaodeng force-pushed the r1 branch 6 times, most recently from 137c704 to 1c71477 Compare August 12, 2016 20:35
@k8s-github-robot k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 14, 2016
@smarterclayton
Copy link
Contributor

registry unit test failing

@hongchaodeng
Copy link
Contributor Author

hongchaodeng commented Aug 14, 2016

@smarterclayton
Fixed. Ready for review. Thanks!

return labels.Set{}
// SelectableFields returns a field set that can be used for filter selection
func SelectableFields(obj *extensions.ThirdPartyResourceData) fields.Set {
return fields.Set{}
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be more correct to return nil here to avoid an allocation. Selectable fields is read only, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

I had some thoughts on fields set:

  • If no selectable fields, return nil
  • An object has static selectable fields. We can store it someway.

Let me do the first one.

@hongchaodeng
Copy link
Contributor Author

@smarterclayton
Addressed comments. Tests passed. PTAL.

@smarterclayton smarterclayton added lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Aug 15, 2016
@xiang90 xiang90 added this to the v1.4 milestone Aug 15, 2016
@k8s-bot
Copy link

k8s-bot commented Aug 19, 2016

GCE e2e build/test passed for commit eb516fb.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Aug 20, 2016

GCE e2e build/test passed for commit eb516fb.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 5caf74c into kubernetes:master Aug 20, 2016
@hongchaodeng hongchaodeng deleted the r1 branch August 20, 2016 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants