-
Notifications
You must be signed in to change notification settings - Fork 981
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
Fixes #37478 - Add host power status column to new All Hosts page #10171
Fixes #37478 - Add host power status column to new All Hosts page #10171
Conversation
@@ -10,6 +10,7 @@ class PowerStatus < Base | |||
|
|||
def power_state(timeout = DEFAULT_TIMEOUT) | |||
result = { id: host.id }.merge(HOST_POWER[:na]) | |||
return random_power_state |
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 added this commit to make testing easier. Will remove it once acked. You can comment out this line to switch back to returning the real power status.
const green = '#6ec664'; | ||
const disabledGray = '#d2d2d2'; | ||
switch (state) { | ||
case 'on': | ||
powerIcon = ( | ||
<span style={{ ...moveItALittleUp, color: green }}> | ||
<PowerOnIcon title={tooltipText} /> | ||
</span> | ||
); | ||
break; | ||
case 'off': | ||
powerIcon = ( | ||
<span style={{ ...moveItALittleUp }}> | ||
<PowerOffIcon title={tooltipText} /> | ||
</span> | ||
); | ||
break; | ||
default: | ||
powerIcon = ( | ||
<span style={{ ...moveItALittleUp, color: disabledGray }}> | ||
<UnknownIcon title={tooltipText} /> | ||
</span> | ||
); | ||
} |
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.
Please use icon status instead of hardcoding colors https://v4-archive.patternfly.org/v4/components/icon#status-colors
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.
The old page has colored power icons. The PF4 status icons aren't power icons so it would be confusing to use, for example, a "success" icon to mean power on.
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.
Can you use pf css variables to do that? I mostly dont want just string colors hashes in the code
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.
@MariaAga Updated to use PF css variables. 👍
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.
Working great! Tested using a VMWare compute resource. The power status turned on and off between page reloads after changing the power from the host details page. Unknown power status is working as well.
Not approving yet since the test power state is still in there.
c793317
to
730ab04
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.
Looking good to me now. I can only assume those test failures about kickstart template rendering are unrelated.
This adds a Power Status column to the new All Hosts page.
Since there is no bulk endpoint for power status, I decided to use the existing API endpoint
/hosts/power
.per_page
API requests at a time. This seems reasonable to me.I added a fake delay and random power status for ease of testing. This will wait 1-4 seconds and give you a random power status back. This way you can see what it looks like when requests come in at different times.
The old page had red for powered off, but I decided to go with black here; it seems a bit less harsh.
I used the PF4-recommended icons for denoting "powered on," "powered off," and "unknown state."