-
Notifications
You must be signed in to change notification settings - Fork 18
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 list resources by label selector method to syncer interfaces #684
Conversation
🤖 Created branch: z_pr684/tpantelis/list_by_selector |
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.
You need to fix your IDE’s import settings ;-).
It's useful for users to query resources by selector filter rather than list all resources and filter afterwards. The new method does not return an error, as ListResources does, to simplify usage. There are 2 error cases, neither of which is recoverable by the caller: - cache.WaitForCacheSync returns false. This means the informer was stopped so log a warning and return an empty list. - conversion from Unstructured to the resource type which is a programming error and shouldn't happen so use r.convertNoError. In fact, we really should just panic but that can be done in a separate PR. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
} | ||
|
||
Expect(actual).To(HaveLen(len(expSpecs))) | ||
for _, obj := range actual { |
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.
pkg/syncer/resource_syncer_test.go:972:2: ranges should only be cuddled with assignments used in the iteration (wsl)
for _, obj := range actual {
^
https://github.com/submariner-io/admiral/actions/runs/5600444498/jobs/10242837481?pr=684
The internal implementation of ListResources can no longer return an error so modify the signature accordingly to simplify usage. Signed-off-by: Tom Pantelis <tompantelis@gmail.com>
9b69b4c
to
7b545b1
Compare
yeah my regular laptop is being repaired so I'm using my RH laptop and things aren't fully setup. |
@skitt @dfarrell07 I made some further changes which seems to have crossed with your reviews. Basically I removed error from the ListResources* methods to simplify usage. |
🤖 Closed branches: [z_pr684/tpantelis/list_by_selector] |
It's useful for users to query resources by selector filter rather than list all resources and filter afterwards.
The new method does not return an error, as
ListResources
does, to simplify usage. There are 2 error cases, neither of which is recoverable by the caller:cache.WaitForCacheSync
returns false. This means the informer was stopped so log a warning and return an empty list.Unstructured
to the resource type failed which is a programming error and shouldn't happen so user.convertNoError
. In fact, we really should just panic but that can be done in a separate PR.The new method is called by
ListResources
which now can no longer return an error so error was removed from the return signature.