-
Notifications
You must be signed in to change notification settings - Fork 50
SAT-38427 - Implement CSV exports on the hosts overview #1132
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
base: develop
Are you sure you want to change the base?
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds and wires up CSV export definitions for RH Cloud-specific host table columns, including recommendations count and a new Total CVEs column, so that these values are available in hosts overview CSV exports. Sequence diagram for hosts CSV export with RH Cloud columnssequenceDiagram
actor User
participant HostsUI
participant ForemanServer
participant RHCloudPlugin
participant PageletRegistry
participant CsvExporter
participant HostModel
User->>HostsUI: Click export CSV on hosts overview
HostsUI->>ForemanServer: Request hosts CSV
ForemanServer->>PageletRegistry: Resolve hosts_table_column_header pagelets
PageletRegistry->>RHCloudPlugin: Load cloud profile pagelets
RHCloudPlugin-->>PageletRegistry: Provide insights_recommendations_count and cves_count export definitions
ForemanServer->>CsvExporter: Initialize export with column definitions
loop For each host
CsvExporter->>HostModel: Load host record
CsvExporter->>HostModel: insights_hits
HostModel-->>CsvExporter: Collection of insights_hits
CsvExporter-->>CsvExporter: Count insights_hits for insights_recommendations_count
CsvExporter->>HostModel: insights_attributes
HostModel-->>CsvExporter: Hash with cves_count
CsvExporter-->>CsvExporter: Extract cves_count via dig
end
CsvExporter-->>ForemanServer: Generated CSV file
ForemanServer-->>HostsUI: Send CSV response
HostsUI-->>User: Download CSV including Recommendations and Total CVEs columns
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- For the CSV export definitions, consider reusing the same localized label used for the column headers (
label: _('Recommendations'),label: _('Total CVEs')) instead of hardcoded English strings to keep i18n consistent between UI and exports. - The content cell for
cves_countfalls back to the string'—'; double-check that this matches the convention used elsewhere in the hosts table (e.g., nil or empty output) to avoid inconsistent placeholder values in the UI.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- For the CSV export definitions, consider reusing the same localized label used for the column headers (`label: _('Recommendations')`, `label: _('Total CVEs')`) instead of hardcoded English strings to keep i18n consistent between UI and exports.
- The content cell for `cves_count` falls back to the string `'—'`; double-check that this matches the convention used elsewhere in the hosts table (e.g., nil or empty output) to avoid inconsistent placeholder values in the UI.
## Individual Comments
### Comment 1
<location> `lib/foreman_rh_cloud/plugin.rb:145-148` </location>
<code_context>
+ export_data: CsvExporter::ExportDefinition.new(:insights_recommendations_count, label: 'Recommendations', callback: ->(host) { host&.insights_hits&.count })
</code_context>
<issue_to_address>
**suggestion:** Use consistent i18n for export labels to match UI labels
For both `:insights_recommendations_count` and `:cves_count`, the table headers use translated labels (`_('RH Cloud')`, `_('Total CVEs')`), while the CSV `label:` arguments use raw strings (`'Recommendations'`, `'Total CVEs'`). If CSV headers are user-facing, please wrap these labels in `_()` as well so UI and export localization remain consistent.
Suggested implementation:
```ruby
export_data: CsvExporter::ExportDefinition.new(:insights_recommendations_count, label: _('Recommendations'), callback: ->(host) { host&.insights_hits&.count })
```
```ruby
export_data: CsvExporter::ExportDefinition.new(:cves_count, label: _('Total CVEs'), callback: ->(host) { host&.insights_attributes&.dig('cves_count') })
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
c77983f to
5542621
Compare
jeremylenz
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 opening the PR @lfu!
lib/foreman_rh_cloud/plugin.rb
Outdated
| context.with_profile :cloud, _('RH Cloud'), default: true do | ||
| add_pagelet :hosts_table_column_header, key: :insights_recommendations_count, label: _('Recommendations'), sortable: true, width: '12%', class: 'hidden-xs ellipsis', priority: 100, | ||
| export_data: CsvExporter::ExportDefinition.new(:insights_recommendations_count, callback: ->(host) { host&.insights_hits&.count }) | ||
| export_data: CsvExporter::ExportDefinition.new(:insights_recommendations_count, label: _('Recommendations'), callback: ->(host) { host&.insights_hits&.count }) |
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.
This won't work with IoP. Insights hits are not stored in our database. Rather, we make an API call to IoP services from the frontend:
foreman_rh_cloud/webpack/ForemanColumnExtensions/index.js
Lines 31 to 32 in 083b91d
| `/insights_cloud/api/insights/v1/system/${uuid}`, | |
| { key: `HOST_RECS_COUNT_${uuid}` } |
Not sure what that means for CSV export. Perhaps you could make the same call from the backend. Or if it's too much, we could disable this column for export, but I hope we don't have to.
lib/foreman_rh_cloud/plugin.rb
Outdated
| export_data: CsvExporter::ExportDefinition.new(:insights_recommendations_count, label: _('Recommendations'), callback: ->(host) { host&.insights_hits&.count }) | ||
| add_pagelet :hosts_table_column_content, key: :insights_recommendations_count, callback: ->(host) { hits_counts_cell(host) }, class: 'hidden-xs ellipsis text-center', priority: 100 | ||
| add_pagelet :hosts_table_column_header, key: :cves_count, label: _('Total CVEs'), width: '12%', class: 'hidden-xs ellipsis', | ||
| export_data: CsvExporter::ExportDefinition.new(:cves_count, label: _('Total CVEs'), callback: ->(host) { host&.insights_attributes&.dig('cves_count') }) |
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.
cves_count is not a valid insights attribute. Just like recommendations, for CVEs we make an API call: https://github.com/jeremylenz/foreman_rh_cloud/blob/083b91d62b519c7921a314fb0bcbfaefb92607a8/webpack/InsightsVulnerabilityHostIndexExtensions/CVECountCell.js#L21-L22
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.
Note that this column is only "relevant" for IoP; it is not available with hosted.
5542621 to
860c8c3
Compare
|
CSV exports are synchronous batch operations in one request. Seems not a fit solution for IoP export which would make hundreds/thousands of sequential API calls during CSV generation depending on the number of hosts. How about we keep the CSV export for hosted only until there is a better solution for IoP export? |
That sounds good to me. In that case, we will not be able to export the CVE column. |
|
CVE data comes from external services rather than the local database. |
What are the changes introduced in this pull request?
Add missing Manage columns to pagelet for CSV export.
Requires theforeman/foreman#10793
Considerations taken when implementing this change?
What are the testing steps for this pull request?
Summary by Sourcery
Add CSV export support for RH Cloud host overview columns related to insights recommendations and CVE counts.
New Features: