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
Baremetal Node overview #4971
Baremetal Node overview #4971
Conversation
d965890
to
5f18045
Compare
31ef8a0
to
d03adee
Compare
d03adee
to
f33ca81
Compare
@@ -140,7 +140,7 @@ const BareMetalHostDetails: React.FC<BareMetalHostDetailsProps> = ({ | |||
<dl> | |||
<dt>Status</dt> | |||
<dd> | |||
<BareMetalHostStatus {...status} /> | |||
<BareMetalHostStatus {...status} maintenance={nodeMaintenance} host={host} /> |
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.
Is this change necessary? The {...status}
already includes maintenance or host. See host-status.ts
@@ -64,4 +70,8 @@ const BareMetalHostStatus: React.FC<StatusProps> = ({ status, title, description | |||
} | |||
}; | |||
|
|||
type BareMetalHostStatusProps = StatusProps & { |
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.
I think we should keep the StatusProps in tact and specify it further here. The host should be optional as it is not necessary in all states
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.
OK, but I removed the [key: string]: any
from StatusProps
which basically removes any type-checking :) and added optional nodeMaintenance
to both BMN and BMH statuses
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.
hmm, the goal of that was that status object has status, title, description and any amount of additional data which can be used by the status components so it does not have to be passed there explicitly and it is already included in the status object. With this in place, you can just do <BareMetalHostStatus {...status} /> without explicitly passing host and maintenance objects because they are already included in status object.
@@ -93,7 +93,7 @@ const HostsTableRow: RowFunction<BareMetalHostBundle> = ({ | |||
/> | |||
</TableData> | |||
<TableData className={tableColumnClasses.status}> | |||
<BareMetalHostStatus {...status} /> | |||
<BareMetalHostStatus {...status} maintenance={nodeMaintenance} host={host} /> |
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.
maintenance and (or) host are provided through {...status}
maintenance?: K8sResourceKind; | ||
className?: string; |
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.
I wouldn't add these two, this defines general shape of the status where it has status, title, description and a set of optional props which are provided as needed. These can be further specified in 'BareMetalHostStatusProps' and 'BareMetalNodeStatusProps'
d0ed0d8
to
c19f776
Compare
@@ -64,4 +70,8 @@ const BareMetalHostStatus: React.FC<StatusProps> = ({ status, title, description | |||
} | |||
}; | |||
|
|||
type BareMetalHostStatusProps = StatusProps & { |
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.
hmm, the goal of that was that status object has status, title, description and any amount of additional data which can be used by the status components so it does not have to be passed there explicitly and it is already included in the status object. With this in place, you can just do <BareMetalHostStatus {...status} /> without explicitly passing host and maintenance objects because they are already included in status object.
@@ -93,7 +93,7 @@ const HostsTableRow: RowFunction<BareMetalHostBundle> = ({ | |||
/> | |||
</TableData> | |||
<TableData className={tableColumnClasses.status}> | |||
<BareMetalHostStatus {...status} /> | |||
<BareMetalHostStatus {...status} nodeMaintenance={nodeMaintenance} host={host} /> |
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.
nodeMaintenance and host are already included in status object, so no need to pass them explicitly here.
@@ -140,7 +140,7 @@ const BareMetalHostDetails: React.FC<BareMetalHostDetailsProps> = ({ | |||
<dl> | |||
<dt>Status</dt> | |||
<dd> | |||
<BareMetalHostStatus {...status} /> | |||
<BareMetalHostStatus {...status} nodeMaintenance={nodeMaintenance} host={host} /> |
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.
no need to add these as they are already included in status object
aaec165
to
d9c2b37
Compare
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.
/lgtm
d9c2b37
to
b7ed53f
Compare
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jtomasek, rawagner 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 |
/retest |
1 similar comment
/retest |
Depends on #4911