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

Filter on resources that support get & delete #486

Merged
merged 1 commit into from
May 15, 2018
Merged

Conversation

nrb
Copy link
Contributor

@nrb nrb commented May 14, 2018

Some resources use GET for listing, which resulted in errors.

Fixes #475

Signed-off-by: Nolan Brubaker [email protected]

@nrb nrb added the Bug label May 14, 2018
@nrb nrb requested a review from ncdc May 14, 2018 16:15
@@ -109,7 +109,7 @@ func (h *helper) Refresh() error {

h.resources = discovery.FilteredBy(
discovery.ResourcePredicateFunc(func(groupVersion string, r *metav1.APIResource) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

@nrb if you wanted to, you could extract this into a separate function, and then unit test it by providing different APIResources and ensuring that it returns true if and only if the resource supports all 4 verbs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would make me a bit more confident in the change. I'll look into this.

Copy link
Contributor

@skriss skriss left a comment

Choose a reason for hiding this comment

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

LGTM.

expected bool
}{
{
name: "resource that supports list, create, get, delete",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you do 1 more that has this same set of 4 but in a different order? Also please have at least 1 test where there are additional Verbs such as update, patch, and deletecollection.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Some resources use GET for listing, which resulted in errors.

Signed-off-by: Nolan Brubaker <[email protected]>
@ncdc
Copy link
Contributor

ncdc commented May 15, 2018

LGTM

@ncdc ncdc merged commit 67263d2 into vmware-tanzu:master May 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants