shared: move lookup utils from kubevirt-plugin #1966

atiratree · 2019-07-09T14:55:54Z

used by kubevirt and newly metal3 (#1967)

atiratree · 2019-07-09T18:18:20Z

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

spadgett · 2019-07-09T19:04:30Z

frontend/packages/console-shared/src/utils/utils.ts

It's not clear to me how this is used, but it's not unique across types. Is there a reason to avoid simply using the UID?

example usage:

https://github.com/openshift/console/blob/236f5617f7fc4f3cc9b21104f72edf8bc07ba6b8/frontend/packages/kubevirt-plugin/src/components/vms/vm.tsx#L125-L128

https://github.com/openshift/console/blob/236f5617f7fc4f3cc9b21104f72edf8bc07ba6b8/frontend/packages/kubevirt-plugin/src/components/vms/vm.tsx#L80-L82

not sure what will be the most prevalent usage:

should I put the getUID function as a default to createLookup when getKey is not specified ?

+1 on favoring UID over a composite ID that may not be unique.

This seems like an unaddressed comment, unless I'm missing something.

added the UID selector by default

frontend/packages/console-shared/src/utils/utils.ts

atiratree · 2019-07-10T08:07:19Z

/test e2e-aws

vojtechszocs · 2019-07-12T13:45:35Z

frontend/packages/console-shared/src/utils/index.ts

If there's just one file, why not simply put code here instead of adding another utils.ts file?

I think the need for more utils will come as time goes on, so I think it is better to start folder for that in the beginning.

OK, fair enough.

vojtechszocs · 2019-07-12T13:57:39Z

frontend/packages/console-shared/src/utils/utils.ts

getLookupID fits the signature of KeyResolver, we could use it here.

Also, do we really need to go overboard with generics here?

Can't we just have K8sEntityMap working with <A extends K8sResourceKind> directly? (i.e. do we need to work with objects that aren't structurally compatible with K8sResourceKind?)

getLookupID fits the signature of KeyResolver, we could use it here.

I tried, but failed - we need the generic type A for entity and make it compatible with createLookup

Also, do we really need to go overboard with generics here?

I think it is quite useful to have the exact type returned of entity the lookup was created for (especial once we get in #1813)

Can't we just have K8sEntityMap working with directly? (i.e. do we need to work with objects that aren't structurally compatible with K8sResourceKind?)

we have many other use cases of inner lists which are not structurally compatible with K8sResourceKind, e.g. https://github.com/openshift/console/blob/master/frontend/packages/kubevirt-plugin/src/components/vm-disks/vm-disks.tsx#L63

OK, thanks for clarification.

vojtechszocs · 2019-07-12T14:02:00Z

frontend/packages/console-shared/src/utils/utils.ts

+1 on favoring UID over a composite ID that may not be unique.

vojtechszocs · 2019-07-12T14:07:42Z

frontend/packages/kubevirt-plugin/src/models/index.ts

vojtechszocs · 2019-07-15T16:49:48Z

frontend/packages/console-shared/src/utils/utils.ts

This seems like an unaddressed comment, unless I'm missing something.

vojtechszocs · 2019-07-15T16:51:07Z

frontend/packages/console-shared/src/utils/utils.ts

OK, thanks for clarification.

atiratree · 2019-07-15T19:03:14Z

frontend/packages/console-shared/src/selectors/common.ts

I had to also change signature of these selectors to be compatible with the lookup generics

Well, that's what I meant by being careful with TS generics, since it might force us to add generic type signatures to all code related to e.g. createLookup stuff.

vojtechszocs · 2019-07-15T19:55:51Z

frontend/packages/console-shared/src/selectors/common.ts

Well, that's what I meant by being careful with TS generics, since it might force us to add generic type signatures to all code related to e.g. createLookup stuff.

vojtechszocs · 2019-07-15T20:02:51Z

LGTM - I hope this also addresses Sam's comment #1966 (comment).

vojtechszocs · 2019-07-15T20:07:51Z

@suomiy When dealing with TS generics, it's always a good idea to think about generic type param fallbacks, for example:

type Foo<T> = (obj: T) => string;
// when using Foo, you *always* need to provide type param T
type Bar<T = any> = Foo<T>;
// when using Bar, you *may* provide type param T to narrow down the signature

vojtechszocs · 2019-07-15T20:08:24Z

/approve cancel

Needs Sam's look.

vojtechszocs · 2019-07-15T20:09:00Z

/lgtm

atiratree · 2019-07-15T20:14:04Z

@suomiy When dealing with TS generics, it's always a good idea to think about generic type param fallbacks, for example:
type Foo<T> = (obj: T) => string;
// when using Foo, you *always* need to provide type param T
type Bar<T = any> = Foo<T>;
// when using Bar, you *may* provide type param T to narrow down the signature

thanks, added the K8sResourceKind as a fallback

atiratree · 2019-07-16T12:29:44Z

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

atiratree · 2019-07-16T12:35:27Z

/test e2e-aws-console

vojtechszocs · 2019-07-16T15:06:59Z

/lgtm

rawagner · 2019-07-17T10:35:51Z

/lgtm

openshift-ci-robot · 2019-07-17T10:36:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rawagner, 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

atiratree · 2019-07-17T13:30:29Z

/test e2e-aws

atiratree · 2019-07-17T15:20:36Z

/test e2e-aws

atiratree · 2019-07-17T19:59:08Z

/retest

openshift-bot · 2019-07-17T20:40:54Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-ci-robot · 2019-07-17T21:01:45Z

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

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	`ae3845e`	link	`/test e2e-aws`
ci/prow/e2e-aws-console-olm	`ae3845e`	link	`/test e2e-aws-console-olm`
ci/prow/e2e-aws-console	`ae3845e`	link	`/test e2e-aws-console`

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.

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 9, 2019

openshift-ci-robot requested review from dtaylor113 and pcbailey July 9, 2019 14:56

atiratree mentioned this pull request Jul 9, 2019

metal3: add node maintenance actions #1967

Merged

3 tasks

spadgett added this to the v4.2 milestone Jul 9, 2019

spadgett reviewed Jul 9, 2019

View reviewed changes

atiratree force-pushed the lookupUtils branch 2 times, most recently from 74b4b3c to d849f27 Compare July 9, 2019 19:58

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 9, 2019

vojtechszocs suggested changes Jul 12, 2019

View reviewed changes

vojtechszocs suggested changes Jul 15, 2019

View reviewed changes

atiratree force-pushed the lookupUtils branch 2 times, most recently from c16b78e to 338a5fd Compare July 15, 2019 19:02

atiratree commented Jul 15, 2019

View reviewed changes

vojtechszocs approved these changes Jul 15, 2019

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2019

openshift-ci-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 15, 2019

openshift-ci-robot assigned vojtechszocs Jul 15, 2019

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 15, 2019

atiratree force-pushed the lookupUtils branch from 338a5fd to f1786e8 Compare July 15, 2019 20:12

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2019

atiratree force-pushed the lookupUtils branch from f1786e8 to 1895357 Compare July 16, 2019 11:31

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 16, 2019

shared: move lookup utils from kubevirt-plugin

ae3845e

atiratree force-pushed the lookupUtils branch from 1895357 to ae3845e Compare July 17, 2019 10:27

openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 17, 2019

openshift-ci-robot assigned rawagner Jul 17, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 17, 2019

openshift-merge-robot merged commit ae3845e into openshift:master Jul 17, 2019

shared: move lookup utils from kubevirt-plugin #1966

shared: move lookup utils from kubevirt-plugin #1966

Uh oh!

Conversation

atiratree commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atiratree commented Jul 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

atiratree commented Jul 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vojtechszocs commented Jul 15, 2019

Uh oh!

vojtechszocs commented Jul 15, 2019

Uh oh!

vojtechszocs commented Jul 15, 2019

Uh oh!

vojtechszocs commented Jul 15, 2019

Uh oh!

atiratree commented Jul 15, 2019

Uh oh!

atiratree commented Jul 16, 2019

Uh oh!

atiratree commented Jul 16, 2019

Uh oh!

vojtechszocs commented Jul 16, 2019

Uh oh!

rawagner commented Jul 17, 2019

Uh oh!

openshift-ci-robot commented Jul 17, 2019

Uh oh!

atiratree commented Jul 17, 2019

Uh oh!

atiratree commented Jul 17, 2019

Uh oh!

atiratree commented Jul 17, 2019

Uh oh!

openshift-bot commented Jul 17, 2019

Uh oh!

openshift-ci-robot commented Jul 17, 2019

Uh oh!

Reviewers

Assignees

atiratree commented Jul 9, 2019 •

edited

Loading