Add host status column #1774
Add host status column #1774
Conversation
openshift-ci-robot
added
the
size/L
There was a problem hiding this comment.
I'm confused. Why are we copying all this logic from utils/status-icon.tsx?
There was a problem hiding this comment.
This patch depends on #1697 which among other things lets plugins re-use the StatusIconAndText functionality (I've tried to cover the motivation here #1697 (comment))
|
/hold |
openshift-ci-robot
added
do-not-merge/hold
|
I've updated this PR to not depend on #1697 and used existing StatusIconAndText component to unblock this PR and work that depends on it. |
There was a problem hiding this comment.
Clever, but is this really better than...?
if (status === HOST_STATUS_DISCOVERED) {
return <AddDiscoveredHostButton host={host} />;
}
if (HOST_PROGRESS_STATES.includes(status)) {
return <StatusIconAndText status={title} iconName="refresh" />;
}
// ...There was a problem hiding this comment.
No strong opinion, but I find the switch statement a bit more readable
There was a problem hiding this comment.
Would be nice if this is sortable, but I don't think there's a way to add sortFunc from a plugin yet.
There was a problem hiding this comment.
Not new, but it would be better to add types to all of these helper functions
There was a problem hiding this comment.
Done, I'll create BaremetalHostKind for host in a separate PR.
There was a problem hiding this comment.
I think this could be inferred from the return value, but I'm not against declaring it explicitly.
|
/hold cancel |
openshift-ci-robot
added
needs-rebase
* Introduces BaremetalHostStatus component which renders appropriate status component based on host status * Introduces utility functions to determine host status (optionally also from host's machine and node)
openshift-ci-robot
removed
the
needs-rebase
|
/approve |
|
@honza: changing LGTM is restricted to collaborators DetailsIn 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. |
|
/lgtm |
|
[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 DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/test e2e-aws |
|
/test frontend |
7 participants
from host's machine and node)
Depends on #1697