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

🐛 fix labels list option in fake client #311

Merged
merged 1 commit into from
Feb 1, 2019
Merged

🐛 fix labels list option in fake client #311

merged 1 commit into from
Feb 1, 2019

Conversation

sebgl
Copy link
Contributor

@sebgl sebgl commented Jan 25, 2019 •
edited
Loading

This commit allows passing label matches to the fake client List() function.
This feature was not working properly before (no filters applied at all).

This is done in a way similar to the CachedReader. Both now share a common call to listutil.FilterWithLabels.

One thing worth discussing:

I wasn't sure where to extract this common function in the package hierarchy. Keeping it in cache/internal does not work since the fake client cannot use a package named internal. Looking at other packages I could not find the right place for it.
Hence the new cache/listutil package. It still feels a bit wrong, suggestions for a new location are very welcome!

Partial fix for #293.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 25, 2019
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 25, 2019
@DirectXMan12
Copy link
Contributor

I'd put this under pkg/internal (because it's mostly an internal detail that we don't want people depending on as part of the public, versioned API surface)

@DirectXMan12
Copy link
Contributor

otherwise LGTM

@sebgl
Copy link
Contributor Author

sebgl commented Jan 30, 2019

@DirectXMan12 thanks for looking at this. You're right, pkg/internal makes more sense here.

I also renamed the package name from listutil to objectutil. I think it leaves more room for other objects manipulation/conversion functions that may exist in the future?

@DirectXMan12
Copy link
Contributor

Looks good. Just squash the commits and I'll merge (our policy is to have "logical" commits at the end, where each commit describes a distinct piece of functionality)

@sebgl
Copy link
Contributor Author

sebgl commented Jan 31, 2019

Sure!

@sebgl sebgl force-pushed the fake-client-list-options branch from 83d6010 to bc400f2 Compare January 31, 2019 21:27
@DirectXMan12
Copy link
Contributor

hmm... did you pull in an extra commit there?

@sebgl
Enable labels list options in fake client
e9c5505 
This commit allows passing label matches to the fake client List()
function.

This is done in a way similar to the CachedReader. Both now share a
common call to `objectutil.FilterWithLabels`.

Signed-off-by: sebgl <contact.sebgl@gmail.com>
@sebgl sebgl force-pushed the fake-client-list-options branch from bc400f2 to e9c5505 Compare January 31, 2019 21:29
@sebgl
Copy link
Contributor Author

sebgl commented Jan 31, 2019 •
edited
Loading

Yes, my bad, I just rebased and pushed again. Looks better now :)

@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 Feb 1, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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 Feb 1, 2019
@k8s-ci-robot k8s-ci-robot merged commit abf0c07 into kubernetes-sigs:master Feb 1, 2019
@alvaroaleman alvaroaleman mentioned this pull request Mar 4, 2019
Closed
cynepco3hahue pushed a commit to cynepco3hahue/machine-api-operator that referenced this pull request Mar 26, 2019
Hack vendor to make label selector work under the fake client
3a84d5b 
The relevant PR(kubernetes-sigs/controller-runtime#311)
under the controller-runtime already merged,
but the whole controller-runtime master code depends on the newer k8s version that
currently prevent from us to use it.
cynepco3hahue pushed a commit to cynepco3hahue/machine-api-operator that referenced this pull request Apr 2, 2019
Hack vendor to make label selector work under the fake client
6993623 
The relevant PR(kubernetes-sigs/controller-runtime#311)
under the controller-runtime already merged,
but the whole controller-runtime master code depends on the newer k8s version that
currently prevent from us to use it.
cynepco3hahue pushed a commit to cynepco3hahue/machine-api-operator that referenced this pull request Apr 4, 2019
Hack vendor to make label selector work under the fake client
8cccc85 
The relevant PR(kubernetes-sigs/controller-runtime#311)
under the controller-runtime already merged,
but the whole controller-runtime master code depends on the newer k8s version that
currently prevent from us to use it.
cynepco3hahue pushed a commit to cynepco3hahue/machine-api-operator that referenced this pull request Apr 4, 2019
Hack vendor to make label selector work under the fake client
47bb852 
The relevant PR(kubernetes-sigs/controller-runtime#311)
under the controller-runtime already merged,
but the whole controller-runtime master code depends on the newer k8s version that
currently prevent from us to use it.
cynepco3hahue pushed a commit to cynepco3hahue/machine-api-operator that referenced this pull request Apr 7, 2019
Hack vendor to make label selector work under the fake client
25ea0da 
The relevant PR(kubernetes-sigs/controller-runtime#311)
under the controller-runtime already merged,
but the whole controller-runtime master code depends on the newer k8s version that
currently prevent from us to use it.
cynepco3hahue pushed a commit to cynepco3hahue/machine-api-operator that referenced this pull request Apr 7, 2019
UPSTREAM: <carry>: openshift: Hack vendor to make label selector work…
4e62dfa 
… under the fake client

The relevant PR(kubernetes-sigs/controller-runtime#311)
under the controller-runtime already merged,
but the whole controller-runtime master code depends on the newer k8s version that
currently prevent from us to use it.
cynepco3hahue pushed a commit to cynepco3hahue/machine-api-operator that referenced this pull request Apr 7, 2019
UPSTREAM: <carry>: openshift: Hack vendor to make label selector work…
ccf9b92 
… under the fake client

The relevant PR(kubernetes-sigs/controller-runtime#311)
under the controller-runtime already merged,
but the whole controller-runtime master code depends on the newer k8s version that
currently prevent from us to use it.
cynepco3hahue pushed a commit to cynepco3hahue/machine-api-operator that referenced this pull request Apr 21, 2019
UPSTREAM: <carry>: openshift: Hack vendor to make label selector work…
189f093 
… under the fake client

The relevant PR(kubernetes-sigs/controller-runtime#311)
under the controller-runtime already merged,
but the whole controller-runtime master code depends on the newer k8s version that
currently prevent from us to use it.
cynepco3hahue pushed a commit to cynepco3hahue/machine-api-operator that referenced this pull request Apr 21, 2019
UPSTREAM: <carry>: openshift: Hack vendor to make label selector work…
c415636 
… under the fake client

The relevant PR(kubernetes-sigs/controller-runtime#311)
under the controller-runtime already merged,
but the whole controller-runtime master code depends on the newer k8s version that
currently prevent from us to use it.
cynepco3hahue pushed a commit to cynepco3hahue/machine-api-operator that referenced this pull request Apr 23, 2019
UPSTREAM: <carry>: openshift: Hack vendor to make label selector work…
382e154 
… under the fake client

The relevant PR(kubernetes-sigs/controller-runtime#311)
under the controller-runtime already merged,
but the whole controller-runtime master code depends on the newer k8s version that
currently prevent from us to use it.
@ingvagabund ingvagabund mentioned this pull request Apr 26, 2019
Merged
cynepco3hahue pushed a commit to cynepco3hahue/machine-api-operator that referenced this pull request Apr 28, 2019
UPSTREAM: <carry>: openshift: Hack vendor to make label selector work…
351c73f 
… under the fake client

The relevant PR(kubernetes-sigs/controller-runtime#311)
under the controller-runtime already merged,
but the whole controller-runtime master code depends on the newer k8s version that
currently prevent from us to use it.
@aszecowka aszecowka mentioned this pull request Jul 2, 2019
Merged
@racevedoo racevedoo mentioned this pull request Oct 8, 2019
Merged
thbkrkr pushed a commit to elastic/cloud-on-k8s that referenced this pull request Oct 9, 2019
@racevedoo @thbkrkr
Add statefulset list test (#1936)
4909bc5 
This tests a statefulset list with an additional pod that does not belong to the sset and
check it is not returned.
This was blocked while the fake client did not support label list options.
(See kubernetes-sigs/controller-runtime#311)
@racevedoo racevedoo mentioned this pull request Oct 14, 2019
Merged
DirectXMan12 pushed a commit that referenced this pull request Jan 31, 2020
@k8s-ci-robot
Merge pull request #311 from fanzhangio/ci
d9fd836 
Speed up tests by caching deps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DirectXMan12 DirectXMan12 Awaiting requested review from DirectXMan12

@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/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@sebgl @DirectXMan12 @k8s-ci-robot