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

Jump to bottom

✨ pkg/client/options.go: MatchingSelector can add a fields.Selector to ListOptions #578

Merged
merged 1 commit into from
Sep 9, 2019
Merged

✨ pkg/client/options.go: MatchingSelector can add a fields.Selector to ListOptions #578

merged 1 commit into from
Sep 9, 2019

Conversation

estroz
Copy link
Contributor

@estroz estroz commented Aug 26, 2019

MatchingSelector provides the full fields.Selector API for listing/deleting resources. See #576 for more details.

Closes #576

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 26, 2019
@k8s-ci-robot k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Aug 26, 2019
DirectXMan12
DirectXMan12 suggested changes Sep 3, 2019
View reviewed changes
pkg/client/options.go Outdated Show resolved Hide resolved
joelanford
joelanford reviewed Sep 4, 2019
View reviewed changes
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

Since there's also a labels.Selector, the naming of this type is a bit ambiguous.

Should we add both MatchingFieldsSelector and MatchingLabelsSelector?

pkg/client/options.go Outdated Show resolved Hide resolved
@DirectXMan12
Copy link
Contributor

MatchingLabelSelector SGTM

@estroz
pkg/client/options.go: MatchingFieldsSelector and MatchingLabelsSelector
940797e 
are wrapper types for fields.Selector and labels.Selector, respectively,
implementing ListOption and DeleteAllOfOption
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 9, 2019
@DirectXMan12
Copy link
Contributor

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 9, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DirectXMan12, estroz

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 9, 2019
@k8s-ci-robot k8s-ci-robot merged commit 6993406 into kubernetes-sigs:master Sep 9, 2019
@estroz estroz deleted the options-selector branch November 14, 2019 19:46
@estroz estroz mentioned this pull request Dec 6, 2019
Closed
6 tasks
DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020
@k8s-ci-robot
Merge pull request #578 from droot/running/tools_1.13.1
64ddaf7 
🏃 updating tools to k8s 1.13.1
@estroz estroz mentioned this pull request May 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DirectXMan12 DirectXMan12 DirectXMan12 requested changes

@joelanford joelanford joelanford left review comments

@droot droot Awaiting requested review from droot

Assignees

@DirectXMan12 DirectXMan12

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

client.MatchingFields should be a fields.Selector
4 participants
@estroz @DirectXMan12 @k8s-ci-robot @joelanford