Bug 1823253: Fix Textfilter issue in Filter Toolbar

[bipuladh]

• textFilter was not passed to applyFilter.

[openshift-ci-robot]

@bipuladh: This pull request references Bugzilla bug 1823253, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

• bug is open, matching expected state (open)
• bug target release (4.5.0) matches configured target release for branch (4.5.0)
• bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1823253: Fix Textfilter issue in Filter Toolbar

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[bipuladh]

/kind bug
/assign @TheRealJon

[TheRealJon]

@bipuladh Just tested this and I am still getting runtime errors if I pass a filter parameter in the url:

http://localhost:9000/k8s/cluster/config.openshift.io~v1~OperatorHub/cluster/sources?name=test

Or if I try and filter on labels

[bipuladh]

@TheRealJon Updated the PR. However, the fix was not straightforward since the structure of the payload used in catalog-source(source -> metadata -> labels) is different. I looked at files that use textFilter to perform filtering in a different manner, however, I did not come across anything that might need to use the fix that I have applied here. Please let me know if I am missing out on any such components. Thanks. :)

[TheRealJon]

I think an easier solution for the name filter problem would be to just change the default name filter in public/components/factory/table-filters.ts to something like this:

  name: (filter, obj) => fuzzyCaseInsensitive(filter, obj?.metadata?.name ?? obj?.name),

There are a few other custom name filters defined in table filters because of a similar issue where the name property isn't in metadata. Maybe we can add a TODO or FIXME comment to those to say that the default name filter will work for these resources after this update.

[bipuladh]

I think an easier solution for the name filter problem would be to just change the default name filter in public/components/factory/table-filters.ts to something like this:

  name: (filter, obj) => fuzzyCaseInsensitive(filter, obj?.metadata?.name ?? obj?.name),

There are a few other custom name filters defined in table filters because of a similar issue where the name property isn't in metadata. Maybe we can add a TODO or FIXME comment to those to say that the default name filter will work for these resources after this update.

This suggestion would allow us to remove the usage of catalog-source-name but there are other table-filters that filter on other properties that are not directly under those two fields. For example projects, they use annotation to store the display name shown to the user, so the solution you’ve proposed will work for catalogSource but not for everything, textFilter is unavoidable.
But we can update it to your suggestion so that we can avoid table-filters which share the logic you proposed.

[TheRealJon]

Yeah, I don't think that this suggestion would be a catch-all fix, but I think it would be good to have it handle the two most common scenarios (obj.name and obj.metadata.name). That might make it possible to remove at least 3 custom table filters. (catalog-source-name, alert-rule-name, and silence-name)

[TheRealJon]

Just want to update the getLabelsAsString function to take labels instead of the whole obj.

[TheRealJon]

/approve

[TheRealJon]

/lgtm

[bipuladh]

/assign @spadgett

[spadgett]

/hold

[spadgett]

This seems like the wrong approach to me. We shouldn't have magical fallbacks when the resource is not a k8s resource. We should instead provide a way to pass custom functions to the list page for getting the name or label when the resource doesn't have a metadata stanza.

We might just disable the name/label filters on this page. You should never have that many catalog sources. I'm not sure they're really helpful.

[spadgett]

Sorry I realize you're getting different feedback on the PR, but I disagree a bit with @TheRealJon here. If it's not a k8s resource, I'd rather be explicit and fix it generally so we can handle any resource shape.

But again we might just remove the filters from this page as the easy fix.

[spadgett]

Another approach would be to pass through the path to the labels or name as a prop with defaults like we do for ResoruceSummary:

https://github.com/openshift/console/blob/master/frontend/public/components/utils/details-page.tsx#L44-L45

[bipuladh]

This seems like the wrong approach to me. We shouldn't have magical fallbacks when the resource is not a k8s resource. We should instead provide a way to pass custom functions to the list page for getting the name or label when the resource doesn't have a metadata stanza.

We might just disable the name/label filters on this page. You should never have that many catalog sources. I'm not sure they're really helpful.

@spadgett
I and @TheRealJon discussed a few approaches.

1. Make changes to the data (via flatten function) such that the data has metadata available similar to any other K8sResource.
2. Add custom Name and Label filters via table-filters.
3. Use the approach used above.

We chose 3 since obj.metadata.name and obj.name are the most common selectors that are used in the table filters at some point in the future we could remove those redundant table-filters that are declared. (1) has a bit more overhead.

[bipuladh]

Sorry I realize you're getting different feedback on the PR, but I disagree a bit with @TheRealJon here. If it's not a k8s resource, I'd rather be explicit and fix it generally so we can handle any resource shape.

But again we might just remove the filters from this page as the easy fix.

We could disable the label filter. But name filter already has a custom table-filter and the fix would be very minimal.

[spadgett]

Sorry, I'm strongly against (1) and (3). obj.name and obj.labels should not be common at all. That should be very rare, and we should be explicit when it's not a normal k8s object. We should also not create "fake" metadata stanzas for things that are not k8s resources.

The right way to fix this is to either make the filter function customizable or to pass the fields paths through as optional properties.

[spadgett]

Put another way, the filter toolbar should have no knowledge of the special object created by the catalog source list. This creates tight coupling between these components. Filter toolbar should only know about k8s objects.

[TheRealJon]

@spadgett @bipuladh I would rather see the customizable filter function than passing an extra path argument, but I'll defer to @spadgett here. The original changes just didn't seem quite right to me, so I was trying to work through some alternative approaches. But I see your point about tight coupling between the filter toolbar and list page item data structure.

[TheRealJon]

As long as @spadgett is good with removing the label filter on this page, that's fine with me.

/lgtm

[spadgett]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bipuladh, spadgett, TheRealJon

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:

• frontend/OWNERS [spadgett]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[bipuladh]

/hold cancel
cc @spadgett @TheRealJon

[openshift-ci-robot]

@bipuladh: All pull requests linked via external trackers have merged: openshift/console#5022. Bugzilla bug 1823253 has been moved to the MODIFIED state.

In response to this:

Bug 1823253: Fix Textfilter issue in Filter Toolbar

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.