Skip to content
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 #36766 - Set disks to amount of disks instead of bytes #10750

Merged
merged 1 commit into from
Oct 12, 2023

Conversation

chris1984
Copy link
Member

@chris1984 chris1984 commented Sep 20, 2023

What are the changes introduced in this pull request?

  • We had the number of disks using bytes instead of an actual count
  • I fixed the count by looking at the block devices and filtering out the cdrom or sr0

Considerations taken when implementing this change?

  • https://projects.theforeman.org/issues/36344 is assigned to someone from the Foreman team who will add the facts from Puppet.
  • Since we are fixing the total disks, I kept this pr doing the same thing instead of taking over the 36344 card.

What are the testing steps for this pull request?

  • Apply PR
  • Register a client
  • Check to see if it reports the correct disk counts

Before PR: https://community.theforeman.org/t/hosts-ram-details-buggy/35050/7?u=cintrix84

After PR:

prdisk

@chris1984 chris1984 changed the title Fixes #36766 -Set disks to amount of disks instead of bytes Fixes #36766 - Set disks to amount of disks instead of bytes Sep 20, 2023
@chris1984
Copy link
Member Author

@sbernhard How does this look? I took a different approach since there is another redmine that someone else is working on for adding puppet support. I just altered the disks to be the correct total like we had before but with a different method of getting the disks.

The girl working on the puppet support will update this card in the future

@sbernhard
Copy link
Member

Maybe a more generic question: is the amount of disks a system has really interesting? Or should it just list the size of the storage?

@chris1984
Copy link
Member Author

Maybe a more generic question: is the amount of disks a system has really interesting? Or should it just list the size of the storage?

I think having both would be beneficial, I will do the total disks and the other Redmine is being looked at for adding disk info from puppet_facts.

@chris1984 chris1984 closed this Sep 25, 2023
@sbernhard
Copy link
Member

What's the reason to close this PR @chris1984 ?
I think we should still try to fix this isuse.

@ekohl
Copy link
Member

ekohl commented Sep 26, 2023

Thinking more about it: it would probably still be a valid bug fix cherry pick.

@chris1984 chris1984 reopened this Sep 26, 2023
@sbernhard
Copy link
Member

Are the failed tests related to this change? Would be good to finish this soonish.

@chris1984
Copy link
Member Author

It looks like they just didn't run. Let me rekick it. @ekohl do you think this is fine as it is or want me to apply more filtering logic to this commit as suggested in the comment I made?

@chris1984
Copy link
Member Author

@sbernhard tests are green if you want to test it

@sbernhard
Copy link
Member

@sbernhard tests are green if you want to test it

I tested it together with my suggestion and it worked:

image

Copy link
Member

@sbernhard sbernhard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Works as expected.

@chris1984
Copy link
Member Author

Awesome, thanks for testing it. Will get you change in and pushed up so we can get it in.

@chris1984
Copy link
Member Author

@sbernhard pushed up your fix, will put this on the review board for someone with merge access to ack it so it can be merged

@chris1984
Copy link
Member Author

[test katello]

@chris1984
Copy link
Member Author

@jeremylenz how does this look?

Copy link
Member

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 👍

@chris1984 chris1984 merged commit 85c310b into Katello:master Oct 12, 2023
5 checks passed
@chris1984 chris1984 deleted the fix-disks branch October 12, 2023 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants