Search generated_reports and other improvements to report file paths and dev setup#1155
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideRefactors how report file paths are resolved and adjusts the base storage folder behavior, prioritizing completed and uploaded reports while keeping dev-generated files outside the Foreman repo, plus a minor comment typo fix. Sequence diagram for resolving report file paths during downloadsequenceDiagram
actor User
participant HammerCLI
participant ForemanServer
participant ForemanInventoryUpload
participant Filesystem
User->>HammerCLI: inventory download-report --organization-id
HammerCLI->>ForemanServer: HTTP request for latest report
ForemanServer->>ForemanInventoryUpload: report_file_paths(organization_id)
activate ForemanInventoryUpload
ForemanInventoryUpload->>ForemanInventoryUpload: facts_archive_name(organization_id)
ForemanInventoryUpload->>ForemanInventoryUpload: done_file_path(filename)
ForemanInventoryUpload->>ForemanInventoryUpload: uploads_file_path(filename)
ForemanInventoryUpload->>ForemanInventoryUpload: generated_reports_file_path(filename)
ForemanInventoryUpload->>Filesystem: Dir[done, uploads, generated_reports]
Filesystem-->>ForemanInventoryUpload: matching file list (priority order)
deactivate ForemanInventoryUpload
ForemanInventoryUpload-->>ForemanServer: ordered report paths
ForemanServer->>Filesystem: read first existing file
Filesystem-->>ForemanServer: report archive
ForemanServer-->>HammerCLI: report file content
HammerCLI-->>User: save report to requested path
Class diagram for updated ForemanInventoryUpload moduleclassDiagram
class ForemanInventoryUpload {
+base_folder()
+done_folder()
+done_file_path(filename)
+uploads_folder()
+uploads_file_path(filename)
+generated_reports_folder()
+generated_reports_file_path(filename)
+report_file_paths(organization_id)
+outputs_folder()
+facts_archive_name(organization_id)
}
Flow diagram for selecting base_folder in different environmentsflowchart TD
A[start] --> B[Check for /var/lib/foreman using Dir.glob]
B -->|found| C[use /var/lib/foreman as root]
B -->|not found| D["use File.dirname(Dir.getwd) as root"]
C --> E[Append red_hat_inventory/]
D --> E[Append red_hat_inventory/]
E --> F[Return base_folder path]
F --> G[end]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Using
File.dirname(Dir.getwd)for the devbase_foldercan produce surprising paths when the process is started from a subdirectory; consider anchoring this to a known project root (e.g., Rails.root or a configured base path) instead of the parent of the current working directory. - The report search priority (done → uploads → generated_reports) is now encoded in
report_file_paths; consider adding a brief inline comment or extracting this ordering to a small constant to make the lifecycle assumptions and precedence explicit and easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Using `File.dirname(Dir.getwd)` for the dev `base_folder` can produce surprising paths when the process is started from a subdirectory; consider anchoring this to a known project root (e.g., Rails.root or a configured base path) instead of the parent of the current working directory.
- The report search priority (done → uploads → generated_reports) is now encoded in `report_file_paths`; consider adding a brief inline comment or extracting this ordering to a small constant to make the lifecycle assumptions and precedence explicit and easier to maintain.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
c4ebe6f to
0f5b372
Compare
|
#1156 should fix the test failures. |
0f5b372 to
9d265e3
Compare
|
@jeremylenz |
|
@LadislavVasina1 I've temporarily cherry-picked the commit from #1158 here - see if it works better now. |
Updates report_file_paths to search in priority order (done, uploads, generated) ensuring downloads return the most recent report. Also fixes base_folder to use parent directory in dev environments, keeping generated files outside the Foreman repo directory. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
1e17502 to
42c2937
Compare
|
rebased after the other PR was merged. |
Explains the file lifecycle through generated → uploads → done folders and why the search order prioritizes newest files first. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
42c2937 to
33452b6
Compare
|
updated the PR description to remove the AI slop :) |
LadislavVasina1
left a comment
There was a problem hiding this comment.
ACK, report can be generated and downloaded, robottelo test testing this passes with packit.
nofaralfasi
left a comment
There was a problem hiding this comment.
LGTM!
Tested and works as expected.
- Reports are correctly discovered in the correct folders
- UI "Report saved to" path is showing the correct paths
…and dev setup (theforeman#1155) * Improve report file paths and dev environment setup Updates report_file_paths to search in priority order (done, uploads, generated) ensuring downloads return the most recent report. Also fixes base_folder to use parent directory in dev environments, keeping generated files outside the Foreman repo directory. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Add documentation for report_file_paths search order Explains the file lifecycle through generated → uploads → done folders and why the search order prioritizes newest files first. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
…and dev setup (#1155) * Improve report file paths and dev environment setup Updates report_file_paths to search in priority order (done, uploads, generated) ensuring downloads return the most recent report. Also fixes base_folder to use parent directory in dev environments, keeping generated files outside the Foreman repo directory. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> * Add documentation for report_file_paths search order Explains the file lifecycle through generated → uploads → done folders and why the search order prioritizes newest files first. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>
What are the changes introduced in this pull request?
report_file_pathsto search for reports in priority order: generated_reports → uploads → done, ensuring downloads return the most recent/complete report. Previously,generated_reportswas not searched at all.generated_reports_file_pathhelper method for consistencybase_folderto use parent directory in dev environments, keeping generated files in/home/vagrant/red_hat_inventory/instead of polluting the Foreman repo directory/ver/lib/foreman→/var/lib/foremanConsiderations taken when implementing this change?
The priority order reflects the report lifecycle:
The report files are moved (not copied) from generated > uploaded > done, and overwrite any file with the same name in the destination folder. For this reason:
Hence the ordering above.
This ensures hammer CLI and the frontend download button serve the most up-to-date report available.
Change 2 - Dev environment report location
For dev environments, using
File.dirname(Dir.getwd)instead ofDir.getwdmoves thered_hat_inventory/folder location to keep report files outside the git repository, avoiding clutter ingit status.What are the testing steps for this pull request?
generated_reports/folderhammer insights inventory download-report --organization-id 1 --path /tmp/home/vagrant/red_hat_inventory/instead of/home/vagrant/foreman/red_hat_inventory/🤖 Generated with Claude Code
Summary by Sourcery
Adjust report storage and lookup to prioritize completed reports and keep development artifacts out of the Foreman repo directory.
New Features:
Bug Fixes:
Enhancements: