Fixes #36736 - Correct memory fact on HW properties card #10737
Conversation
|
Issues: #36736 |
| @@ -43,7 +43,7 @@ const HwPropertiesCard = ({ isExpandedGlobal, hostDetails }) => { | |||
| const coreSocket = facts?.['cpu::core(s)_per_socket']; | |||
| const reportedFacts = propsToCamelCase(hostDetails?.reported_data || {}); | |||
| const totalDisks = reportedFacts?.disksTotal; | |||
| const memory = facts?.['dmi::memory::maximum_capacity']; | |||
| const memory = facts?.['dmi::memory::size']; | |||
There was a problem hiding this comment.
I think, this is wrong and should stay at maximum_capacity:
sub-man facts:
puppet facts:
free -m
3GB is still more correct than 1GB.
There was a problem hiding this comment.
What about having a max capacity field and then a system_total field?
There was a problem hiding this comment.
If the user does not have puppet facts, then I guess we just only show max capacity and rename that field?
There was a problem hiding this comment.
the question is, is it about "hardware properties" as the headline describes or hardware resources the OS can handle?
There are some other facts, e.g. from subscription manager.
Maybe just use what is already there:
https://github.com/theforeman/foreman/blob/31956e5267cc74b7e06eec73042a24505ba45c6f/app/services/katello/rhsm_fact_parser.rb#L107
or
this is what users expect I guess.
chris1984
force-pushed
the
fixramhwprop
branch
from
0f2026b to
d5b43db
Compare
|
@sbernhard fixed, let me know if this looks better? |
There was a problem hiding this comment.
If this comes from facts, why isn't this in Foreman itself? And everything should be stored as reported_data (see https://github.com/theforeman/foreman/blob/develop/app/models/host_facets/reported_data_facet.rb). Direct use of facts is something we should avoid.
| @@ -43,7 +44,7 @@ const HwPropertiesCard = ({ isExpandedGlobal, hostDetails }) => { | |||
| const coreSocket = facts?.['cpu::core(s)_per_socket']; | |||
| const reportedFacts = propsToCamelCase(hostDetails?.reported_data || {}); | |||
| const totalDisks = reportedFacts?.disksTotal; | |||
| const memory = facts?.['dmi::memory::maximum_capacity']; | |||
| const memory = facts?.['memory::memtotal']; | |||
There was a problem hiding this comment.
We have this as a reported fact called ram.
| const memory = facts?.['memory::memtotal']; | |
| const memory = reportedFacts?.ram; |
| @@ -43,7 +44,7 @@ const HwPropertiesCard = ({ isExpandedGlobal, hostDetails }) => { | |||
| const coreSocket = facts?.['cpu::core(s)_per_socket']; | |||
There was a problem hiding this comment.
I think this is reportedFacts?.cores. I can't comment on it, but above there's also reportedFacts?.sockets
So this would need a pr in Foreman then to have that fact come as a reported value then use that here? I'll see what ram is coming back with them and if it's not correct make a PR to foreman. |
|
IMHO yes, though I'd even say that this entire card should be dropped from Katello and implemented in Foreman itself. I see no reason why it would be Katello-specific, except that it uses RHSM facts directly instead of normalizing. |
| <DescriptionListDescription>{memory}</DescriptionListDescription> | ||
| <DescriptionListDescription>{NumberToHumanSize(memory*1024, { | ||
| strip_insignificant_zeros: true, precision: 2 | ||
| })}</DescriptionListDescription> | ||
| </DescriptionListGroup> | ||
| <DescriptionListGroup> | ||
| <HostDisks totalDisks={totalDisks} /> |
There was a problem hiding this comment.
I just noticed this on the screenshot. This value is the total number of bytes of all disks combined, not the number of disks.
There was a problem hiding this comment.
I have that fixed in a separate pr
What are the changes introduced in this pull request?
Considerations taken when implementing this change?
What are the testing steps for this pull request?