-
Notifications
You must be signed in to change notification settings - Fork 303
Fixes #38940 - Implement CSV exports on the hosts overview #11591
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
Open
lfu
wants to merge
1
commit into
Katello:master
Choose a base branch
from
lfu:38427_export_hosts
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+141
−7
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
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:
- The
request.path.start_with?('/new/hosts')checks incsv_columnsandcsv_headersare a bit brittle; consider using a more robust signal (e.g., a controller flag, route helper, or format/context check) to distinguish the new hosts overview from other hosts index endpoints. - The added Katello associations in
included_associationswill now be preloaded for all uses of this controller, not just CSV export or the new hosts overview; consider scoping this to only the relevant actions or formats to avoid unnecessary eager loading. - The Katello CSV column keys and headers are currently inlined in the methods; extracting them into named constants or helper methods would reduce duplication and make future changes to the CSV export schema easier to manage.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `request.path.start_with?('/new/hosts')` checks in `csv_columns` and `csv_headers` are a bit brittle; consider using a more robust signal (e.g., a controller flag, route helper, or format/context check) to distinguish the new hosts overview from other hosts index endpoints.
- The added Katello associations in `included_associations` will now be preloaded for all uses of this controller, not just CSV export or the new hosts overview; consider scoping this to only the relevant actions or formats to avoid unnecessary eager loading.
- The Katello CSV column keys and headers are currently inlined in the methods; extracting them into named constants or helper methods would reduce duplication and make future changes to the CSV export schema easier to manage.
## Individual Comments
### Comment 1
<location> `app/controllers/katello/concerns/hosts_controller_extensions.rb:56-59` </location>
<code_context>
included do
prepend Overrides
def included_associations(include = [])
- [:host_traces] + super
+ katello_associations = [:host_traces, :subscription_facet, :content_facet,
+ :applicable_rpms, :content_view_environments]
+ katello_associations + super
end
</code_context>
<issue_to_address>
**issue (performance):** Broadly prepending multiple associations may introduce unnecessary eager loading and performance overhead.
These additional associations will now be eager loaded for every caller of this helper, not just the new use case. For endpoints that deal with large host sets (e.g., JSON index actions), this could noticeably increase query complexity and memory usage. If only some endpoints need these relations (e.g., CSV export), consider scoping the extra includes to those contexts instead of changing the default for all callers.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
app/controllers/katello/concerns/hosts_controller_extensions.rb
Outdated
Show resolved
Hide resolved
2cd216e to
7b2fec9
Compare
Member
13191a7 to
c502b90
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What are the changes introduced in this pull request?
Requires theforeman/foreman#10793
Considerations taken when implementing this change?
Change should work for both new hosts page and legacy UI page
What are the testing steps for this pull request?
Before this patch and theforeman/foreman#10793
After this patch and theforeman/foreman#10793
Hosts - All Hosts - Legacy UI - Export
The exported file should be the same as the saved exported CSV file A
Hosts - All Hosts - Export
The columns in the exported file should contain the columns exported from CSV file A + CSV file B
Summary by Sourcery
Extend host CSV export to include Katello-related data for the new hosts overview while loading required associations.
New Features:
Enhancements: