Redesign download server UI and add Konflux Containerfile#158
Redesign download server UI and add Konflux Containerfile#158Joeavaikath merged 5 commits intomigtools:oadp-devfrom
Conversation
Replace inline HTML with Go html/template and separated CSS using embed.FS. Group downloads by OS (Linux/macOS/Windows) in table layout with architecture badges, SHA256 checksums (click to copy), and per-command copy buttons. Use official Red Hat brand colors. Also optimize Containerfile to clean build cache within RUN steps and include .sha256 checksum files in the image. Signed-off-by: Joseph <jvaikath@redhat.com>
Uses the OSBS golang builder with FIPS-compliant build flags (CGO_ENABLED=1, strictfipsruntime). Dependencies are prefetched by the Konflux pipeline via Hermeto, so no go mod download is needed. Includes Red Hat metadata labels and license copy. Signed-off-by: Joseph <jvaikath@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a hermetic, multi-stage container build and a templated download server: embeds static assets and CSS, catalogs platform-specific release archives with SHA256 checksums, serves index and downloads, and produces a UBI9 runtime image with archives and a non-root download-server binary. (≤50 words) Changes
Sequence DiagramsequenceDiagram
participant Client
participant Server
participant FS as Archive_FileSystem
participant Template as Template_Renderer
participant Static as Static_Assets
Client->>Server: GET /
Server->>FS: List files in /archives
Server->>FS: Read `*.sha256` checksums
Server->>Server: Parse platforms & build archiveFile collections
Server->>Template: Render index.html with categorized files
Template-->>Server: HTML
Server-->>Client: 200 OK + HTML
Client->>Static: GET /static/style.css
Static-->>Client: CSS
Client->>Server: GET /download/{archive}
Server->>FS: Stream archive file
FS-->>Client: Archive bytes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
|
/cherry-pick release-1.6 |
|
@Joeavaikath: once the present PR merges, I will cherry-pick it on top of 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. |
![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
🧹 Nitpick comments (1)
konflux.Containerfile.download (1)
5-5: Pin immutable base images in the hermetic variant.Floating tags, especially
registry.redhat.io/ubi9/ubi:latest, make repeated builds resolve different base layers over time, which works against the stated hermetic goal. Please pin digests here, or document where Konflux resolves and locks them before build.Also applies to: 24-24
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@konflux.Containerfile.download` at line 5, The FROM line uses a floating tag for the hermetic builder image ("FROM brew.registry.redhat.io/rh-osbs/openshift-golang-builder:rhel_9_golang_1.24"); update this to pin the image to an immutable digest (sha256) or add documentation pointing to where Konflux resolves and locks the digest before builds. Locate the FROM statement in the Containerfile (the builder stage) and replace the tag with the corresponding image@sha256:<digest> OR add a short comment and/or README note explaining the digest-resolution/locking process and where artifacts storing the locked digests live so hermetic builds are reproducible.
🤖 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 80-92: The switch on osName in the loop that builds archiveFile
entries (using parsePlatform, readChecksum and archiveFile) silently defaults
unknown OS values into linuxFiles; change this so unknown/malformed results are
not misclassified: detect when parsePlatform returns an unexpected osName and
either skip adding the archiveFile and log a warning (including the filename and
osName) or append it to a new unknownFiles slice for later inspection instead of
falling through to the linuxFiles case; update the switch/default to perform the
skip/log or push to unknownFiles and ensure any callers that expected only
linux/darwin/windows handle the new behavior.
In `@cmd/downloads/templates/index.html`:
- Line 38: The template currently shows only a checksum prefix and attaches copy
behavior to a non-interactive span; update the markup to render the full SHA256
string in the DOM (use {{.Checksum}} instead of {{slice .Checksum 0 16}}...),
replace the clickable span with a real button (e.g. class "copy-checksum" or
similar) that uses navigator.clipboard.writeText(this.dataset.checksum) or reads
the full checksum from a data-checksum attribute, include an accessible
label/aria-label like "Copy full checksum" and keep the visible short display
handled via CSS if you want truncation, and apply this same change for the other
occurrences referenced (lines 66 and 94) so keyboard users and manual
verification can access the complete digest.
- Around line 103-133: Replace the hard-coded POSIX install block in the
template with OS- and client-specific blocks: detect the OS/template variable
and render a Linux/macOS section that keeps the tar/chmod/mv commands (replace
static strings like "tar -xzf kubectl-oadp_*.tar.gz", "chmod +x kubectl-oadp",
"sudo mv kubectl-oadp /usr/local/bin/") and a Windows section that uses
zip/unzip or .exe guidance and PATH instructions instead of POSIX commands;
update the final verification line so it is neutral or client-aware (use
"kubectl-oadp --help" by default and render "oc oadp --help" only when the oc
client is the advertised artifact) and ensure the copy buttons' data-cmd values
and onclick="copyCmd(this)" targets are set per-platform/command.
---
Nitpick comments:
In `@konflux.Containerfile.download`:
- Line 5: The FROM line uses a floating tag for the hermetic builder image
("FROM
brew.registry.redhat.io/rh-osbs/openshift-golang-builder:rhel_9_golang_1.24");
update this to pin the image to an immutable digest (sha256) or add
documentation pointing to where Konflux resolves and locks the digest before
builds. Locate the FROM statement in the Containerfile (the builder stage) and
replace the tag with the corresponding image@sha256:<digest> OR add a short
comment and/or README note explaining the digest-resolution/locking process and
where artifacts storing the locked digests live so hermetic builds are
reproducible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 25cfad9b-e61b-41b2-a54f-6b8bb4f632fa
📒 Files selected for processing (5)
Containerfile.downloadcmd/downloads/server.gocmd/downloads/static/style.csscmd/downloads/templates/index.htmlkonflux.Containerfile.download
|
/lgtm |
Add RHEL technology icon (Red RGB) and Windows logo SVGs to static assets. Update section headers to use img tags with proper sizing instead of Unicode emoji characters. Signed-off-by: Joseph <jvaikath@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/downloads/static/style.css (1)
48-58: Consider contrast for subtitle and binary-note text.The reduced-opacity white text (
rgba(255,255,255,0.8)andrgba(255,255,255,0.65)) on the red header may fall below WCAG AA contrast ratio (4.5:1), especially the.binary-noteat 0.8rem font size. If accessibility compliance is required, consider increasing opacity or using solid white.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/downloads/static/style.css` around lines 48 - 58, The translucent white text colors for .subtitle (rgba(255,255,255,0.8)) and .binary-note (rgba(255,255,255,0.65)) may not meet WCAG AA contrast on the red header; update the CSS rules for .subtitle and .binary-note to use higher-contrast color values (e.g., increase opacity to 1.0 or switch to solid `#ffffff`) or adjust font-size/weight for .binary-note so the computed contrast ratio meets at least 4.5:1 (verify with a contrast checker) while keeping the selectors .subtitle and .binary-note unchanged.
🤖 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/static/style.css`:
- Around line 169-183: Add accessible keyboard focus styles for interactive
selectors by adding corresponding :focus and/or :focus-visible rules that mirror
the :hover visual feedback and provide a clear visible indicator (e.g., outline
or focus ring and/or matching background change). Update the CSS for
.download-btn, .copy-btn, .logo-link, and .footer a to include these focus
selectors (or a shared utility focus style) so keyboard users see focus; prefer
using :focus-visible to avoid showing the ring on mouse interactions and ensure
contrast and thickness are sufficient for visibility.
---
Nitpick comments:
In `@cmd/downloads/static/style.css`:
- Around line 48-58: The translucent white text colors for .subtitle
(rgba(255,255,255,0.8)) and .binary-note (rgba(255,255,255,0.65)) may not meet
WCAG AA contrast on the red header; update the CSS rules for .subtitle and
.binary-note to use higher-contrast color values (e.g., increase opacity to 1.0
or switch to solid `#ffffff`) or adjust font-size/weight for .binary-note so the
computed contrast ratio meets at least 4.5:1 (verify with a contrast checker)
while keeping the selectors .subtitle and .binary-note unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a36d214d-5baa-4326-b92a-5dd7da62976a
⛔ Files ignored due to path filters (2)
cmd/downloads/static/rhel-icon.svgis excluded by!**/*.svgcmd/downloads/static/windows-logo.svgis excluded by!**/*.svg
📒 Files selected for processing (2)
cmd/downloads/static/style.csscmd/downloads/templates/index.html
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/downloads/templates/index.html
|
/cherry-pick oadp-1.6 |
|
@kaovilai: once the present PR merges, I will cherry-pick it on top of 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. |
Signed-off-by: Joseph <jvaikath@redhat.com>
|
/lgtm |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
konflux.Containerfile.download (2)
24-24: Consider pinning base image for reproducible builds.Using
ubi9/ubi:latestmay produce different images over time as thelatesttag is updated. For a hermetic/reproducible Konflux build, consider pinning to a specific version tag (e.g.,ubi9/ubi:9.4) or using a digest reference.♻️ Suggested change
-FROM registry.redhat.io/ubi9/ubi:latest +FROM registry.redhat.io/ubi9/ubi:9.4Or use a digest for maximum reproducibility:
FROM registry.redhat.io/ubi9/ubi@sha256:<specific-digest>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@konflux.Containerfile.download` at line 24, The Docker base image in the Containerfile currently uses an unpinned tag "FROM registry.redhat.io/ubi9/ubi:latest"; change this to a specific, immutable reference to ensure reproducible builds by replacing the ":latest" tag with a concrete version tag (e.g., ":9.4") or, even better, a digest reference ("@sha256:<digest>") so the FROM line in the Containerfile always resolves to the exact same image.
26-26: Consider simplifying package installation.The
reinstall tzdataapproach works but could be replaced with a simpler install command. If tzdata is already in the base image, installing it again is a no-op; if it's missing, install will add it.♻️ Optional simplification
-RUN dnf -y install openssl && dnf -y reinstall tzdata && dnf clean all +RUN dnf -y install openssl tzdata && dnf clean all🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@konflux.Containerfile.download` at line 26, Replace the RUN line that currently uses "dnf -y reinstall tzdata" with a simpler install invocation: change "RUN dnf -y install openssl && dnf -y reinstall tzdata && dnf clean all" to use "dnf -y install tzdata" (or combine into a single install command such as "dnf -y install openssl tzdata && dnf clean all"); update the command containing "dnf -y reinstall tzdata" so tzdata is installed if missing and avoids an unnecessary reinstall when present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@konflux.Containerfile.download`:
- Line 24: The Docker base image in the Containerfile currently uses an unpinned
tag "FROM registry.redhat.io/ubi9/ubi:latest"; change this to a specific,
immutable reference to ensure reproducible builds by replacing the ":latest" tag
with a concrete version tag (e.g., ":9.4") or, even better, a digest reference
("@sha256:<digest>") so the FROM line in the Containerfile always resolves to
the exact same image.
- Line 26: Replace the RUN line that currently uses "dnf -y reinstall tzdata"
with a simpler install invocation: change "RUN dnf -y install openssl && dnf -y
reinstall tzdata && dnf clean all" to use "dnf -y install tzdata" (or combine
into a single install command such as "dnf -y install openssl tzdata && dnf
clean all"); update the command containing "dnf -y reinstall tzdata" so tzdata
is installed if missing and avoids an unnecessary reinstall when present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d0ac5eb-c69f-4068-bc9f-4df78802fa5b
📒 Files selected for processing (1)
konflux.Containerfile.download
Replace icon+label section headers with plain text labels: "Download for Linux & Unix", "Download for macOS", and "Download for Windows". Delete unused rhel-icon.svg and windows-logo.svg assets. Signed-off-by: Joseph <jvaikath@redhat.com>
Joeavaikath
dismissed stale reviews from shubham-pampattiwar and kaovilai
via
51c301b
|
/lgtm |
|
/cherry-pick oadp-1.6 |
|
@mpryc: once the present PR merges, I will cherry-pick it on top of 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 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 |
|
@mpryc: new pull request created: #163 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. |
|
@Joeavaikath: cannot checkout 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. |
5 participants
Summary
and official Red Hat brand colors
architecture badges, SHA256 checksums (click to copy), and per-command
copy buttons
html/templatewithembed.FSfor bothtemplates and static assets
Containerfile.downloadto clean build cache within RUN stepsand include
.sha256checksum fileskonflux.Containerfile.downloadfor hermetic downstream builds(FIPS-compliant, Hermeto-prefetched dependencies)
Test plan
podman build -f Containerfile.download -t oadp-cli-binaries .ARCHIVE_DIR=cmd/downloads/test-archives go run ./cmd/downloads/localhost:8080renders correctly with OS grouping🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores