-
Notifications
You must be signed in to change notification settings - Fork 146
rhsm: Update function call to match new Storage API #1530
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
Conversation
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.
Code Review
This pull request updates the publish_facts function to align with a new Storage API. The changes are functionally correct, but I've identified an opportunity to simplify the implementation by removing a redundant function call. This will make the code more efficient and easier to read. My suggestion is to directly use the complete return value from get_status_require_booted instead of calling get_status a second time.
crates/lib/src/rhsm.rs
Outdated
| let (booted_deployment, ..) = crate::status::get_status_require_booted(ostree)?; | ||
| let (_deployments, host) = crate::status::get_status(ostree, Some(&booted_deployment))?; |
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 get_status_require_booted function already computes and returns the host object. The subsequent call to get_status is redundant and can be removed for better performance and code clarity. You can directly destructure the tuple returned by get_status_require_booted to get the host.
| let (booted_deployment, ..) = crate::status::get_status_require_booted(ostree)?; | |
| let (_deployments, host) = crate::status::get_status(ostree, Some(&booted_deployment))?; | |
| let (_, _, host) = crate::status::get_status_require_booted(ostree)?; |
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.
Yes.
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.
Updated
|
Ah right the main CI doesn't enable the rhsm feature since we only turn it on via the spec file, and the packit builds aren't gating. |
Update according to API changes introduced in bootc-dev#1525 Signed-off-by: Johan-Liebert1 <[email protected]>
2b3cd3c to
f5d2621
Compare
➡️ #1531 should help that from one angle, but I guess we may also need to make packit gating |
No description provided.