Create hook for k8sGet #3439
Create hook for k8sGet #3439
Conversation
openshift-ci-robot
added
component/ceph
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
rawagner
force-pushed
the
k8s_get_hook
branch
from
a70a1eb to
2443192
Compare
There was a problem hiding this comment.
I'd expect this to always return a single resource and we'd have useListK8s for lists. I might call it useK8sGet to be consistent with k8sGet. We probably want K8sResourceCommon as the type here.
| export const useGetK8s = <R extends K8sResourceKind | K8sResourceKind[] = K8sResourceKind>( | |
| export const useK8sGet = <R extends K8sResourceCommon = K8sResourceKind>( |
There was a problem hiding this comment.
K8sResourceCommon is probably better but its incompatible with K8sResourceKind.
K8sResourceCommon has all props required but K8sResourceKind has all props from K8sResourceCommon optional. It seems to me that K8sResourceKind should extend K8sResourceCommon (added to this PR) but I wonder if the apiVersion, kind and metadata should be optional or required ? So far I've marked them as optional.
Naming updated per your suggestion
There was a problem hiding this comment.
I think it makes sense for K8sResourceKind to extend K8sResourceCommon.
Should the default be K8sResourceCommon? I'd like to start getting away from K8sResourceKind since it doesn't guard against property name typos due to [key: string]: any;
https://github.com/openshift/console/blob/master/frontend/public/module/k8s/index.ts#L123
There was a problem hiding this comment.
makes sense, changed the default to K8sResourceCommon
rawagner
force-pushed
the
k8s_get_hook
branch
from
2443192 to
6b84454
Compare
openshift-ci-robot
added
size/L
rawagner
force-pushed
the
k8s_get_hook
branch
from
6b84454 to
4196239
Compare
rawagner
force-pushed
the
k8s_get_hook
branch
from
4196239 to
06b372a
Compare
| export const getKind = <A extends K8sResourceCommon = K8sResourceCommon>(value: A) => | ||
| _.get(value, 'kind') as K8sResourceCommon['kind']; | ||
| export const getOwnerReferences = <A extends K8sResourceCommon = K8sResourceCommon>(value: A) => | ||
| _.get(value, 'metadata.ownerReferences') as K8sResourceCommon['metadata']['ownerReferences']; |
There was a problem hiding this comment.
When #3747 merges, we should be able to switch to optional chaining for better type safety.
https://www.typescriptlang.org/docs/handbook/release-notes/typescript-3-7.html#optional-chaining
| setLoaded(true); | ||
| } | ||
| }; | ||
| fetch(); |
There was a problem hiding this comment.
We might consider using AbortController to cancel the request if the component unmounts as in #1775. I'm not sure offhand if it would work with k8sGet
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rawagner, spadgett The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
5 participants
No description provided.