DetailsPage - pass extra resources to PageHeading Actions

[atiratree]

It allows passing of additional resources to actions or to action creator. Needed for #1906

[atiratree]

@spadgett could you please take a look?

[vojtechszocs]

This resourceKeys prop addition seems to be the only effective change here.

[vojtechszocs]

Just curious, is resources expected to be an object? (Instead of an array?)

[atiratree]

it could be literary anything

[vojtechszocs]

Just one nit-pick and question, otherwise LGTM.

Please see #1907 (comment) - I think we can lower the complexity by augmenting & reusing the existing KebabAction type, instead of adding an alternative one.

@spadgett FYI, this PR touches core DetailsPage and PageHeading components.

[vojtechszocs]

@suomiy @rawagner @spadgett

KebabAction is actually the "creator" of KebabOption (single) object, while the proposed KebabOptionsCreator is the "creator" of KebabOption object array, reusing the same argument list.

This adds ambiguity as to which one to use, since it's either a list of functions (KebabAction[]) or a single function (KebabOptionsCreator), each of which have different return types (and therefore different signatures), and therefore need to be handled differently.

We can make this simpler, without introducing an alternative type/signature

export type KebabAction = (kind: K8sKind, obj: K8sResourceKind, resources?: any) => KebabOption | KebabOption[];

and in PageHeading component

const hasMenuActions = !_.isEmpty(menuActions); // no change here, contract stays the same
// { hasMenuActions && <ActionsMenu actions={menuActions.map(a => a(kindObj, data))} /> }
{ hasMenuActions && <ActionsMenu actions={menuActions.reduce((items: KebabOption[], action: KebabAction) => [
  ...items,
  ...(_.castArray(action(kindObj, data))),
], [])} /> }

This way, we use a single KebabAction type while supporting multiple outputs, i.e. KebabOption | KebabOption[].

[atiratree]

Yup, that would be nicer but it would not work.

The issue with this approach

• the resources are not the actual resources but are computed from the loaded data, please see for example vm status https://github.com/openshift/console/pull/1906/files#diff-e1ea60ed2e820073acdfd59753ed6636R147
• the resources are first accessible in PageHeading (please see DetailsPage)
• we have to compute something from these resources, hence the need for callback

[vojtechszocs]

Both KebabAction and KebabOptionsCreator are functions with same arguments

kind: K8sKind, obj: K8sResourceKind, resources?: any

but different return value types, KebabOption vs. KebabOption[].

The argument semantics when calling one function vs. the other one should be the same, right?

_.isFunction(menuActions)
  ? menuActions(kindObj, data, extraResources) // case A
  : menuActions.map(a => a(kindObj, data, extraResources)) // case B

• In case A, we run one function and expect KebabOption[].
• In case B, we run N functions each returning KebabOption and map result to KebabOption[].
• The discriminator is the menuActions shape, either a single function (case A) or array of functions (case B).

---

TL;DR

In both PageHeadingProps and DetailsPageProps:

-menuActions?: any[];
+menuActions?: Function[] | KebabMultiAction; // FIXME: should be "KebabAction[] | KebabMultiAction"

At the bottom of headings.tsx (this type is unrelated to kebab.tsx, we need it specifically for PageHeading):

export type KebabMultiAction = (kind: K8sKind, obj: K8sResourceKind, resources?: any) => KebabOption[];

[atiratree]

fixed the nit

[atiratree]

/test e2e-aws-console-olm
/test e2e-aws-console

[atiratree]

/test e2e-aws-console-olm

[vojtechszocs]

Both KebabAction and KebabOptionsCreator are functions with same arguments

kind: K8sKind, obj: K8sResourceKind, resources?: any

but different return value types, KebabOption vs. KebabOption[].

The argument semantics when calling one function vs. the other one should be the same, right?

_.isFunction(menuActions)
  ? menuActions(kindObj, data, extraResources) // case A
  : menuActions.map(a => a(kindObj, data, extraResources)) // case B

• In case A, we run one function and expect KebabOption[].
• In case B, we run N functions each returning KebabOption and map result to KebabOption[].
• The discriminator is the menuActions shape, either a single function (case A) or array of functions (case B).

---

TL;DR

In both PageHeadingProps and DetailsPageProps:

-menuActions?: any[];
+menuActions?: Function[] | KebabMultiAction; // FIXME: should be "KebabAction[] | KebabMultiAction"

At the bottom of headings.tsx (this type is unrelated to kebab.tsx, we need it specifically for PageHeading):

export type KebabMultiAction = (kind: K8sKind, obj: K8sResourceKind, resources?: any) => KebabOption[];

[vojtechszocs]

One more thing, TypeScript allows you to extract & reuse function parameter list, so you don't have to duplicate it:

type KebabAction = (kind: K8sKind, obj: K8sResourceKind, resources?: any) => KebabOption;
type KebabMultiAction = (...args: Parameters<KebabAction>) => KebabOption[];

[atiratree]

One more thing, TypeScript allows you to extract & reuse function parameter list, so you don't have to duplicate it:

type KebabAction = (kind: K8sKind, obj: K8sResourceKind, resources?: any) => KebabOption;
type KebabMultiAction = (...args: Parameters<KebabAction>) => KebabOption[];

good to know, changed

I am not convinced that KebabMultiAction is a good name though. It might imply that this is just one action which does multiple things.

[vojtechszocs]

I am not convinced that KebabMultiAction is a good name though. It might imply that this is just one action which does multiple things.

KebabAction itself is a creator of KebabOption objects.

KebabMultiAction was supposed to imply it's a creator of multiple (array of) KebabOption objects.

Anyway, I'm not good at naming things, it was just a suggestion.

[vojtechszocs]

As suggested in my other comment, I'd prefer to put this type next to React component that actually uses it, i.e. in public/components/utils/headings.tsx.

[atiratree]

ok, moved to headings.tsx

[atiratree]

/test e2e-aws-console-olm

[vojtechszocs]

/lgtm

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: suomiy, vojtechszocs

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• frontend/OWNERS [vojtechszocs]

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]

@suomiy: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	4fc2850	link	/test e2e-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.