-
Notifications
You must be signed in to change notification settings - Fork 238
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
Add indexer to fakeCilent when bumping k8s.io deps to v0.26.1 #588
Conversation
✅ Deploy Preview for kubernetes-sigs-kueue canceled.
|
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.
/lgtm
can you bump all the k8s libraries together? |
74d8f24
to
cfb0e88
Compare
cfb0e88
to
7983193
Compare
/retest |
7983193
to
54196b1
Compare
Signed-off-by: Kante Yin <kerthcet@gmail.com>
The test error was introduced in kubernetes-sigs/controller-runtime#2025, for fakeClient will check the indexes but we didn't register any in unit tests. |
But we can register these indexes when building the client. |
479cb82
to
a3f8127
Compare
Signed-off-by: Kante Yin <kerthcet@gmail.com>
a3f8127
to
774c656
Compare
Signed-off-by: Kante Yin <kerthcet@gmail.com>
) | ||
|
||
var ( | ||
IndexQueueClusterQueue = func(obj client.Object) []string { |
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.
These index functions both used in package cache
and queue
, so moved to util/indexer
.
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.
Maybe we should just have a single SetupIndexes
function for the kueue APIs.
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.
Do you mean have only one setupIndexes()
in main.go? Not separated into different packages like queue
, cache
, job
.
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.
probably not in main.go, because the tests need it. But yes, a single place.
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.
Let me make this cleanup tomorrow. We can leave this a follow up if that's ok with you.
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.
sg
Yes, the test error fixed. |
) | ||
|
||
var ( | ||
IndexQueueClusterQueue = func(obj client.Object) []string { |
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.
Maybe we should just have a single SetupIndexes
function for the kueue APIs.
Signed-off-by: Kante Yin <kerthcet@gmail.com>
All addressed except #588 (comment). |
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!
LGTM from my side.
return []string{string(q.Spec.ClusterQueue)} | ||
} | ||
|
||
IndexWorkloadQueue = func(obj client.Object) []string { |
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.
IndexWorkloadQueue = func(obj client.Object) []string { | |
func IndexWorkloadQueue(obj client.Object) []string { |
why overcomplicate things :)
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.
I agree.
@kerthcet Can you follow up with another PR?
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.
The follow up PR will just move the entire SetupIndexes functions here.
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.
sgtm.
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alculquicondor, kerthcet 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 |
/lgtm |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #586
Special notes for your reviewer:
The test error was introduced in kubernetes-sigs/controller-runtime#2025, for fakeClient will check the indexes but we didn't register any in unit tests.
But we can register these indexes when building the client.