-
Notifications
You must be signed in to change notification settings - Fork 1k
Fixes #38888 - Add Cloud Provider Details card to Host Details page #10758
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 #38888 - Add Cloud Provider Details card to Host Details page #10758
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.
...assets/javascripts/react_app/components/HostDetails/Tabs/Details/Cards/CloudBilling/index.js
Outdated
Show resolved
Hide resolved
...s/javascripts/react_app/components/HostDetails/Tabs/Details/Cards/CloudBilling/BillingAws.js
Outdated
Show resolved
Hide resolved
...javascripts/react_app/components/HostDetails/Tabs/Details/Cards/CloudBilling/BillingAzure.js
Outdated
Show resolved
Hide resolved
...s/javascripts/react_app/components/HostDetails/Tabs/Details/Cards/CloudBilling/BillingGcp.js
Outdated
Show resolved
Hide resolved
...cripts/react_app/components/HostDetails/Tabs/Details/Cards/CloudBilling/BillingDetailItem.js
Outdated
Show resolved
Hide resolved
|
@chris1984 updated 👍 |
f4c1814 to
7727c3c
Compare
ekohl
left a comment
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.
Normally we only show parsed facts that are then stored as normalized data. Looking up raw facts should IMHO be avoided. Both for compatibility (if we need to somehow modify them or parse them from multiple fact sources that present them in different ways) and performance (fact table can be large and needs joins).
It also makes it easier for others to retrieve the same info in an API call
I'd prefer to stick to that pattern.
@ekohl Does this mean you require that we persist these on some facet? Or is there some in-between state I don't know about? I thought this particular lookup was okay because the facts are already part of the API response, and it's only for a single host. |
|
IMHO that it retrieves facts now is a bug already, not something to expand on. |
|
And on persistence, yes we have a facet. But linking that on my phone is awkward |
|
I think you probably mean the reported data facet, right? Will update this PR, but probably not before next week. |
|
Yes, that is exactly what I was thinking about |
|
@jeremylenz here is a PR I made a while back for the host OS UI card adding facts into the reported_data facet if you want an example: #9460 |
3328362 to
13e30e2
Compare
|
@chris1984 Updated to use reported data facet. |
chris1984
left a comment
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.
Thanks for switching to the reported data facet, tested again on my box and I can see card coming up for hosts and in the debug console I can see it's using the reported facts. Code looks good too and the migration works properly up and down, thanks for all the test coverage :)
AWS:
Azure:
GCP:
Non cloud host:
Cloud host with facts not being reported and card not showing up as intended:
...assets/javascripts/react_app/components/HostDetails/Tabs/Details/Cards/CloudBilling/index.js
Outdated
Show resolved
Hide resolved
13e30e2 to
55223e5
Compare
|
Addressed comments (see commit messages.) |
|
@jnagare-redhat When automation is complete and you are happy, please comment here to ack my PR :) thanks! |
|
Ack |
chris1984
left a comment
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.
Tested with the latest changes and still works. Screenshots from the last test are still valid so keeping them out of the PR to reduce noise.
6ba8f70 to
d17ef20
Compare
|
squashed |
ekohl
left a comment
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.
On my phone so a partial review. Apologies for that.
I'm starting to think how it looks. In my head I'm wondering how to tackle this (without coming to an answer). Do you want a block that had a generic header "cloud provider" (or similar) or specific (like "AWS details" or Azure details")?
The other interaction I'm wondering about is compute resources. If you have it linked up, do you combine it? Might be confusing to have a block with partially data you can set and partially data that us reported. Again, on my phone so hard to check but some might even overlap.
Sorry to throw this all at you but it took me a while to wrap my head around it.
webpack/assets/javascripts/react_app/components/HostDetails/Tabs/Details/CardRegistry.js
Outdated
Show resolved
Hide resolved
...assets/javascripts/react_app/components/HostDetails/Tabs/Details/Cards/CloudBilling/index.js
Outdated
Show resolved
Hide resolved
6be4808 to
730367c
Compare
|
Updated:
@ekohl I didn't quite understand your comment about compute resources and combining editable data with read-only data. Did you mean the Virtualization card vs. this Cloud |
...p/components/HostDetails/Tabs/Details/Cards/CloudProvider/__tests__/CloudProviderAws.test.js
Outdated
Show resolved
Hide resolved
730367c to
27e3069
Compare
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
27e3069 to
b527306
Compare
|
Updated the card display logic so the absence of cloud_provider and compute_resource_provider won't prevent display of the card. We now have a three-level fallback: cloud_provider → compute_resource_provider → auto-detect from data. |
The point I tried to make: we have compute resources, which is displayed as the Now as a user it may be confusing to have it in 2 cards, each with their own data source. We've already seen the feedback in the community that the host details page is becoming messy because it's too many cards. Quoting https://community.theforeman.org/t/new-hosts-page-ui-is-getting-bad-reviews/44394/6#p-144324-host-details-page-3
And later in the summary:
So just because it came via PM doesn't mean we can't push back or thing about the implementation. Technically PM can't even force anything in the upstream community. In short, I'd like to think about the bigger picture: does adding this information actually make the overall user experience worse? |
I think it makes it better, since it gathers relevant account data into one place and focuses on the cloud provider rather than the host. Also I think it's possible to have this card and not have a Virtualization card, since that card relies on |
|
Also, IMO the time for debate about basic design should be much sooner than the final PR review stages. I'm happy to talk about it but I wish it had been earlier. (preferably before the PR is even opened.) |
ColeHiggins2
left a comment
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.
Since this has been tested twice, im happy to ack it. @ekohl you are the final approval :)
|
Also I'd like @jnagare-redhat 's final ack when automation is complete, thanks tho Cole :) |
|
Ack Automation running and raised PR |
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.
Thanks for addressing my many comments. I know I sort of steered the design quite late, but I think it's better now.
I still think we should think more about how we organize the various cards on the detail page, but this PR is not the place for that.
Edit: not pressing the merge button since it's not clear to me if @jnagare-redhat's message means this is ready to be merged or not.
|
Yes its ready to merge. |
|
Thanks! Merged. @jnagare-redhat FYI: everyone can leave a review on PRs and approve, even if they don't have merge permissions. I'd encourage you to use that. Also request changes when something is failing. |





Summary
This PR adds a new Cloud Billing Details card to the Host Details page that displays cloud provider billing information for AWS, Azure, and GCP hosts.
Features
host factsreported dataTesting Instructions
Manual Testing
Test with an AWS host:
Test with an Azure host:
Test with a GCP host:
Test with a non-cloud host:
Test with missing billing data:
🤖 Generated with Claude Code