Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add list page for network attachment definition #2545

Merged
merged 1 commit into from
Oct 16, 2019
Merged

Add list page for network attachment definition #2545

merged 1 commit into from
Oct 16, 2019

Conversation

pcbailey
Copy link
Contributor

@pcbailey pcbailey commented Aug 30, 2019 •
edited
Loading

This PR adds the list page for network attachment definitions.

Note: This does relocate the network attachment definition model from the kubevirt package to the share package.

Empty state:
Network Attachment Definitions · OKD - Google Chrome_605

Populated list:
Network Attachment Definitions · OKD - Google Chrome_601

@openshift-ci-robot openshift-ci-robot added component/core Related to console core functionality component/kubevirt Related to kubevirt-plugin component/shared Related to console-shared size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 30, 2019
@pcbailey
Copy link
Contributor Author

@rawagner Could you please review?

@pcbailey pcbailey force-pushed the add-network-attachment-definition-list-page branch from 70bb4d2 to 61667dd Compare September 3, 2019 20:58
@pcbailey pcbailey force-pushed the add-network-attachment-definition-list-page branch 2 times, most recently from 689f8aa to ad8de9f Compare September 5, 2019 13:26
@pcbailey pcbailey mentioned this pull request Sep 13, 2019
Merged
@pcbailey pcbailey force-pushed the add-network-attachment-definition-list-page branch 2 times, most recently from c850c29 to 64457b5 Compare September 19, 2019 12:59
@pcbailey pcbailey force-pushed the add-network-attachment-definition-list-page branch 3 times, most recently from 333dcdf to f33d49a Compare October 1, 2019 12:52
atiratree
atiratree suggested changes Oct 1, 2019
View reviewed changes
...ition-plugin/src/components/network-attachment-definitions/network-attachment-definition.tsx Outdated Show resolved Hide resolved
...ition-plugin/src/components/network-attachment-definitions/network-attachment-definition.tsx Outdated
<ResourceLink kind={NamespaceModel.kind} name={namespace} title={namespace} />
</TableData>
<TableData className={tableColumnClasses[2]}>
{config.type || <span className="text-secondary">Not available</span>}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please make a <NotAvailable /> component?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. But as this is already used on other multiple places, I suggest to do it consistently in a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. I'll make note to take care of this in a follow-up PR.

...ition-plugin/src/components/network-attachment-definitions/network-attachment-definition.tsx Outdated Show resolved Hide resolved
...ition-plugin/src/components/network-attachment-definitions/network-attachment-definition.tsx Outdated Show resolved Hide resolved
frontend/packages/network-attachment-definition-plugin/src/plugin.ts Show resolved Hide resolved
mareklibra
mareklibra suggested changes Oct 2, 2019
View reviewed changes
frontend/packages/kubevirt-plugin/src/components/create-vm-wizard/create-vm-wizard.tsx Outdated Show resolved Hide resolved
...ition-plugin/src/components/network-attachment-definitions/network-attachment-definition.tsx Outdated
<ResourceLink kind={NamespaceModel.kind} name={namespace} title={namespace} />
</TableData>
<TableData className={tableColumnClasses[2]}>
{config.type || <span className="text-secondary">Not available</span>}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. But as this is already used on other multiple places, I suggest to do it consistently in a follow-up.

frontend/packages/network-attachment-definition-plugin/src/models/index.ts Show resolved Hide resolved
frontend/packages/network-attachment-definition-plugin/src/types/index.ts Outdated Show resolved Hide resolved
frontend/packages/network-attachment-definition-plugin/src/utils/index.ts Outdated Show resolved Hide resolved
@pcbailey pcbailey force-pushed the add-network-attachment-definition-list-page branch 2 times, most recently from 0efca37 to ad47bb8 Compare October 2, 2019 18:06
@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. component/ceph Related to ceph-storage-plugin component/dashboard Related to dashboard component/metal3 Related to metal3-plugin component/noobaa Related to noobaa-storage-plugin component/sdk Related to console-plugin-sdk and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 2, 2019
@atiratree
Copy link
Member

I think there is a commit which doesn't belong to this PR (Add Status card)

@pcbailey pcbailey force-pushed the add-network-attachment-definition-list-page branch from 013a06c to 7b0af49 Compare October 10, 2019 14:00
@pcbailey
Copy link
Contributor Author

/retest

@mareklibra
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2019
@pcbailey pcbailey force-pushed the add-network-attachment-definition-list-page branch from 7b0af49 to fe2a608 Compare October 11, 2019 19:40
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2019
@pcbailey
Copy link
Contributor Author

/retest

@pcbailey pcbailey force-pushed the add-network-attachment-definition-list-page branch from fe2a608 to fb2094e Compare October 14, 2019 11:02
vojtechszocs
vojtechszocs suggested changes Oct 15, 2019
View reviewed changes
frontend/packages/console-shared/src/models/index.ts Outdated
@@ -0,0 +1,13 @@
import { K8sKind } from '@console/internal/module/k8s';

export const NetworkAttachmentDefinitionModel: K8sKind = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please git-move packages/console-shared/src/models/index.ts to packages/network-attachment-definition-plugin/src/models/index.ts.

@pcbailey pcbailey force-pushed the add-network-attachment-definition-list-page branch from fb2094e to 03c5a1d Compare October 15, 2019 18:46
@vojtechszocs
Copy link
Contributor

/lgtm

We should improve the imports, instead of the following forms

'@console/shared/src'
'@console/shared/src/foo/bar'

just import from

'@console/shared'

but this can be done as a follow-up.

@openshift-ci-robot 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. and removed lgtm Indicates that a PR is ready to be merged. labels Oct 15, 2019
@pcbailey pcbailey force-pushed the add-network-attachment-definition-list-page branch from 81b68cf to 2a7cb7d Compare October 15, 2019 22:24
@pcbailey
Copy link
Contributor Author

Since it removed the LGTM label due to the conflicts caused by f2fd107 merging first, I went ahead and fixed the imports as you suggested, so no follow-up needed. =)

@pcbailey
Copy link
Contributor Author

/retest

@jelkosz
Copy link

jelkosz commented Oct 16, 2019

checked that Vojtech's comment has been addressed, returning the lgtm

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 16, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jelkosz, mareklibra, pcbailey, suomiy, 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:

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

@openshift-merge-robot openshift-merge-robot merged commit 8af3819 into openshift:master Oct 16, 2019
@spadgett spadgett added this to the v4.3 milestone Oct 16, 2019
@pcbailey pcbailey deleted the add-network-attachment-definition-list-page branch October 26, 2022 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewcarleton matthewcarleton matthewcarleton left review comments

@rawagner rawagner rawagner requested changes

@vojtechszocs vojtechszocs vojtechszocs requested changes

@mareklibra mareklibra mareklibra requested changes

@atiratree atiratree atiratree approved these changes

Assignees

@vojtechszocs vojtechszocs

@jelkosz jelkosz

@mareklibra mareklibra

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/dashboard Related to dashboard component/kubevirt Related to kubevirt-plugin component/metal3 Related to metal3-plugin component/noobaa Related to noobaa-storage-plugin component/sdk Related to console-plugin-sdk component/shared Related to console-shared lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v4.3
Development

Successfully merging this pull request may close these issues.
10 participants
@pcbailey @atiratree @mareklibra @vojtechszocs @jelkosz @openshift-ci-robot @matthewcarleton @rawagner @spadgett @openshift-merge-robot