Skip to content

[metal3] Add details card to baremetal host dashboard #2553

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
Merged
openshift-merge-robot merged 1 commit into from
Sep 19, 2019
Merged

[metal3] Add details card to baremetal host dashboard #2553

openshift-merge-robot merged 1 commit into from
Sep 19, 2019

Conversation

@honza
Copy link
Member

@honza honza commented Aug 30, 2019 •
edited
Loading

Depends on #2520

@openshift-ci-robot openshift-ci-robot added component/core Related to console core functionality size/M Denotes a PR that changes 30-99 lines, ignoring generated files. component/metal3 Related to metal3-plugin labels Aug 30, 2019
@honza honza requested review from knowncitizen and rawagner August 30, 2019 19:53
rawagner
rawagner suggested changes Sep 2, 2019
View reviewed changes
Copy link
Contributor

@rawagner rawagner left a comment

Choose a reason for hiding this comment

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

Depends on #2520 being merged first and we also need to target this to 4.3 branch, once it's available

jtomasek
jtomasek suggested changes Sep 2, 2019
View reviewed changes
Copy link

@jtomasek jtomasek left a comment

Choose a reason for hiding this comment

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

If the goal is to eventually replace the 'Overview' page with the dashboard, The details card should hold much more information - everything which is in overview page and not present in other cards (e.g. inventory card). Is the details card going to scale well to carry that amount of information?
The same concern applies to the fact that the intention is also to eventually merge dashboards across several resources (Host/Node).

@andybraren any comments, please?

frontend/packages/metal3-plugin/src/components/dashboard/details-card.tsx Outdated
<DetailsBody>
<DetailItem key="node-name" title="Node name" value={nodeName} isLoading={false} />
<DetailItem key="host-role" title="Role" value={hostRole} isLoading={false} />
<DetailItem key="machine-cell" title="Machine" value={machineCell} isLoading={false} />
Copy link

@jtomasek jtomasek Sep 2, 2019

Choose a reason for hiding this comment

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

To align with relatively recent decision to link to a Node instead of Machine from the host, this should reference a Node using NodeCell

Copy link
Member Author

@honza honza Sep 3, 2019

Choose a reason for hiding this comment

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

🤦‍♂️

@andybraren
Copy link
Contributor

andybraren commented Sep 2, 2019

If the goal is to eventually replace the 'Overview' page with the dashboard, The details card should hold much more information - everything which is in overview page and not present in other cards (e.g. inventory card).

This was the original plan, but we later learned that some Overview pages have tables and other info from the YAML that would make combining the two tabs tricky (among other issues).

We mocked up some larger Details cards, but...

Is the details card going to scale well to carry that amount of information?

Yeah we ran into this :)

Instead, resource dashboard Details cards should contain the same fields that the resource’s List view does (same columns) in the same order. Clicking “View all” in the top-right of the card should then take the user to the Overview tab to see everything.

The same concern applies to the fact that the intention is also to eventually merge dashboards across several resources (Host/Node).

For now we’ll continue to follow the guideline above, even with bare metal Nodes, and see if that’s sufficient. A refined dashboard design for that is coming soon.

@jtomasek
Copy link

jtomasek commented Sep 2, 2019

For now we’ll continue to follow the guideline above

Thanks for clarification, sounds good to me.

@honza
Copy link
Member Author

honza commented Sep 2, 2019 •
edited
Loading

I was using this image for reference.

image

@openshift-ci-robot openshift-ci-robot added component/backend Related to backend component/ceph Related to ceph-storage-plugin component/dev-console Related to dev-console needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. component/knative Related to knative-plugin component/kubevirt Related to kubevirt-plugin component/monitoring Related to monitoring component/noobaa Related to noobaa-storage-plugin component/olm Related to OLM component/sdk Related to console-plugin-sdk component/shared Related to console-shared size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 3, 2019
@honza
Copy link
Member Author

honza commented Sep 3, 2019

Rebasing on top of #2520 seems to have worked out well...

@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 3, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2019
@knowncitizen
Copy link
Contributor

knowncitizen commented Sep 5, 2019

/approve
/test e2e-aws-console

@spadgett
Copy link
Member

spadgett commented Sep 5, 2019

/hold
until we branch for 4.3

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 5, 2019
@spadgett spadgett removed component/backend Related to backend component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/dev-console Related to dev-console component/knative Related to knative-plugin component/kubevirt Related to kubevirt-plugin component/monitoring Related to monitoring component/noobaa Related to noobaa-storage-plugin component/olm Related to OLM component/sdk Related to console-plugin-sdk component/shared Related to console-shared labels Sep 13, 2019
@honza honza changed the base branch from master to master-4.3 September 17, 2019 14:04
@honza
Copy link
Member Author

honza commented Sep 17, 2019

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 17, 2019
@honza
Copy link
Member Author

honza commented Sep 19, 2019

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

jtomasek
jtomasek approved these changes Sep 19, 2019
View reviewed changes
frontend/packages/metal3-plugin/src/components/dashboard/details-card.tsx
const hostName = getName(obj);
const namespace = getNamespace(obj);
const nodeCell = <NodeCell nodeName={hostName} namespace={namespace} />;
const hostRole = <BaremetalHostRole machine={machine} />;
Copy link

@jtomasek jtomasek Sep 19, 2019

Choose a reason for hiding this comment

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

Nit: these two could go directly into the return jsx

@jtomasek
Copy link

jtomasek commented Sep 19, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 19, 2019
@jtomasek
Copy link

jtomasek commented Sep 19, 2019

/test e2e-aws-console-olm

@openshift-bot
Copy link
Contributor

openshift-bot commented Sep 19, 2019

/retest

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

@knowncitizen
Copy link
Contributor

knowncitizen commented Sep 19, 2019

/lgtm
/test e2e-aws-console-olm

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Sep 19, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: honza, jtomasek, knowncitizen

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:

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 b915afd into openshift:master-4.3 Sep 19, 2019
@spadgett spadgett added this to the v4.3 milestone Sep 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alecmerdler alecmerdler Awaiting requested review from alecmerdler

@knowncitizen knowncitizen Awaiting requested review from knowncitizen

2 more reviewers

@rawagner rawagner rawagner requested changes

@jtomasek jtomasek jtomasek approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@knowncitizen knowncitizen

@jtomasek jtomasek

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. component/metal3 Related to metal3-plugin lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

v4.3

Development

Successfully merging this pull request may close these issues.

9 participants

@honza @andybraren @jtomasek @knowncitizen @spadgett @openshift-bot @openshift-ci-robot @rawagner @openshift-merge-robot