Bug 1768616: Allow gating RoutePage extensions by feature flags

[vojtechszocs]

Follow up to #3066 (comment) the RoutePage extension now supports gating by Console feature flags.

Entry modules of following plugins were modified to utilize this feature:

• ceph-storage-plugin (cc @cloudbehl)
• kubevirt-plugin (cc @mareklibra)
• metal3-plugin (cc @jtomasek)
• network-attachment-definition-plugin (cc @pcbailey)
• noobaa-storage-plugin (cc @afreen23)

Entry module of operator-lifecycle-manager (cc @alecmerdler) now uses declared consumed extension types (the as operator in TypeScript is a "force" cast and should be avoided if possible).

@jtomasek Can you please verify this with existing Metal3 plugin routes?

[openshift-ci-robot]

@vojtechszocs: This pull request references Bugzilla bug 1768616, 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.

In response to this:

Bug 1768616: Allow gating RoutePage extensions by feature flags

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.

[vojtechszocs]

cc @spadgett

[spadgett]

/assign

[pcbailey]

@vojtechszocs I don't see any problems with the changes made to the network attachment definitions plugin.

[afreen23]

Ack @vojtechszocs

[jtomasek]

I can confirm that metal3-plugin routes are no more applied on cloud based cluster (AWS). On bare metal cluster the routes are applied as expected.

[jtomasek]

/test e2e-gcp-console

[spadgett]

/retest

[spadgett]

/lgtm

[vojtechszocs]

Thanks everyone for the review, FYI the second commit 2d48790 removes React.memo usage from AppContents component.

@christianvogt Are you OK with that? The effective change is:

- // use `withRouter` to force a re-render when routes change since we are using React.memo
const AppContents = connect((state: RootState) => ({
- activePerspective: getActivePerspective(state),
+ pageRouteExtensions: getPageRouteExtensions(state),
- }))(
-   withRouter(
-     React.memo(({ activePerspective }) => (
+ }))(({ pageRouteExtensions }) => (

[christianvogt]

Won't this cause a re-render here on every state change since getPageRouteExtensions returns a new array.

Specifically I forgot the use case for using memo. But I believe this was all related to performance to reduce unnecessary re-renders.

@vojtechszocs Did you do any comparisons?

Edit note: the memo and withRouter was added before the use of connect. It was used to prevent unnecessary re-renders.

[christianvogt]

I did some comparisons. It doesn't look like we're worse than before.
However we have regressed (even without this change) from the original performance fix here: #1558

[vojtechszocs]

Won't this cause a re-render here on every state change since getPageRouteExtensions returns a new array.

That's correct 😞

plugins.registry
  .getRoutePages()
  .filter((e) => plugins.registry.isExtensionInUse(e, flags))

yields a new array, due to filter in getRoutePages(). However, the content of such array should stay the same in case of state changes that don't effect RoutePage extensions, since we're operating on the same extension objects (this.extensions in ExtensionRegistry).

I'll update the code to ensure we don't re-render unless RoutePage extension required feature flags change.

@vojtechszocs Did you do any comparisons?

I'll update the code and will do some comparisons.

I did some comparisons. It doesn't look like we're worse than before.
However we have regressed (even without this change) from the original performance fix here: #1558

Maybe we can open a bug to track it so we don't forget about it.

[spadgett]

/retest

[spadgett]

/hold
as @christianvogt has review comments

[vojtechszocs]

@christianvogt @spadgett

I've split AppContents_ (actual component) vs. AppContents (Redux-connected HOC) while using connectToFlags to ensure a re-render when feature flags used to gate RoutePage extensions are changed.

This component split is very similar to existing DefaultPage_ vs. DefaultPage.

Since connectToFlags injects new flags object upon every state change, we can't React.memo on that. We could React.memo on activePerspective but I don't think it changes that frequently to justify its memoization, right?

Let me know what you think.

[spadgett]

Since connectToFlags injects new flags object upon every state change, we can't React.memo on that.

I think you can based on the discussion above, #3246 (review). React.memo will do a shallow comparison of each object in props, which should work here.

[christianvogt]

You don't need memo because react-redux connect uses the pure option by default which internally uses memo.

Latest change to the connect args looks better now.

[christianvogt]

/lgtm

[christianvogt]

/hold cancel

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, jtomasek, spadgett, vojtechszocs

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 [christianvogt,spadgett,vojtechszocs]

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

[christianvogt]

/retest

[openshift-ci-robot]

@vojtechszocs: All pull requests linked via external trackers have merged. Bugzilla bug 1768616 has been moved to the MODIFIED state.

In response to this:

Bug 1768616: Allow gating RoutePage extensions by feature flags

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.