-
Notifications
You must be signed in to change notification settings - Fork 35
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
✨ Add support for download static analysis #475
Conversation
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
EnvFileTTL = "FILE_TTL" | ||
EnvAppName = "APP_NAME" | ||
EnvDisconnected = "DISCONNECTED" | ||
EnvAnalysisReportPath = "ANALYSIS_REPORT_PATH" |
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.
hint: just added EnvAnalysisReportPath
Signed-off-by: Jeff Ortel <jortel@redhat.com>
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.
Added few minor questions, but overall LGTM.
// | ||
// Create an analysis file and returns the path. | ||
func (r *AnalysisWriter) Create(id uint) (path string, err error) { | ||
path = fmt.Sprintf("/tmp/report-%d", rand.Int()) |
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.
Just curious if https://pkg.go.dev/os#CreateTemp would be applicable here.
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.
Adds minimal value but probably worth using.
@@ -0,0 +1,65 @@ | |||
package tar |
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 adding unit test tar writer, but the placement of this test migh need to be updated.
Most of unit tests are located directly in the package they test (like auth), so Makefile test target [0] differentiates unit and other tests based on the package. I'd suggest move this directory content into top-level tar directory (or update Makefile, as you prefer) to ensure the test is executed by make test.
[0] https://github.com/konveyor/tackle2-hub/blob/main/Makefile#L126-L128
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 test feels more like a functional test mainly because it includes canned data and interacts with an external component (the filesystem). The test
directory seems like the appropriate place for functional tests in addition to integration tests.
I will add a target to makefile in a follow up pr.
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Signed-off-by: Jeff Ortel <jortel@redhat.com>
Feature
Add support for composing and downloading the static analysis report.
New route:
/applications/:id/analysis/report
endpoint returns the analysis report tarball.Adds
AnalysisWriter
used to create/write the Analysis resource to a temporary file. This is needed to support being streamed which bounds the memory footprint.Adds
ReportWriter
used to compose and stream the analysis report tarball. The AppLatestReport() delegates handling of the request to the writer.Dockerfile updated to copy the analysis report (tree) from the static-report project image to /tmp/analysis/report. The tree is used when creating the analysis report tarball.
Side Effects
Created a new
tar
package the provides common, symmetrical functionality for reading and writing tar (g-zipped) content streams. This also fixes the problem of existing implementations reading entire files into memory. This will be used by the new endpoint for static reports. The implementation has been improved to eliminate loading the files into memory before zipping.Added
/test/tar
functional tests.Also, the BucketOwner & client getDir() and putDir() endpoints can be stripped down and refactored to delegate to the new
tar
package.Adds: BaseHandler.Attachment() method to simplify setting the Content-Disposition header in multiple places.
closes #354