NO-JIRA: Fix issue in the downloads server: archives are not compressed#16677
NO-JIRA: Fix issue in the downloads server: archives are not compressed#16677nunnatsa wants to merge 1 commit into
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@nunnatsa: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe downloads server now loads embedded YAML config bytes, generates ChangesDownloads server and archive changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 12 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Trivy (0.69.3)Trivy execution failed: 2026-06-24T11:07:36Z FATAL Fatal error run error: fs scan error: scan error: scan failed: failed analysis: post analysis error: post analysis error: kubernetes scan error: fs filter error: fs filter error: walk error range error: stat .golangci.yml: no such file or directory: range error: stat .golangci.yml: no such file or directory Comment |
nunnatsa
force-pushed
the
downloads-fix-compression
branch
from
6d1fb11 to
89d3e6b
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: nunnatsa The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/downloads/config/downloads_config.go (1)
224-247: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHandle archive writer close errors before publishing archives.
tar.Writer.Close,gzip.Writer.Close, andzip.Writer.Closefinalize required archive metadata; ignoring those errors can leave a corrupt.tar.gzor.zipon disk whilearchivesReadyis still closed. As per path instructions,**/*.go: “Never ignore error returns.”Proposed fix
-func createArchive(pathToTargetFile string, format string) error { +func createArchive(pathToTargetFile string, format string) (err error) { targetFileInfo, err := os.Stat(pathToTargetFile) if err != nil { return err } targetFile, err := os.Open(pathToTargetFile) if err != nil { return err } - defer targetFile.Close() + defer func() { + if closeErr := targetFile.Close(); closeErr != nil && err == nil { + err = fmt.Errorf("could not close target file %s: %w", pathToTargetFile, closeErr) + } + }() @@ if format == ".tar.gz" { compressWriter := gzip.NewWriter(file) - defer compressWriter.Close() archiveWriter := tar.NewWriter(compressWriter) - defer archiveWriter.Close() err = addFileToTar(archiveWriter, targetFile, targetFileInfo) if err != nil { return fmt.Errorf("could not create archive for %s: %w", pathToTargetFile, err) } + if err := archiveWriter.Close(); err != nil { + return fmt.Errorf("could not finalize tar archive for %s: %w", pathToTargetFile, err) + } + if err := compressWriter.Close(); err != nil { + return fmt.Errorf("could not finalize gzip archive for %s: %w", pathToTargetFile, err) + } } else if format == ".zip" { archiveWriter := zip.NewWriter(file) - defer archiveWriter.Close() err = addFileToZip(archiveWriter, targetFile, targetFileInfo) if err != nil { return fmt.Errorf("could not create archive for %s: %w", pathToTargetFile, err) } + if err := archiveWriter.Close(); err != nil { + return fmt.Errorf("could not finalize zip archive for %s: %w", pathToTargetFile, err) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/downloads/config/downloads_config.go` around lines 224 - 247, Archive writer close errors are being ignored in the download archive flow, which can leave a bad .tar.gz or .zip marked as ready. Update the archive creation logic in downloads_config.go around the tar.gz and zip branches in the archive-building function to check and return errors from gzip.Writer.Close, tar.Writer.Close, and zip.Writer.Close instead of deferring them blindly. Use the existing archive writer setup and add explicit close handling so any finalize failure prevents publishing a corrupt archive.Sources: Path instructions, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/downloads/config/downloads_config_test.go`:
- Around line 159-163: The archive-reading setup in the test still ignores
errors from os.Open and gzip.NewReader and only closes the file handle, which
can hide corrupted archives and leak resources. Update the test around tarPath
to check and handle both open and gzip reader creation errors explicitly, and
ensure both f and gzw are closed after use. Use the existing tar.NewReader and
hdr, err flow to keep the reader setup clear while adding proper cleanup and
error handling.
- Line 218: The test requests in downloads_config_test use httptest.NewRequest,
which triggers noctx; update this request and the matching one below to use
httptest.NewRequestWithContext with context.Background(). Keep the change
localized to the request setup in the relevant test cases so the existing test
behavior stays the same.
In `@cmd/downloads/config/downloads_config.go`:
- Around line 406-411: The setupArtifactsDirectory function currently logs
symlink creation failures but still continues, which can leave
generateDirFileContents serving a broken LICENSE link. Update
setupArtifactsDirectory so that when os.Symlink fails for ocLicenseFile it
returns that error immediately instead of only calling klog.Errorf, and make
sure the caller path in DownloadsServerConfig respects the returned error and
aborts index generation when the symlink cannot be created.
- Around line 391-395: The download handler in downloads_config.go ignores the
error returned by fmt.Fprintln, which violates the no-ignored-errors rule.
Update the response path in the branch that checks r.Context().Done() to use
http.Error instead of manually calling WriteHeader plus Fprintln, and keep the
existing status code and message behavior in the handler.
In `@cmd/downloads/main.go`:
- Around line 4-11: The downloads server started in main should set a bounded
HTTP header read timeout because the current http.Server configuration leaves
header reads unbounded. Update the server construction in cmd/downloads/main.go
to include ReadHeaderTimeout with an appropriate value that matches normal
client behavior, keeping the rest of the server setup in main unchanged.
- Around line 27-28: The deprecated config-path flag is currently accepted but
ignored without any feedback, which can hide stale configuration. Update the
flag handling in main so that when fs.String("config-path", ...) receives a
non-empty value, the command emits a warning that the flag is deprecated and
ignored; keep the flag for backward compatibility, but ensure the warning is
tied to the parsed config-path value before proceeding.
---
Outside diff comments:
In `@cmd/downloads/config/downloads_config.go`:
- Around line 224-247: Archive writer close errors are being ignored in the
download archive flow, which can leave a bad .tar.gz or .zip marked as ready.
Update the archive creation logic in downloads_config.go around the tar.gz and
zip branches in the archive-building function to check and return errors from
gzip.Writer.Close, tar.Writer.Close, and zip.Writer.Close instead of deferring
them blindly. Use the existing archive writer setup and add explicit close
handling so any finalize failure prevents publishing a corrupt archive.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fb72dbf9-ddab-4d2b-a9bb-5b79f17426cc
📒 Files selected for processing (4)
Dockerfile.downloadscmd/downloads/config/downloads_config.gocmd/downloads/config/downloads_config_test.gocmd/downloads/main.go
| done := make(chan int, 1) | ||
| go func() { | ||
| req := httptest.NewRequest(http.MethodGet, "/amd64/linux/oc.tar", nil) | ||
| req := httptest.NewRequest(http.MethodGet, "/amd64/linux/oc.tar.gz", nil) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
sed -n '180,390p' cmd/downloads/config/downloads_config_test.goRepository: openshift/console
Length of output: 5813
🏁 Script executed:
rg -n "NewRequest\\(|NewRequestWithContext|noctx|httptest.NewRequest" cmd/downloads/config/downloads_config_test.goRepository: openshift/console
Length of output: 374
🏁 Script executed:
sed -n '180,390p' cmd/downloads/config/downloads_config_test.goRepository: openshift/console
Length of output: 5813
🏁 Script executed:
rg -n "NewRequest\\(|NewRequestWithContext|noctx|httptest.NewRequest" cmd/downloads/config/downloads_config_test.goRepository: openshift/console
Length of output: 374
🌐 Web query:
golangci-lint noctx httptest.NewRequest NewRequestWithContext lint behavior
💡 Result:
The noctx linter in golangci-lint is designed to detect function and method calls that are missing a context.Context argument where one is expected [1][2]. Regarding httptest.NewRequest, the noctx linter explicitly flags it to encourage the use of httptest.NewRequestWithContext [3][4]. When noctx is enabled, using httptest.NewRequest will trigger a linting error with a message similar to: "must not be called. use net/http/httptest.NewRequestWithContext" [3][4]. This behavior was explicitly added to noctx in version 0.5.0 to improve context handling in tests [5]. To resolve this linting violation, you should update your code to use httptest.NewRequestWithContext whenever a context is available or required for the test case [3][6]. Top results: [1], [3], [5], [4]
Citations:
- 1: https://github.com/sonatard/noctx
- 2: https://golangci-lint.run/docs/linters/
- 3: https://github.com/99designs/gqlgen-contrib/actions/runs/25009155391
- 4: https://github.com/sonatard/noctx/blob/master/noctx.go
- 5: https://golangci-lint.run/docs/product/changelog/
- 6: https://qiita.com/sonatard/items/ae94d2331447894d1405
🌐 Web query:
golangci-lint noctx httptest.NewRequest NewRequestWithContext lint behavior
💡 Result:
golangci-lint’s noctx linter flags calls that create/send HTTP requests without a context.Context. In practice this means it will warn on net/http/httptest.NewRequest (and other request-creation APIs) because those use an underlying context.Background / no explicit context, while it will not warn when you use context-aware constructors like httptest.NewRequestWithContext (or http.NewRequestWithContext / Request.WithContext), since those explicitly provide a context.[1][2][3] Key behavior details 1) httptest.NewRequest (no explicit context) -> triggers noctx The net/http/httptest package documents that NewRequest “wraps NewRequestWithContext using context.Background,” i.e., there is an implicit context but it is not an explicit caller-supplied one.[3] Because noctx is meant to ensure you pass/propagate context, using this API is exactly the pattern it targets.[2][1] 2) httptest.NewRequestWithContext(ctx,...) (explicit context) -> should not trigger noctx Using the context-aware variant is how you satisfy noctx’s requirement to supply a context.Context to request creation.[3][2] 3) http.NewRequestWithContext(...) and Request.WithContext(...) The noctx project and its golangci-lint docs focus on detecting “function calls without context.Context” and list the net/http/constructors and Request.WithContext as relevant APIs.[2][1] Practical guidance / recommended pattern - In tests, prefer httptest.NewRequestWithContext(ctx, method, target, body) (or create a request with httptest.NewRequest and then call req.WithContext(ctx), depending on what your code is using). - If you need to use httptest.NewRequest for convenience, either accept the lint warning or explicitly document/suppress it (e.g., //nolint:noctx) since the linter intentionally enforces context propagation.[2][1] Note about the “context propagation over HTTP” concern If you create a request with a context, the Go context values are not automatically meaningful across network hops; HTTP protocols don’t carry Go context values directly. That’s separate from noctx’s static rule: noctx cares that a context is present on the request object, not that the context’s values are serialized over the wire.[4] If you share your exact snippet and golangci-lint version/config (especially whether you enable-all / default linters), I can tell you the exact expected diagnostic and the minimal code change to make it pass.
Citations:
- 1: https://pkg.go.dev/github.com/sonatard/noctx
- 2: https://github.com/sonatard/noctx
- 3: https://pkg.go.dev/net/http/httptest
- 4: https://stackoverflow.com/questions/69845005/request-created-with-http-newrequestwithcontext-looses-context-when-passed-to
Use httptest.NewRequestWithContext here and in the matching request below. httptest.NewRequest is flagged by noctx, so switch these test requests to httptest.NewRequestWithContext(context.Background(), ...) to avoid CI failures.
🧰 Tools
🪛 golangci-lint (2.12.2)
[error] 218-218: net/http/httptest.NewRequest must not be called. use net/http/httptest.NewRequestWithContext
(noctx)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/downloads/config/downloads_config_test.go` at line 218, The test requests
in downloads_config_test use httptest.NewRequest, which triggers noctx; update
this request and the matching one below to use httptest.NewRequestWithContext
with context.Background(). Keep the change localized to the request setup in the
relevant test cases so the existing test behavior stays the same.
Source: Linters/SAST tools
| func (artifactsConfig *DownloadsServerConfig) setupArtifactsDirectory() error { | ||
| // symlink file in the temporary directory that points to the openshift license | ||
| os.Symlink(pathToOCLicense, filepath.Join(artifactsConfig.TempDir, ocLicenseFile)) | ||
| err := os.Symlink(pathToOCLicense, filepath.Join(artifactsConfig.TempDir, ocLicenseFile)) | ||
| if err != nil { | ||
| klog.Errorf("can't create symlink to the LICENSE file in %s: %v", artifactsConfig.TempDir, err) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Return the LICENSE symlink error instead of serving a broken license link.
generateDirFileContents always adds the license URL, so continuing after symlink failure leaves the generated index pointing at missing content.
Proposed fix
err := os.Symlink(pathToOCLicense, filepath.Join(artifactsConfig.TempDir, ocLicenseFile))
if err != nil {
- klog.Errorf("can't create symlink to the LICENSE file in %s: %v", artifactsConfig.TempDir, err)
+ return fmt.Errorf("can't create symlink to the LICENSE file in %s: %w", artifactsConfig.TempDir, err)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (artifactsConfig *DownloadsServerConfig) setupArtifactsDirectory() error { | |
| // symlink file in the temporary directory that points to the openshift license | |
| os.Symlink(pathToOCLicense, filepath.Join(artifactsConfig.TempDir, ocLicenseFile)) | |
| err := os.Symlink(pathToOCLicense, filepath.Join(artifactsConfig.TempDir, ocLicenseFile)) | |
| if err != nil { | |
| klog.Errorf("can't create symlink to the LICENSE file in %s: %v", artifactsConfig.TempDir, err) | |
| } | |
| func (artifactsConfig *DownloadsServerConfig) setupArtifactsDirectory() error { | |
| // symlink file in the temporary directory that points to the openshift license | |
| err := os.Symlink(pathToOCLicense, filepath.Join(artifactsConfig.TempDir, ocLicenseFile)) | |
| if err != nil { | |
| return fmt.Errorf("can't create symlink to the LICENSE file in %s: %w", artifactsConfig.TempDir, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/downloads/config/downloads_config.go` around lines 406 - 411, The
setupArtifactsDirectory function currently logs symlink creation failures but
still continues, which can leave generateDirFileContents serving a broken
LICENSE link. Update setupArtifactsDirectory so that when os.Symlink fails for
ocLicenseFile it returns that error immediately instead of only calling
klog.Errorf, and make sure the caller path in DownloadsServerConfig respects the
returned error and aborts index generation when the symlink cannot be created.
| _ "embed" | ||
| "flag" | ||
| "fmt" | ||
|
|
||
| "net/http" | ||
| "os" | ||
| "strconv" | ||
|
|
||
| "k8s.io/klog/v2" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files cmd/downloads/main.go
wc -l cmd/downloads/main.go
cat -n cmd/downloads/main.goRepository: openshift/console
Length of output: 1830
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the downloads server implementation and similar HTTP servers for timeout patterns.
rg -n "ReadHeaderTimeout|ReadTimeout|WriteTimeout|IdleTimeout|http.Server{" cmd frontend pkg -g '!**/vendor/**' -g '!**/node_modules/**'
# Show the full downloads main file if it is short enough.
sed -n '1,220p' cmd/downloads/main.goRepository: openshift/console
Length of output: 353
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the download server file and any related server timeout patterns.
sed -n '1,220p' cmd/downloads/main.go
printf '\n----\n'
rg -n "ReadHeaderTimeout|ReadTimeout|WriteTimeout|IdleTimeout|http.Server{" cmd pkg frontend -g '!**/vendor/**' -g '!**/node_modules/**'Repository: openshift/console
Length of output: 1623
Add ReadHeaderTimeout to the downloads server. cmd/downloads/main.go starts an http.Server with no header-read timeout, so slow clients can hold connections open. Set a bounded timeout that matches expected client behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/downloads/main.go` around lines 4 - 11, The downloads server started in
main should set a bounded HTTP header read timeout because the current
http.Server configuration leaves header reads unbounded. Update the server
construction in cmd/downloads/main.go to include ReadHeaderTimeout with an
appropriate value that matches normal client behavior, keeping the rest of the
server setup in main unchanged.
Source: Path instructions
| // leave it here for backward compatible | ||
| _ = fs.String("config-path", "", "this flag is deprecated and ignored") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Warn when the deprecated config flag is supplied.
Keeping --config-path for compatibility is fine, but silently ignoring a non-empty value can hide stale deployment configuration.
Proposed fix
- _ = fs.String("config-path", "", "this flag is deprecated and ignored")
+ configPath := fs.String("config-path", "", "this flag is deprecated and ignored")
@@
if err := fs.Parse(os.Args[1:]); err != nil {
klog.Fatalf("Failed to parse flags: %v", err)
os.Exit(1)
}
+ if *configPath != "" {
+ klog.Warning("--config-path is deprecated and ignored; using embedded downloads config")
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // leave it here for backward compatible | |
| _ = fs.String("config-path", "", "this flag is deprecated and ignored") | |
| // leave it here for backward compatible | |
| configPath := fs.String("config-path", "", "this flag is deprecated and ignored") | |
| // ... later, after fs.Parse(...) | |
| if *configPath != "" { | |
| klog.Warning("--config-path is deprecated and ignored; using embedded downloads config") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmd/downloads/main.go` around lines 27 - 28, The deprecated config-path flag
is currently accepted but ignored without any feedback, which can hide stale
configuration. Update the flag handling in main so that when
fs.String("config-path", ...) receives a non-empty value, the command emits a
warning that the flag is deprecated and ignored; keep the flag for backward
compatibility, but ensure the warning is tied to the parsed config-path value
before proceeding.
The download server creates zip and tar archives on boot. However, when downloading these archives, their size is the same as the original non-compressed file. This PR fixes this isssue. It also changes the tar file extension to "tar.gz". Few more improvements: embed the configuration file, instead of handling the file: copy it to the image and write code around open and close it with error handling. embed the file makes it all much more simple. deprecate the config-path as it is no longer required. move the port parameter out of the download config as it's not belong there. don't create archives for broken symlinks, to avoid let the user download them. Avoid keeping the handler waiting forever, creating handler leaks. Signed-off-by: Nahshon Unna Tsameret <nunnatsa@redhat.com>
nunnatsa
force-pushed
the
downloads-fix-compression
branch
from
89d3e6b to
943d3e3
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/downloads/config/downloads_config.go (1)
220-248: 🗄️ Data Integrity & Integration | 🟠 MajorHandle archive finalization errors.
defer-ingClose()on the gzip/tar/zip writers drops flush/finalization errors, socreateArchivecan return success while producing a corrupt.tar.gzor.zip.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/downloads/config/downloads_config.go` around lines 220 - 248, The createArchive flow in downloads_config.go is ignoring finalization errors from the gzip, tar, and zip writers because their Close calls are deferred without checking the returned error. Update createArchive (and the addFileToTar/addFileToZip paths it uses) so writer Close/flush errors are captured and returned, rather than allowing a successful return with a corrupt archive. Make sure the archive writer and its wrapping gzip writer are finalized explicitly and any error from those Close calls is propagated.Sources: Path instructions, Linters/SAST tools
♻️ Duplicate comments (1)
cmd/downloads/config/downloads_config_test.go (1)
185-191: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winHandle close errors for both readers in
checkTarFile.Line 185 and Line 191 ignore
Close()errors, which can hide cleanup/stream issues and failerrcheck.Suggested fix
- defer f.Close() + defer func() { + if cerr := f.Close(); cerr != nil { + t.Errorf("failed to close archive %s: %v", tarPath, cerr) + } + }() @@ - defer gzw.Close() + defer func() { + if cerr := gzw.Close(); cerr != nil { + t.Errorf("failed to close gzip reader for %s: %v", tarPath, cerr) + } + }()As per path instructions,
**/*.go: “Never ignore error returns”.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/downloads/config/downloads_config_test.go` around lines 185 - 191, In checkTarFile, the deferred Close calls for both the file handle and the gzip reader are ignoring returned errors, which can hide cleanup failures and violate the never-ignore-errors rule. Update the deferred cleanup around f.Close and gzw.Close to capture and report any close errors, using the existing checkTarFile test flow so failures are surfaced in the same test context.Sources: Path instructions, Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cmd/downloads/config/downloads_config.go`:
- Around line 220-248: The createArchive flow in downloads_config.go is ignoring
finalization errors from the gzip, tar, and zip writers because their Close
calls are deferred without checking the returned error. Update createArchive
(and the addFileToTar/addFileToZip paths it uses) so writer Close/flush errors
are captured and returned, rather than allowing a successful return with a
corrupt archive. Make sure the archive writer and its wrapping gzip writer are
finalized explicitly and any error from those Close calls is propagated.
---
Duplicate comments:
In `@cmd/downloads/config/downloads_config_test.go`:
- Around line 185-191: In checkTarFile, the deferred Close calls for both the
file handle and the gzip reader are ignoring returned errors, which can hide
cleanup failures and violate the never-ignore-errors rule. Update the deferred
cleanup around f.Close and gzw.Close to capture and report any close errors,
using the existing checkTarFile test flow so failures are surfaced in the same
test context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 878be91e-128f-4bb5-a509-89d15564624d
📒 Files selected for processing (4)
Dockerfile.downloadscmd/downloads/config/downloads_config.gocmd/downloads/config/downloads_config_test.gocmd/downloads/main.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/downloads/main.go
- Dockerfile.downloads
|
@nunnatsa: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
The download server creates zip and tar archives on boot. However, when downloading these archives, their size is the same as the original non-compressed file.
This PR fixes this isssue. It also changes the tar file extension to "tar.gz".
Few more improvements:
embed the configuration file, instead of handling
the file: copy it to the image and write code around open and close it with error handling. embed the file makes it all much more simple.
deprecate the config-path as it is no longer required.
move the port parameter out of the download config as it's not belong there.
don't create archives for broken symlinks, to avoid let the user download them.
Analysis / Root cause:
Solution description:
Screenshots / screen recording:
Test setup:
Test cases:
Browser conformance:
Additional info:
Reviewers and assignees:
Summary by CodeRabbit
Summary by CodeRabbit
New Features
.tar.gz) in addition to.zip.Bug Fixes
.tar.gz/.zipand returns503promptly when the request is canceled.