Add extension for list-page filters #1693

mareklibra · 2019-06-11T12:19:46Z

A plug-in can newly register custom list-page filters.

First consumer of this feature is kubevirt for listing virtual machines (#1689).

openshift-ci-robot · 2019-06-11T12:19:55Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mareklibra
To complete the pull request process, please assign benjaminapetersen
You can assign the PR to them by writing /assign @benjaminapetersen in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

frontend/OWNERS

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

openshift-ci-robot · 2019-06-11T12:20:01Z

Hi @mareklibra. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

mareklibra · 2019-06-11T12:23:39Z

Cc @vojtechszocs

spadgett · 2019-06-11T19:10:48Z

This will need to be updated for #1465

vojtechszocs · 2019-06-14T18:07:21Z

frontend/packages/console-plugin-sdk/src/typings/pages.ts

-  }>;
+  }> & {
+    /** Function. If "true" is returned then particular item is included in the list. */
+    filter?: (filterGroups: ResourceListFilterGroups, obj: K8sResourceKind) => boolean;


@mareklibra If I understand correctly, this PR is effectively about extending the listFilters object declared in public/components/factory/list.tsx, which is used to filter List component's rendered data.

The ResourceListPage extension makes Console aware of additional list pages, mapping model to page component to handle listing for that model. The implementation of the entire list page component is provided by the plugin.

Therefore, I think a better approach is to make the List component itself extensible in terms of list filters used.

ListInnerProps already includes filters, this value is used to lookup the corresponding filter impl. within the listFilters object. So I'd suggest doing the following:

Step 1, improve typing of list filters. In the List component:

add ListFilterGroups type (the Resource prefix is not needed)

add type: type ListFilter = (groups: ListFilterGroups, obj: K8sResourceKind) => boolean;

update listFilters object to use ListFilter type consistently, for example:

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

adapt ListFilterGroups type if necessary (add properties like values etc.)

Step 2, allow extension of built-in list filters. In the List component:

in ListInnerProps, add customFilterFuncs?: {[name: string]: ListFilter};

in stateToProps

add customFilterFuncs = {} to props object destructuring assignment

change getFilteredRows(allFilters, data); to getFilteredRows(allFilters, data, customFilterFuncs);

in getFilteredRows

const filter = listFilters[name] || customFilterFuncs[name];

in filterPropType

if (key in listFilters || key in customFilterFuncs || key === 'loadTest') { continue; }

Step 3, use the newly introduced customFilterFuncs prop of List component in VirtualMachinesPage.

This way, we have a List component that supports using custom filters in addition to ones that are built-in.

cc @christianvogt @spadgett

Note that the List component is deprecated and soon to be removed! We should be making these changes to table.tsx.

Agreed, I'll look into this.

What we actually need here is adding new prop to Table component (successor of List) that allows us to extend the built-in tableFilters.

(This is unrelated to resource list page mapping extension point.)

spadgett · 2019-06-14T18:40:52Z

/hold

christianvogt · 2019-06-16T20:20:16Z

frontend/public/components/factory/list.tsx

+    .forEach(rlp => {
+      if (listFilters[rlp.properties.model.kind]) {
+        // eslint-disable-next-line no-console
+        console.warn(`List filter for ${rlp.properties.model.kind} already defined, skipping.`);


Move this check to a unit test.
fyi @vojtechszocs

Yeah, this is exactly the purpose of #1708 - run extension checks as part of yarn test while keeping the runtime browser console log cleaner.

christianvogt · 2019-06-16T20:20:52Z

frontend/public/components/factory/list.tsx

+    });
+
+  // eslint-disable-next-line no-console
+  console.info(`${Object.keys(listFilters).length - baseListCount} new list filters added by plugins.`);


Is this really necessary?

This is in-line with a former convention to inform users about extensions being applied & any potential conflicts.

With #1708 we no longer need this because

conflicts are detected via extension checks (test phase)

reporting on various extensions is meant to be done via plugin-stats (printed as part of the Console webpack build)

christianvogt · 2019-06-16T20:23:37Z

frontend/packages/console-plugin-sdk/src/typings/pages.ts

-  }>;
+  }> & {
+    /** Function. If "true" is returned then particular item is included in the list. */
+    filter?: (filterGroups: ResourceListFilterGroups, obj: K8sResourceKind) => boolean;


Shouldn't the filter be named and allow more than one?

filters?: { [key: string]: (filterGroups: ResourceListFilterGroups, obj: K8sResourceKind) => boolean };

christianvogt · 2019-06-16T20:26:03Z

Dev console needs a solution here as well in order. I added something very similar as part of this PR: #1721

christianvogt · 2019-06-17T14:19:44Z

@mareklibra are you still working on this?

vojtechszocs · 2019-06-17T17:41:54Z

After talking with @christianvogt I wrote a separate PR #1735.

@mareklibra, please have a look at that one.

spadgett · 2019-06-17T19:31:29Z

Replaced by #1735

/close

openshift-ci-robot · 2019-06-17T19:31:31Z

@spadgett: Closed this PR.

Details

In response to this:

Replaced by #1735

/close

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.

Add extension for list-page filters

29bea82

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 11, 2019

openshift-ci-robot requested review from benjaminapetersen and christianvogt June 11, 2019 12:20

mareklibra mentioned this pull request Jun 11, 2019

kubevirt: Add Status column to VmList #1689

Merged

7 tasks

vojtechszocs suggested changes Jun 14, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 14, 2019

spadgett added this to the v4.2 milestone Jun 14, 2019

spadgett mentioned this pull request Jun 15, 2019

Dev console #1721

Merged

christianvogt reviewed Jun 16, 2019

View reviewed changes

openshift-ci-robot closed this Jun 17, 2019

Add extension for list-page filters #1693

Add extension for list-page filters #1693

Uh oh!

Conversation

mareklibra commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Jun 11, 2019

Uh oh!

openshift-ci-robot commented Jun 11, 2019

Uh oh!

mareklibra commented Jun 11, 2019

Uh oh!

spadgett commented Jun 11, 2019

Uh oh!

vojtechszocs Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

spadgett Jun 14, 2019

Choose a reason for hiding this comment

Uh oh!

vojtechszocs Jun 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

spadgett commented Jun 14, 2019

Uh oh!

christianvogt Jun 16, 2019

Choose a reason for hiding this comment

Uh oh!

vojtechszocs Jun 17, 2019

Choose a reason for hiding this comment

Uh oh!

christianvogt Jun 16, 2019

Choose a reason for hiding this comment

Uh oh!

vojtechszocs Jun 17, 2019

Choose a reason for hiding this comment

Uh oh!

christianvogt Jun 16, 2019

Choose a reason for hiding this comment

Uh oh!

christianvogt commented Jun 16, 2019

Uh oh!

christianvogt commented Jun 17, 2019

Uh oh!

vojtechszocs commented Jun 17, 2019

Uh oh!

spadgett commented Jun 17, 2019

Uh oh!

openshift-ci-robot commented Jun 17, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mareklibra commented Jun 11, 2019 •

edited

Loading

vojtechszocs Jun 17, 2019 •

edited

Loading