Use clean binary names in download server#9
Use clean binary names in download server#9openshift-merge-bot[bot] merged 1 commit intooadp-devfrom
Conversation
📝 WalkthroughWalkthroughThe changes refactor the binary download infrastructure to replace aggregate checksum files with per-file SHA256 sidecar files, rename internal "archive" terminology to "binary" in the download server, add LICENSE file distribution and serving capabilities, and update the download page UI to display licenses when available. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Replace versioned binary names (oadp-vmdp_${VERSION}_${os}_${arch})
with clean names (oadp-vmdp_${os}_${arch}) so users can directly
curl/wget binaries without version/commit hash in the filename.
Switch from single sha256sum.txt to per-file .sha256 sidecar checksums.
Add LICENSE as a downloadable file on the download server page.
Similar to migtools/oadp-cli#174
Co-authored-by: Claude <noreply@anthropic.com>
Signed-off-by: Michal Pryc <mpryc@redhat.com>
mpryc
force-pushed
the
fix-download-server-clean-names
branch
from
a4a2391 to
8e80775
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/downloads/templates/index.html (1)
99-107:⚠️ Potential issue | 🟠 MajorUpdate the install snippets for the clean filename format.
These commands still assume the old versioned pattern (
oadp-vmdp_*_linux_*/oadp-vmdp_*_windows_*.exe). With the newoadp-vmdp_<os>_<arch>names, the copy/paste flow no longer matches the downloaded files.Possible update
- <code class="cmd">chmod +x oadp-vmdp_*_linux_*</code> - <button class="copy-btn" onclick="copyCmd(this)" data-cmd="chmod +x oadp-vmdp_*_linux_*">Copy</button> + <code class="cmd">chmod +x oadp-vmdp_linux_*</code> + <button class="copy-btn" onclick="copyCmd(this)" data-cmd="chmod +x oadp-vmdp_linux_*">Copy</button> ... - <code class="cmd">sudo mv oadp-vmdp_*_linux_* /usr/local/bin/oadp-vmdp</code> - <button class="copy-btn" onclick="copyCmd(this)" data-cmd="sudo mv oadp-vmdp_*_linux_* /usr/local/bin/oadp-vmdp">Copy</button> + <code class="cmd">sudo mv oadp-vmdp_linux_* /usr/local/bin/oadp-vmdp</code> + <button class="copy-btn" onclick="copyCmd(this)" data-cmd="sudo mv oadp-vmdp_linux_* /usr/local/bin/oadp-vmdp">Copy</button> ... - <code class="cmd">Rename-Item oadp-vmdp_*_windows_*.exe oadp-vmdp.exe</code> - <button class="copy-btn" onclick="copyCmd(this)" data-cmd="Rename-Item oadp-vmdp_*_windows_*.exe oadp-vmdp.exe">Copy</button> + <code class="cmd">Rename-Item oadp-vmdp_windows_*.exe oadp-vmdp.exe</code> + <button class="copy-btn" onclick="copyCmd(this)" data-cmd="Rename-Item oadp-vmdp_windows_*.exe oadp-vmdp.exe">Copy</button>Also applies to: 125-126
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/downloads/templates/index.html` around lines 99 - 107, Update the install snippets to use the new filename format instead of the old glob patterns: replace occurrences of "oadp-vmdp_*_linux_*" and "oadp-vmdp_*_windows_*.exe" in the <code> elements and the copy button data-cmd attributes with the new concrete pattern "oadp-vmdp_<os>_<arch>" (and "oadp-vmdp_<os>_<arch>.exe" for Windows); specifically update the chmod, sudo mv, and corresponding button data-cmd strings so the copy/paste commands match the downloaded filenames (also apply the same replacements for the analogous occurrences later in the file).cmd/downloads/server.go (1)
207-213:⚠️ Potential issue | 🟡 Minor
LICENSEis still forced to download.The new page advertises “View License”, but this handler always sets
Content-Disposition: attachment, so most browsers will download it instead of rendering it inline. Either special-caseLICENSEhere or rename the CTA to “Download License”.One server-side fix
- w.Header().Set("Content-Disposition", "attachment; filename="+filename) - - if filename == licenseFile { - w.Header().Set("Content-Type", "text/plain") - } else { - w.Header().Set("Content-Type", "application/octet-stream") - } + if filename == licenseFile { + w.Header().Set("Content-Type", "text/plain; charset=utf-8") + w.Header().Set("Content-Disposition", "inline; filename="+filename) + } else { + w.Header().Set("Content-Type", "application/octet-stream") + w.Header().Set("Content-Disposition", "attachment; filename="+filename) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/downloads/server.go` around lines 207 - 213, The handler always forces download by setting Content-Disposition to "attachment"; change it so that when filename == licenseFile it uses an inline disposition (e.g., "inline; filename="+filename) so browsers will render the LICENSE instead of downloading it, while keeping attachment for other files; look for the variables/headers in this handler (filename, licenseFile, w.Header().Set("Content-Disposition", ...), and the existing Content-Type logic) and only alter the Content-Disposition branch for the license case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/downloads/server.go`:
- Around line 55-58: The startup log conflates discovery errors with an empty
result: update the check after calling discoverBinaries() so you handle the
error and the empty slice separately—if err != nil log/abort with the actual err
included (referencing discoverBinaries() and err), and only if err == nil but
len(files) == 0 call log.Fatal with a message about no binaries found mentioning
binaryDir; this preserves the original behavior while ensuring the discovery
error is logged.
In `@Containerfile.download`:
- Line 76: The sha256 file currently records the absolute path because the
command "sha256sum /archives/$out_name > /archives/$out_name.sha256" is run with
the full path; change it so the checksum file contains the binary's basename (so
checks use local filename). Fix by running sha256sum from the /archives
directory or by invoking sha256sum on the basename (e.g., cd /archives &&
sha256sum "$out_name" > "$out_name.sha256" or sha256sum "$(basename
"$out_name")" > "$(basename "$out_name").sha256"), replacing the current
"sha256sum /archives/$out_name > /archives/$out_name.sha256" invocation.
In `@konflux.Dockerfile`:
- Line 39: The sha256 file is being generated with the absolute path because the
recipe runs sha256sum /archives/$out_name > /archives/$out_name.sha256, which
embeds “/archives/…” into the checksum file; change the command so the checksum
entry uses only the binary basename (use $out_name) before writing the
.sha256—e.g., run sha256sum from inside /archives or run sha256sum on $out_name
(not /archives/$out_name) and write to /archives/$out_name.sha256—so that the
produced file contains the basename and can be verified locally with sha256sum
-c; update the Dockerfile invocation that references sha256sum
/archives/$out_name and $out_name accordingly.
---
Outside diff comments:
In `@cmd/downloads/server.go`:
- Around line 207-213: The handler always forces download by setting
Content-Disposition to "attachment"; change it so that when filename ==
licenseFile it uses an inline disposition (e.g., "inline; filename="+filename)
so browsers will render the LICENSE instead of downloading it, while keeping
attachment for other files; look for the variables/headers in this handler
(filename, licenseFile, w.Header().Set("Content-Disposition", ...), and the
existing Content-Type logic) and only alter the Content-Disposition branch for
the license case.
In `@cmd/downloads/templates/index.html`:
- Around line 99-107: Update the install snippets to use the new filename format
instead of the old glob patterns: replace occurrences of "oadp-vmdp_*_linux_*"
and "oadp-vmdp_*_windows_*.exe" in the <code> elements and the copy button
data-cmd attributes with the new concrete pattern "oadp-vmdp_<os>_<arch>" (and
"oadp-vmdp_<os>_<arch>.exe" for Windows); specifically update the chmod, sudo
mv, and corresponding button data-cmd strings so the copy/paste commands match
the downloaded filenames (also apply the same replacements for the analogous
occurrences later in the file).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 61bb59b5-1c50-47a6-bf57-8573a42d3242
📒 Files selected for processing (4)
Containerfile.downloadcmd/downloads/server.gocmd/downloads/templates/index.htmlkonflux.Dockerfile
| files, err := discoverBinaries() | ||
| if err != nil || len(files) == 0 { | ||
| log.Fatal("No downloadable files found in ", archiveDir) | ||
| log.Fatal("No binaries found in ", binaryDir) | ||
| } |
There was a problem hiding this comment.
Keep the discovery error in the startup log.
Right now an unreadable mount and an empty binary directory both become “No binaries found”, which makes bad permissions or a misconfigured volume much harder to diagnose. Split the branches and include err in the fatal log.
Suggested fix
files, err := discoverBinaries()
-if err != nil || len(files) == 0 {
- log.Fatal("No binaries found in ", binaryDir)
+if err != nil {
+ log.Fatalf("failed to discover binaries in %s: %v", binaryDir, err)
+}
+if len(files) == 0 {
+ log.Fatalf("no binaries found in %s", binaryDir)
}As per coding guidelines, Ensure proper error handling throughout Go code.
📝 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.
| files, err := discoverBinaries() | |
| if err != nil || len(files) == 0 { | |
| log.Fatal("No downloadable files found in ", archiveDir) | |
| log.Fatal("No binaries found in ", binaryDir) | |
| } | |
| files, err := discoverBinaries() | |
| if err != nil { | |
| log.Fatalf("failed to discover binaries in %s: %v", binaryDir, err) | |
| } | |
| if len(files) == 0 { | |
| log.Fatalf("no binaries found in %s", binaryDir) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/downloads/server.go` around lines 55 - 58, The startup log conflates
discovery errors with an empty result: update the check after calling
discoverBinaries() so you handle the error and the empty slice separately—if err
!= nil log/abort with the actual err included (referencing discoverBinaries()
and err), and only if err == nil but len(files) == 0 call log.Fatal with a
message about no binaries found mentioning binaryDir; this preserves the
original behavior while ensuring the discovery error is logged.
| -X github.com/kopia/kopia/repo.BuildGitHubRepo=github.com/openshift/oadp-vmdp" \ | ||
| -o /archives/$out_name \ | ||
| . ; \ | ||
| sha256sum /archives/$out_name > /archives/$out_name.sha256; \ |
There was a problem hiding this comment.
Generate .sha256 files with the binary basename, not /archives/....
sha256sum /archives/$out_name > /archives/$out_name.sha256 writes /archives/$out_name into the sidecar. Once the file is downloaded, sha256sum -c will try to read that absolute path and fail instead of checking the local binary.
Suggested fix
- sha256sum /archives/$out_name > /archives/$out_name.sha256; \
+ (cd /archives && sha256sum "$out_name" > "$out_name.sha256"); \📝 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.
| sha256sum /archives/$out_name > /archives/$out_name.sha256; \ | |
| (cd /archives && sha256sum "$out_name" > "$out_name.sha256"); \ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Containerfile.download` at line 76, The sha256 file currently records the
absolute path because the command "sha256sum /archives/$out_name >
/archives/$out_name.sha256" is run with the full path; change it so the checksum
file contains the binary's basename (so checks use local filename). Fix by
running sha256sum from the /archives directory or by invoking sha256sum on the
basename (e.g., cd /archives && sha256sum "$out_name" > "$out_name.sha256" or
sha256sum "$(basename "$out_name")" > "$(basename "$out_name").sha256"),
replacing the current "sha256sum /archives/$out_name >
/archives/$out_name.sha256" invocation.
| -X github.com/kopia/kopia/repo.BuildGitHubRepo=github.com/openshift/oadp-vmdp" \ | ||
| -o /archives/$out_name \ | ||
| . ; \ | ||
| sha256sum /archives/$out_name > /archives/$out_name.sha256; \ |
There was a problem hiding this comment.
Generate .sha256 files with the binary basename, not /archives/....
sha256sum /archives/$out_name > /archives/$out_name.sha256 writes /archives/$out_name into the sidecar. After a user downloads the pair, sha256sum -c will look for that absolute path and fail instead of verifying the local file.
Suggested fix
- sha256sum /archives/$out_name > /archives/$out_name.sha256; \
+ (cd /archives && sha256sum "$out_name" > "$out_name.sha256"); \📝 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.
| sha256sum /archives/$out_name > /archives/$out_name.sha256; \ | |
| (cd /archives && sha256sum "$out_name" > "$out_name.sha256"); \ |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@konflux.Dockerfile` at line 39, The sha256 file is being generated with the
absolute path because the recipe runs sha256sum /archives/$out_name >
/archives/$out_name.sha256, which embeds “/archives/…” into the checksum file;
change the command so the checksum entry uses only the binary basename (use
$out_name) before writing the .sha256—e.g., run sha256sum from inside /archives
or run sha256sum on $out_name (not /archives/$out_name) and write to
/archives/$out_name.sha256—so that the produced file contains the basename and
can be verified locally with sha256sum -c; update the Dockerfile invocation that
references sha256sum /archives/$out_name and $out_name accordingly.
/lgtm |
|
/override ci/prow/images |
|
@mpryc: Overrode contexts on behalf of mpryc: ci/prow/images 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 kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Joeavaikath, kaovilai, mpryc, shubham-pampattiwar The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/override ci/prow/images |
|
@mpryc: Overrode contexts on behalf of mpryc: ci/prow/images 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 kubernetes-sigs/prow repository. |
4 participants
Summary
oadp-vmdp_${VERSION}_${os}_${arch}) with clean names (oadp-vmdp_${os}_${arch}) so users can directly curl/wget binaries without version/commit hash in the filenamesha256sum.txtto per-file.sha256sidecar checksums for consistency with oadp-cliSimilar to migtools/oadp-cli#174 (adapted for oadp-vmdp — no ppc64le/s390x to drop since they were never shipped)
Files changed
Containerfile.download— clean binary names, per-file.sha256, copy LICENSEkonflux.Dockerfile— same changes for Konflux buildscmd/downloads/server.go— support LICENSE download, removesha256sum.txtfallbackcmd/downloads/templates/index.html— add License sectionTest plan
docker build -f Containerfile.download -t oadp-vmdp-downloads .oadp-vmdp_linux_amd64, notoadp-vmdp_dev_linux_amd64).sha256checksums are generated/download/LICENSE/Summary by CodeRabbit
New Features
Chores