New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
OCPBUGS-13808: Console SDK components should be using GroupVersionKin… #12946
Conversation
@tvu20: This pull request references Jira Issue OCPBUGS-13808, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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. |
/jira refresh |
@tvu20: This pull request references Jira Issue OCPBUGS-13808, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
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. |
/retest |
1 similar comment
/retest |
The code looks good 👍 |
/retest |
1 similar comment
/retest |
…d object Fixes https://issues.redhat.com/browse/OCPBUGS-13808 Components in Console Dynamic Plugin SDK use new `K8sGroupVersionKind` object. Reference string still supported for backwards compatibility.
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job @tvu20 👍
/lgtm
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jhadvig, tvu20 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:
Approvers can indicate their approval by writing |
@tvu20: Jira Issue OCPBUGS-13808: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-13808 has been moved to the MODIFIED state. In response to this:
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry to comment on a merged PR, but there is an issue with the implementation.
const kind = typeof groupVersionKind !== 'string' ? groupVersionKind.kind : groupVersionKind; | ||
|
||
const [k8sModel] = useK8sModel(kind); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect, we need to be passing the API group and version as well to useK8sModel
. We need to disambiguate between the same kind
in different groups as well as different API versions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like useK8sModel
already takes a groupVersionKind
, so maybe we don't need to change the code here.
If so, it can still be useK8sModel(groupVersionKind)
as before
@@ -691,7 +691,7 @@ const exampleList: React.FC<MyProps> = () => { | |||
|
|||
| Parameter Name | Description | | |||
| -------------- | ----------- | | |||
| `groupVersionKind` | the resource group/version/kind to represent | | |||
| `groupVersionKind` | group, version, kind of k8s resource {@link K8sGroupVersionKind} is preferred alternatively can pass reference for group, version, kind which is deprecated i.e `group~version~kind` {@link K8sResourceKindReference}. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should document how to handle core resource like pod without a group as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Sam, I'm not sure what handle core resource like pod without a group is referring to. Can you give me an example of how to document this?
@@ -111,8 +113,8 @@ const ListPageCreate: React.FC<ListPageCreateProps> = ({ | |||
: `/k8s/cluster/${k8sModel.plural}/~new`; | |||
if (k8sModel.crd) { | |||
to = usedNamespace | |||
? `/k8s/ns/${usedNamespace || 'default'}/${groupVersionKind}/~new` | |||
: `/k8s/cluster/${groupVersionKind}/~new`; | |||
? `/k8s/ns/${usedNamespace || 'default'}/${kind}/~new` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We want to make sure we're using the "reference" string here which is ~
delimited for resources in an API group.
…d object
Fixes https://issues.redhat.com/browse/OCPBUGS-13808
Components in Console Dynamic Plugin SDK use new
K8sGroupVersionKind
object. Reference string still supported for backwards compatibility.