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
Add no power management status and alert for BMH #5830
Add no power management status and alert for BMH #5830
Conversation
2cbebc4
to
2a75dc8
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.
That was fast, and great improvements/additions! 😄
There's some new discussion happening in MGMT-1262 about additional API changes that I still need to read through, which may affect some of this.
Nit: A period should be placed in every place after the word provided.
EDIT: I see it's correct in the code, maybe the screenshots are just a bit older?
I notice that the BMH Overview's Status card status has Status
as the primary text and Externally provisioned
as secondary. That status should probably align with the status shown in the List page, so just Externally provisioned
, right?
Details - maybe in this case the message does not have to be alert ?
Agreed, in that particular popover moving the text outside of the inline alert and putting the Add credentials
button in the footer of the popover makes sense to me.
Improved the BMH activities on overview page - wont be shown anymore if there's no power mgmt.
Nice, I've filed a somewhat-related styling issue.
Now I wonder what should we do about the New/Edit BMH dialog - BMC address/pass/username is required right now but I guess we should remove that. But if user fills in one of these values he has to fill in all - its all or nothing - how to visualize that ?
I've created MGMT-1352 with some ideas.
FYI @nirfarkas
isVisible={visible} | ||
shouldClose={close} | ||
statusBody={<StatusIconAndText title="Disabled" />} | ||
> | ||
<p> | ||
Operators provided by this source will not appear in OperatorHub and any operators installed | ||
from this source will not receive updates unitl this source is re-enabled. |
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.
from this source will not receive updates unitl this source is re-enabled. | |
from this source will not receive updates until this source is re-enabled. |
I guess not in scope for this PR, but spelling error here.
I knew you would notice it :) These are older screenshots.
Right, I will align those.
+1
Thanks I will take a look.
|
044cc01
to
ad14c50
Compare
@andybraren @jtomasek |
|
Reading through baremetal-operator/#569 and thinking about this more with @nirfarkas, maybe this @dhellmann @beekhof Are there any potentially serious/annoying limitations of I know Machine Health Checks wouldn't work, but what about upgrades? Would we be able to upgrade a host without its BMC credentials? If not, that seems like something we'd want to bring to users' attention in a much more visible way, and nudge the user more strongly toward providing those credentials as soon as they can. |
Upgrades happen in place and don't go through the machine API or use metal3, so they'll work fine without credentials. We won't be able to hard-reboot the host (when/if we have soft reboot the remediation code may be able to use that). It will be possible to end up with an unmanaged host outside of the assisted installer by creating the host resource without the BMC details. If that happens, the host will not be associated with a node, won't have hardware details, and the user won't be able to provision it until they add the credentials. It seems like it would be worth using a different representation to indicate that we don't have full control. I'm not sure a "warning" icon is necessary, though. Maybe a question mark? Or just a solid circle without the check mark in it? Something that's a gentle reminder that they could do more to get more. |
I like the info mark. We need to be careful not to press too hard for BMC credentials. The cluster isn't broken without them. It isn't degraded. There are just some features not in use. |
@andybraren What do you think? |
c1aa7ef
to
af773dc
Compare
I updated the code with info icon, since I like that one too. @andybraren let us know if you have any objections. |
Let's proceed with an info icon for now and we'll work with the other OpenShift designers to see if we can figure out a better one (or maybe even none at all?) for this pretty unique case. I believe this is the first time we'd be using this info icon as a status icon which is why I'm a little hesitant, but I'd rather not hold up the rest of this PR's fixes longer for something that can be easily changed later. Since it sounds like a user could leave hosts in this state for months with no impact on upgrades or anything potentially dangerous, maybe it is a "checkmark" state after all, but more like a "mostly happy grey checkmark" instead of green if we can do a color-only tweak. I realize that a checkmark doesn't quite match up to a noun like "Unmanaged" which sounds like a negative thing, however. We'll follow up. |
icon={<RebootingIcon />} | ||
/> | ||
{!hasPowerManagement(host) ? ( | ||
<SecondaryStatus status={HOST_NO_POWER_MGMT} /> |
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.
Shouldn't this be StatusIconAndText
? SecondaryStatus is used as an additional info for Status
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 wanted to show No power management
as grayed out text which SecondaryStatus
provides. I could change that to StatusIconAndText
and add className
. But I dont really see that as better solution.
} | ||
|
||
return hostStatus; |
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'd prefer returning from the cases instead of setting 'hostStatus' variable and breaking. No pressure, though :)
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.
fixed
}; | ||
|
||
type BareMetalHostStatusProps = StatusProps & { | ||
host?: BareMetalHostKind; | ||
nodeMaintenance?: K8sResourceKind; | ||
showCredentials?: boolean; |
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 prop actually used in the component? I don't see it anywhere.
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.
leftover, removed
af773dc
to
4fc5993
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 |
List view
Overview (message in Status)
Details - maybe in this case the message does not have to be alert ?
I've also disabled power actions if there's no power management.
Improved the BMH activities on overview page - wont be shown anymore if there's no power mgmt.
Now I wonder what should we do about the New/Edit BMH dialog - BMC address/pass/username is required right now but I guess we should remove that. But if user fills in one of these values he has to fill in all - its all or nothing - how to visualize that ? - tracking in https://issues.redhat.com/browse/MGMT-1352