Skip to content
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

Merged
merged 7 commits into from
Aug 28, 2023
Merged

Conversation

jortel
Copy link
Contributor

@jortel jortel commented Aug 16, 2023

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

@jortel jortel marked this pull request as ready for review August 24, 2023 21:40
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>
@jortel jortel requested review from aufi and mansam August 25, 2023 15:30
EnvFileTTL = "FILE_TTL"
EnvAppName = "APP_NAME"
EnvDisconnected = "DISCONNECTED"
EnvAnalysisReportPath = "ANALYSIS_REPORT_PATH"
Copy link
Contributor Author

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>
Copy link
Member

@aufi aufi left a 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())
Copy link
Member

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.

Copy link
Contributor Author

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
Copy link
Member

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

Copy link
Contributor Author

@jortel jortel Aug 28, 2023

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

✨ Download analysis report.
2 participants